[PATCH 0/4] util: Fix race when checking whether a path is on a shared FS
See 2/4 for details. Jiri Denemark (4): tests: Test virFileIsSharedFSOverride util: Fix race condition in virFileIsSharedFSType util: Fix race condition in virFileIsSharedFSOverride util: Rework virFileIsSharedFSOverride using virFileCheckParents src/util/virfile.c | 133 +++++++++++++++++++++++++++++--------------- tests/virfiletest.c | 69 +++++++++++++++++++++++ 2 files changed, 156 insertions(+), 46 deletions(-) -- 2.52.0
From: Jiri Denemark <jdenemar@redhat.com> Technically virFileIsSharedFSOverride is available on any OS, but we need a mocked realpath() to test it. Because the virfilemock library also mocks statfs() which is only available on Linux, we don't even try to load the library anywhere else. Thus we need to skip testing virFileIsSharedFSOverride on non-Linux too. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- tests/virfiletest.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/tests/virfiletest.c b/tests/virfiletest.c index e05925a321..ccd76a3fac 100644 --- a/tests/virfiletest.c +++ b/tests/virfiletest.c @@ -329,6 +329,55 @@ testFileIsSharedFSType(const void *opaque G_GNUC_UNUSED) } +static const char *shared_filesystems[] = { + "/run/user/501/gvfs", + "/nfs", + "/gluster", + "/ceph/multi", + "/gpfs/data/blaf", + "/quobyte", + NULL, +}; + +static int +testFileIsSharedFSOverride(const void *opaque G_GNUC_UNUSED) +{ +#ifndef __linux__ + return EXIT_AM_SKIP; +#else + const struct testFileIsSharedFSType *data = opaque; + g_autofree char *mtabFile = NULL; + bool actual; + int ret = -1; + + /* mtab is used by mocked realpath to decide whether a given path exists */ + mtabFile = g_strdup_printf(abs_srcdir "/virfiledata/%s", data->mtabFile); + + if (!g_setenv("LIBVIRT_MTAB", mtabFile, true)) { + fprintf(stderr, "Unable to set env variable\n"); + goto cleanup; + } + + actual = virFileIsSharedFSOverride(data->filename, + (char * const *) shared_filesystems); + + if (actual != data->expected) { + fprintf(stderr, "FS of '%s' is %s. Expected: %s\n", + data->filename, + actual ? "shared" : "not shared", + data->expected ? "shared" : "not shared"); + goto cleanup; + } + + ret = 0; + + cleanup: + g_unsetenv("LIBVIRT_MTAB"); + return ret; +#endif +} + + static int mymain(void) { @@ -439,6 +488,26 @@ mymain(void) DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gpfs/data", true); DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/quobyte", true); +#define DO_TEST_FILE_IS_SHARED_FS_OVERRIDE(mtab, file, exp) \ + do { \ + struct testFileIsSharedFSType data = { \ + .mtabFile = mtab, .filename = file, .expected = exp \ + }; \ + if (virTestRun(virTestCounterNext(), testFileIsSharedFSOverride, &data) < 0) \ + ret = -1; \ + } while (0) + + virTestCounterReset("testFileIsSharedFSOverride "); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts2.txt", "/boot/vmlinuz", false); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts2.txt", "/run/user/501/gvfs/some/file", true); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts3.txt", "/nfs/file", true); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts3.txt", "/gluster/file", true); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts3.txt", "/some/symlink/file", true); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts3.txt", "/ceph/file", false); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts3.txt", "/ceph/multi/file", true); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts3.txt", "/gpfs/data", false); + DO_TEST_FILE_IS_SHARED_FS_OVERRIDE("mounts3.txt", "/quobyte", true); + return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS; } -- 2.52.0
From: Jiri Denemark <jdenemar@redhat.com> virFileIsSharedFSType could end up calling statfs on a path that no longer exists and return an error. If this happens for a path on a shared filesystem, the caller may incorrectly consider the path as non-shared. Specifically, when starting a domain with TPM enabled and deciding whether its vTPM state is stored on a shared storage, the race could cause qemuTPMEmulatorBuildCommand to consider the state to be non-shared. This means swtpm would be started without --migration even when the state is actually stored on a shared storage and any attempt to migrate such domain would fail with Operation not supported: the running swtpm does not support migration with shared storage In fact, any caller of virFileGetExistingParent contained an inherent TOCTOU race condition as the existing parent of a given path return by virFileGetExistingParent may no longer exist at the time the caller wants to check it. This patch introduces a new virFileCheckParents API which is almost identical to virFileGetExistingParent, but uses a supplied callback to check each path. This new API is used in virFileIsSharedFSType to avoid the race. The old function will later be completely removed once all callers are switched to the new one. Fixes: 05526b50909ff50c16e13a0b5580d41de74e3d59 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virfile.c | 71 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index f195d02e29..b0f4041df4 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3445,6 +3445,63 @@ virFileRemoveLastComponent(char *path) } +/* Check callback for virFileCheckParents */ +typedef bool (*virFileCheckParentsCallback)(const char *dirpath, + void *opaque); + +/** + * virFileCheckParents: + * @path: path to check + * @parent: where to store the closest parent satisfying the check + * @check: callback called on parent paths + * @opaque: data for the @check callback + * + * Calls @check on the @path and its parent paths until it returns true or a + * root directory is reached. When @check returns true, the @parent (if + * non-NULL) will be set to a copy of the corresponding path. The caller is + * responsible for freeing it. + * + * Returns 0 on success (@parent set), + * -1 on invalid input, + * -2 when no path (including "/") satisfies the @check. + */ +static int +virFileCheckParents(const char *path, + char **parent, + virFileCheckParentsCallback check, + void *opaque) +{ + g_autofree char *dirpath = g_strdup(path); + char *p = NULL; + bool checkOK; + + checkOK = check(dirpath, opaque); + + while (!checkOK && p != dirpath) { + if (!(p = strrchr(dirpath, G_DIR_SEPARATOR))) { + virReportSystemError(EINVAL, + _("Invalid absolute path '%1$s'"), path); + return -1; + } + + if (p == dirpath) + *(p + 1) = '\0'; + else + *p = '\0'; + + checkOK = check(dirpath, opaque); + } + + if (!checkOK) + return -2; + + if (parent) + *parent = g_steal_pointer(&dirpath); + + return 0; +} + + static char * virFileGetExistingParent(const char *path) { @@ -3599,6 +3656,14 @@ static const struct virFileSharedFsData virFileSharedFs[] = { }; +static bool +virFileCheckParentsStatFS(const char *path, + void *opaque) +{ + return statfs(path, (struct statfs *) opaque) == 0; +} + + int virFileIsSharedFSType(const char *path, unsigned int fstypes) @@ -3607,11 +3672,13 @@ virFileIsSharedFSType(const char *path, struct statfs sb; long long f_type = 0; size_t i; + int rc; - if (!(dirpath = virFileGetExistingParent(path))) + if ((rc = virFileCheckParents(path, &dirpath, + virFileCheckParentsStatFS, &sb)) == -1) return -1; - if (statfs(dirpath, &sb) < 0) { + if (rc != 0) { virReportSystemError(errno, _("cannot determine filesystem for '%1$s'"), path); -- 2.52.0
From: Jiri Denemark <jdenemar@redhat.com> Switch virFileIsSharedFSOverride to use virFileCheckParents to avoid a race which could result in virFileCanonicalizePath to be called on a path that does not exist anymore. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virfile.c | 59 +++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 37 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index b0f4041df4..ea68215655 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3502,33 +3502,6 @@ virFileCheckParents(const char *path, } -static char * -virFileGetExistingParent(const char *path) -{ - g_autofree char *dirpath = g_strdup(path); - char *p = NULL; - - /* Try less and less of the path until we get to a directory we can access. - * Even if we don't have 'x' permission on any directory in the path on the - * NFS server (assuming it's NFS), we will be able to stat the mount point. - */ - while (!virFileExists(dirpath) && p != dirpath) { - if (!(p = strrchr(dirpath, '/'))) { - virReportSystemError(EINVAL, - _("Invalid relative path '%1$s'"), path); - return NULL; - } - - if (p == dirpath) - *(p + 1) = '\0'; - else - *p = '\0'; - } - - return g_steal_pointer(&dirpath); -} - - #ifdef __linux__ # ifndef NFS_SUPER_MAGIC @@ -3875,6 +3848,17 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs, } +static bool +virFileCheckParentsCanonicalize(const char *path, + void *opaque) +{ + char **canonical = opaque; + + *canonical = virFileCanonicalizePath(path); + return !!*canonical; +} + + /** * virFileIsSharedFSOverride: * @path: Path to check @@ -3888,24 +3872,25 @@ virFileIsSharedFSOverride(const char *path, char *const *overrides) { g_autofree char *dirpath = NULL; - g_autofree char *existing = NULL; char *p = NULL; + int rc; if (!path || path[0] != '/' || !overrides) return false; /* We only care about the longest existing sub-path. Further components - * may will later be created by libvirt will not magically become a shared - * filesystem. */ - if (!(existing = virFileGetExistingParent(path))) + * that may later be created by libvirt will not magically become a shared + * filesystem. Overrides have been canonicalized ahead of time, so we need + * to do the same for the provided path or we'll never be able to find a + * match if symlinks are involved. + */ + rc = virFileCheckParents(path, NULL, + virFileCheckParentsCanonicalize, &dirpath); + if (rc == -1) return false; - /* Overrides have been canonicalized ahead of time, so we need to - * do the same for the provided path or we'll never be able to - * find a match if symlinks are involved */ - if (!(dirpath = virFileCanonicalizePath(existing))) { - VIR_DEBUG("Cannot canonicalize parent '%s' of path '%s'", - existing, path); + if (rc != 0) { + VIR_DEBUG("Cannot canonicalize path '%s'", path); return false; } -- 2.52.0
From: Jiri Denemark <jdenemar@redhat.com> The newly introduced virFileCheckParents is generic enough to be used for checking whether a specific path or any of its parents is included in the overrides array. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virfile.c | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index ea68215655..9316606ce8 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3859,6 +3859,14 @@ virFileCheckParentsCanonicalize(const char *path, } +static bool +virFileCheckParentsInOverrides(const char *path, + void *opaque) +{ + return g_strv_contains((const char *const *) opaque, path); +} + + /** * virFileIsSharedFSOverride: * @path: Path to check @@ -3872,7 +3880,6 @@ virFileIsSharedFSOverride(const char *path, char *const *overrides) { g_autofree char *dirpath = NULL; - char *p = NULL; int rc; if (!path || path[0] != '/' || !overrides) @@ -3894,29 +3901,11 @@ virFileIsSharedFSOverride(const char *path, return false; } - if (g_strv_contains((const char *const *) overrides, dirpath)) - return true; + if (virFileCheckParents(dirpath, NULL, virFileCheckParentsInOverrides, + (void *) overrides) < 0) + return false; - /* Continue until we've scanned the entire path */ - while (p != dirpath) { - - /* Find the last slash */ - if ((p = strrchr(dirpath, '/')) == NULL) - break; - - /* Truncate the path by overwriting the slash that we've just - * found with a null byte. If it is the very first slash in - * the path, we need to handle things slightly differently */ - if (p == dirpath) - *(p+1) = '\0'; - else - *p = '\0'; - - if (g_strv_contains((const char *const *) overrides, dirpath)) - return true; - } - - return false; + return true; } -- 2.52.0
On 12/9/25 16:02, Jiri Denemark via Devel wrote:
See 2/4 for details.
Jiri Denemark (4): tests: Test virFileIsSharedFSOverride util: Fix race condition in virFileIsSharedFSType util: Fix race condition in virFileIsSharedFSOverride util: Rework virFileIsSharedFSOverride using virFileCheckParents
src/util/virfile.c | 133 +++++++++++++++++++++++++++++--------------- tests/virfiletest.c | 69 +++++++++++++++++++++++ 2 files changed, 156 insertions(+), 46 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Tue, Dec 09, 2025 at 16:41:37 +0100, Michal Prívozník wrote:
On 12/9/25 16:02, Jiri Denemark via Devel wrote:
See 2/4 for details.
Jiri Denemark (4): tests: Test virFileIsSharedFSOverride util: Fix race condition in virFileIsSharedFSType util: Fix race condition in virFileIsSharedFSOverride util: Rework virFileIsSharedFSOverride using virFileCheckParents
src/util/virfile.c | 133 +++++++++++++++++++++++++++++--------------- tests/virfiletest.c | 69 +++++++++++++++++++++++ 2 files changed, 156 insertions(+), 46 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Oops, I forgot to add this to the commits before pushing :-( Jirka
participants (3)
-
Jiri Denemark -
Jiří Denemark
-
Michal Prívozník