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@xxxxxxxx>
---
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_selinux.c?ref_type=heads#L1494
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.
