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;
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;
--
2.44.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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 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 :| _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
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_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.
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);
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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_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.
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 :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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_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.
>
> 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/MSB53HQTNUEK5RS5EPQB6TF7AAEQZVKA/
Regards,
Jim
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Hi All! Any comments beyond my own on this patch? :-) As said in the self reply, I'm happy to explore other suggestions for squelching the alarming, yet harmless error messages. Regards, Jim 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 > > 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; > > 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; > _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
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@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;
>
> 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;
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
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@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; >> 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@redhat.com> Michal _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
© 2016 - 2024 Red Hat, Inc.