On 3/9/24 01:06, Jim Fehlig wrote: > On 3/8/24 16:26, 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 > > I've already s/harmles/harmless in my local branch. > >> >> 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. > > BTW, I'm happy to explore other suggestions for squelching these errors. > >> >> 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; >> diff --git a/src/security/security_selinux.c >> b/src/security/security_selinux.c >> index ffad058d9a..b21986cb7e 100644 >> --- a/src/security/security_selinux.c >> +++ b/src/security/security_selinux.c >> @@ -2915,11 +2915,11 @@ >> virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, >> rc = -1; >> } >> - if (def->os.kernel && >> + if (def->os.kernel && virFileExists(def->os.kernel) && >> virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, >> true) < 0) >> rc = -1; >> - if (def->os.initrd && >> + if (def->os.initrd && virFileExists(def->os.initrd) && >> virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, >> true) < 0) >> rc = -1; >> > > If this is acceptable, I can squash in a comment such as below that > explains the logic. > > Regards, > Jim > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index be606c6f33..3e3d7c00b6 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -1993,6 +1993,10 @@ virSecurityDACRestoreAllLabel(virSecurityManager > *mgr, > rc = -1; > } > > + /* In scenarios like VM installation, tooling such as virt-install may > + * have already removed the install kernel/initrd. Ensure they still > + * exist before relabeling. > + */ > if (def->os.kernel && virFileExists(def->os.kernel) && > virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0) > rc = -1; > diff --git a/src/security/security_selinux.c > b/src/security/security_selinux.c > index b21986cb7e..495d5aa01c 100644 > --- a/src/security/security_selinux.c > +++ b/src/security/security_selinux.c > @@ -2915,6 +2915,10 @@ > virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, > rc = -1; > } > > + /* In scenarios like VM installation, tooling such as virt-install may > + * have already removed the install kernel/initrd. Ensure they still > + * exist before relabeling. > + */ > if (def->os.kernel && virFileExists(def->os.kernel) && > virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) > rc = -1; Yes, please add these comments. Unfortunately, I don't have a better solution. Maybe, this virFileExist() check can be moved to virSecuritySELinuxRestoreFileLabel() and virSecurityDACRestoreFileLabel()? That would solve this problem for other files too. One thing to be aware though - virSecurityDACRestoreFileLabelInternal() might work with a virStorageSource* and is specifically designed to not call 'chown' but a chownCallback() in same cases (I guess it has to do with RBD? ceph? something like that). But I guess we can so something similar to what's already present there: if (path && !virFileExist(path) return 0; But if you want to go with your version, that's okay too. Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx> Michal _______________________________________________ Devel mailing list -- devel@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
