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 ?
Regards, Jim
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 567be4bd23..4e850e219e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -825,6 +825,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManager *mgr, virStorageSourceIsLocalStorage(src)) path = src->path;
+ if (!virFileExists(path)) + return 0; + /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b49af26e49..aaec34ff8b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1488,6 +1488,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, */ if (!path) return 0; + if (!virFileExists(path)) + return 0;
VIR_INFO("Restoring SELinux context on '%s'", path);
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|