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:
diff --git i/tests/qemusecuritymock.c w/tests/qemusecuritymock.c
index bb0a85544b..92e7c9ac26 100644
--- i/tests/qemusecuritymock.c
+++ w/tests/qemusecuritymock.c
@@ -66,6 +66,8 @@ static int (*real_close)(int fd);
static int (*real_setfilecon_raw)(const char *path, const char *context);
static int (*real_getfilecon_raw)(const char *path, char **context);
#endif
+static bool (*real_virFileExists)(const char *file);
+
/* Global mutex to avoid races */
@@ -123,6 +125,7 @@ init_syms(void)
VIR_MOCK_REAL_INIT(setfilecon_raw);
VIR_MOCK_REAL_INIT(getfilecon_raw);
#endif
+ VIR_MOCK_REAL_INIT(virFileExists);
/* Intentionally not calling init_hash() here */
}
@@ -382,12 +385,12 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
}
-bool virFileExists(const char *path G_GNUC_UNUSED)
+bool virFileExists(const char *path)
{
VIR_LOCK_GUARD lock = virLockGuardLock(&m);
if (getenv(ENVVAR) == NULL)
- return access(path, F_OK) == 0;
+ return real_virFileExists(path);
init_hash();
if (virHashHasEntry(chown_paths, path))
Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
Michal
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
