On 3/27/24 08:01, Daniel P. Berrangé wrote:
On Mon, Mar 25, 2024 at 07:13:05PM -0600, Jim Fehlig wrote:
On 3/21/24 08:57, Daniel P. Berrangé wrote:
On Fri, Mar 08, 2024 at 04:26:27PM -0700, Jim Fehlig wrote:
When performing an install, it's common for tooling such as virt-install to remove the install kernel/initrd once they are successfully booted and the domain has been redefined to boot without them. After the installation is complete and the domain is rebooted/shutdown, the DAC and selinux security drivers attempt to restore labels on the now deleted files. It's harmles wrt functionality, but results in error messages such as
Mar 08 12:40:37 virtqemud[5639]: internal error: child reported (status=125): unable to stat: /var/lib/libvirt/boot/vir> Mar 08 12:40:37 virtqemud[5639]: unable to stat: /var/lib/libvirt/boot/virtinst-yvp19moo-linux: No such file or directo> Mar 08 12:40:37 virtqemud[5639]: Unable to run security manager transaction
Avoid the messages by checking if the kernel and initrd still exist before including them in the restore label transaction.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_dac.c | 4 ++-- src/security/security_selinux.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b8130630f..be606c6f33 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1993,11 +1993,11 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, rc = -1; } - if (def->os.kernel && + if (def->os.kernel && virFileExists(def->os.kernel) && virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) rc = -1; - if (def->os.initrd && + if (def->os.initrd && virFileExists(def->os.initrd) && virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0) rc = -1;
I wonder if this scenario is conceptually relevant to other files though.
eg someone created a qcow2 overlay for the disk, to capture writes, and then immediatley unlinked it as they wanted to discard them. ie manual equivalent of QEMU's -snapshot arg.
Should we instead plumb something in so that the 'stat()' failure gets silently ignored when it is ENOENT, on a "restore" code path
Something like the following works for the DAC driver
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 567be4bd23..28afa4846b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -667,7 +667,8 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, const virStorageSource *src, const char *path, uid_t uid, - gid_t gid) + gid_t gid, + bool ignoreNoEnt) { int rc = 0;
@@ -689,6 +690,9 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, return 0;
if (stat(path, &sb) < 0) { + if (errno == ENOENT && ignoreNoEnt) + return 0; + virReportSystemError(errno, _("unable to stat: %1$s"), path); return -1; } @@ -787,7 +791,7 @@ virSecurityDACSetOwnership(virSecurityManager *mgr, VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid, false) < 0) goto error;
return 0; @@ -847,7 +851,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr, VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld", NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); + return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid, true); }
The selinux driver is not as simple. I suspect the call to virFileResolveLink() would fail if the file no longer exists, well before the call to stat()
https://gitlab.com/libvirt/libvirt/-/blob/master/src/security/security_selin...
Adding an 'ignoreNoEnt' parameter to virSecuritySELinuxRestoreFileLabel, plumbing it through to virFileResolveLink, and adjusting all call sites seems a bit overkill.
An FYI: while testing the above patch, I thought something simple like the below hack was a clever fix, but it causes several qemusecuritytest failures.
This simple patch is not unreasonable. Might just be that test that has bad assumptions that need fixing ?
Thanks for confirming the sanity of the change. With that, I went about figuring out how the test works and will write it here for future reference. qemusecuritytest instantiates virDomainObj based on config in tests/qemuxmlconfdata/*.xml. It then calls qemuSecuritySetAllLabel to label everything, followed by qemuSecurityRestoreAllLabel to restore the labels. xattr, chown, and selinux {get,set}filecon_raw are mocked in qemusecuritymock.c, where a VIR_MOCK_STAT_HOOK is also defined. Using the DAC driver as an example, when setting labels virSecurityDACSetOwnershipInternal calls stat on a path, where the mock hook adds the path to a hashtable with associated UID:GID set to DEFAULT_{UID,GID}. virSecurityDACSetOwnershipInternal() later calls chown on the path to set UID:GID. The mocked chown() updates the new values in the hashtable. On restore, the values get changed back to DEFAULT_{UID,GID}. Finally, the checkPaths() function in qemusecuritymock.c verifies everything was restored to DEFAULT_{UID,GID}. Assuming I've understood the test and described it correctly, it's clear why adding the virFileExists() in the restore path of the drivers results in the test failure. None of the files actually exist, so the restore was never executed. I've sent a V2 of the patch, which mocks virFileExists to return true when encountering a file previously seen by the mocked stat, chown, and {get,set}filecon_raw functions https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/MSB53... Regards, Jim