On 4/8/24 23:10, Jim Fehlig wrote:
> On 4/8/24 10:48 AM, Jim Fehlig wrote:
>> On 4/8/24 6:18 AM, Michal Prívozník wrote:
>>> On 4/2/24 00:14, 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
>>>>
>>>> Add a check for file existence to the virSecurity*RestoreFileLabel
>>>> functions,
>>>> and avoid relabeling if the file is no longer available. Skipping
>>>> the restore
>>>> caused failures in qemusecuritytest, which mocks stat, chown, etc as
>>>> part of
>>>> ensuring the security drivers properly restore labels. virFileExists
>>>> is now
>>>> mocked in qemusecuritymock.c to return true when passed a file
>>>> previously
>>>> seen by the mocked stat, chown, etc functions.
>>>>
>>>> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
>>>> ---
>>>> src/security/security_dac.c | 3 +++
>>>> src/security/security_selinux.c | 2 ++
>>>> tests/qemusecuritymock.c | 18 ++++++++++++++++++
>>>> 3 files changed, 23 insertions(+)
>>>>
>>>
>>>
>>> Looks better now, thanks!
>>>
>>>> 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);
>>>> diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c
>>>> index 80d59536b1..bb0a85544b 100644
>>>> --- a/tests/qemusecuritymock.c
>>>> +++ b/tests/qemusecuritymock.c
>>>> @@ -382,6 +382,24 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
>>>> }
>>>> +bool virFileExists(const char *path G_GNUC_UNUSED)
>>>
>>> This argument is used, so please drop the unused annotation.
>>>
>>>> +{
>>>> + VIR_LOCK_GUARD lock = virLockGuardLock(&m);
>>>> +
>>>> + if (getenv(ENVVAR) == NULL)
>>>> + return access(path, F_OK) == 0;
>>>
>>> I wonder whether we should call real virFileExists() here. That way, if
>>> a test would chain mocks they can work (though both would probably be
>>> mocking access()). I'll leave it up to you whether you want to squash in
>>> the following:
>>
>> I considered calling the real virFileExists, but was being lazy since
>> the impl is a one-liner :-). Agree it's better, for consistency if
>> nothing else. I've squashed in your suggestion and pushed the patch.
>
> Weird. Calling the real virFileExists results in a segfault in some of
> the CI jobs, e.g.
>
> https://gitlab.com/libvirt/libvirt/-/jobs/6574951832
Ah, what I forgot in my code is a call to init_syms(). Like this:
bool virFileExists(const char *path)
{
VIR_LOCK_GUARD lock = virLockGuardLock(&m);
if (getenv(ENVVAR) == NULL) {
init_syms();
return real_virFileExists(path);
>
> I wasn't able to quickly reproduce it on my development setup and don't
> have time ATM to investigate further. Calling access(2) instead of the
> real virFileExists, as the original patch did, avoids the CI failures
I guess it's because on Linux some other mocked function is called first
and thus real_virFileExists is initialized properly.
I've posted a patch here:
https://lists.libvirt.org/archives/list/devel@xxxxxxxxxxxxxxxxx/thread/J3VKCRJQ4BLWHW4KTVAXKDPPKHMWMOUG/
Michal
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
