Messages in this thread Patch in this message | | | From | Wenyu Liu <> | Subject | [PATCH] tools api fs: fix the concurrency problem for xxx__mountpoint() | Date | Mon, 13 Sep 2021 03:35:53 -0400 |
| |
When xxx_mountpoint() first acquires the mountpoint path,it also fills the path into fs__entries[idx]->path and set fs__entries[idx]->found to be true.
After that,every time xxx_mountpoint() is executed,the path is gotten directly from fs__entries[idx]->path.
There will be a concurrency problem: The fs__entries[idx]->found has been set by a thread, but the fs__entries[idx]->path has not been completely filled. However, another thread thinks that the mountpoint path has been found and read it from fs__entries[idx]->path.
We found this bug when we repeatedly executed the "perf top" command: When the "perf top" command is executed, multiple threads are created and concurrently execute the func hugetlbfs_mountpoint().
Using memory barrier alone does not solve this problem, because if the current thread thinks the path is not found,it will still trys to find the path and fills it into fs__entries[idx]->path,it also will causes other threads that are getting fs__entries[idx]->path to read a wrong path.
This patch add a mutex_lock when fs__get_mountpoint(fs) is filling the fs->path,and uses memory barrier to ensure the path is completely filled before the fs->found is set.
Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com> Signed-off-by: Wenyu Liu <liuwenyu7@huawei.com> --- tools/lib/api/Makefile | 1 + tools/lib/api/fs/fs.c | 51 +++++++++++++++++++++++++++++++++--------- 2 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/tools/lib/api/Makefile b/tools/lib/api/Makefile index a13e9c7f1fc5..b75ba280dde9 100644 --- a/tools/lib/api/Makefile +++ b/tools/lib/api/Makefile @@ -19,6 +19,7 @@ LIBFILE = $(OUTPUT)libapi.a CFLAGS := $(EXTRA_WARNINGS) $(EXTRA_CFLAGS) CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -U_FORTIFY_SOURCE -fPIC +CFLAGS += -lpthread ifeq ($(DEBUG),0) ifeq ($(CC_NO_CLANG), 0) diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c index 82f53d81a7a7..65d19de3911c 100644 --- a/tools/lib/api/fs/fs.c +++ b/tools/lib/api/fs/fs.c @@ -12,6 +12,9 @@ #include <fcntl.h> #include <unistd.h> #include <sys/mount.h> +#include <pthread.h> +#include <asm/barrier.h> +#include <linux/compiler.h> #include "fs.h" #include "debug-internal.h" @@ -92,6 +95,7 @@ struct fs { bool found; bool checked; long magic; + pthread_mutex_t lock; }; enum { @@ -113,43 +117,69 @@ static struct fs fs__entries[] = { .mounts = sysfs__fs_known_mountpoints, .magic = SYSFS_MAGIC, .checked = false, + .lock = PTHREAD_MUTEX_INITIALIZER, }, [FS__PROCFS] = { .name = "proc", .mounts = procfs__known_mountpoints, .magic = PROC_SUPER_MAGIC, .checked = false, + .lock = PTHREAD_MUTEX_INITIALIZER, }, [FS__DEBUGFS] = { .name = "debugfs", .mounts = debugfs__known_mountpoints, .magic = DEBUGFS_MAGIC, .checked = false, + .lock = PTHREAD_MUTEX_INITIALIZER, }, [FS__TRACEFS] = { .name = "tracefs", .mounts = tracefs__known_mountpoints, .magic = TRACEFS_MAGIC, .checked = false, + .lock = PTHREAD_MUTEX_INITIALIZER, }, [FS__HUGETLBFS] = { .name = "hugetlbfs", .mounts = hugetlbfs__known_mountpoints, .magic = HUGETLBFS_MAGIC, .checked = false, + .lock = PTHREAD_MUTEX_INITIALIZER, }, [FS__BPF_FS] = { .name = "bpf", .mounts = bpf_fs__known_mountpoints, .magic = BPF_FS_MAGIC, .checked = false, + .lock = PTHREAD_MUTEX_INITIALIZER, }, }; +static void fs_fill_path(struct fs *fs, const char *path, size_t len) +{ + if (len >= sizeof(fs->path)) + len = sizeof(fs->path) - 1; + + pthread_mutex_lock(&fs->lock); + if (fs->found) { + pthread_mutex_unlock(&fs->lock); + return; + } + strncpy(fs->path, path, len); + fs->path[len] = '\0'; + /* Make sure the path is filled before fs->found is set */ + smp_wmb(); + fs->found = true; + pthread_mutex_unlock(&fs->lock); +} + + static bool fs__read_mounts(struct fs *fs) { bool found = false; char type[100]; + char path[PATH_MAX]; FILE *fp; fp = fopen("/proc/mounts", "r"); @@ -158,15 +188,17 @@ static bool fs__read_mounts(struct fs *fs) while (!found && fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n", - fs->path, type) == 2) { + path, type) == 2) { - if (strcmp(type, fs->name) == 0) + if (strcmp(type, fs->name) == 0) { + fs_fill_path(fs, path, sizeof(path) - 1); found = true; + } } fclose(fp); fs->checked = true; - return fs->found = found; + return found; } static int fs__valid_mount(const char *fs, long magic) @@ -188,8 +220,7 @@ static bool fs__check_mounts(struct fs *fs) ptr = fs->mounts; while (*ptr) { if (fs__valid_mount(*ptr, fs->magic) == 0) { - fs->found = true; - strcpy(fs->path, *ptr); + fs_fill_path(fs, *ptr, strlen(*ptr)); return true; } ptr++; @@ -227,10 +258,7 @@ static bool fs__env_override(struct fs *fs) if (!override_path) return false; - fs->found = true; - fs->checked = true; - strncpy(fs->path, override_path, sizeof(fs->path) - 1); - fs->path[sizeof(fs->path) - 1] = '\0'; + fs_fill_path(fs, override_path, sizeof(fs->path) - 1); return true; } @@ -252,8 +280,11 @@ static const char *fs__mountpoint(int idx) { struct fs *fs = &fs__entries[idx]; - if (fs->found) + if (READ_ONCE(fs->found)) { + /* Make sure the path is filled completely */ + smp_rmb(); return (const char *)fs->path; + } /* the mount point was already checked for the mount point * but and did not exist, so return NULL to avoid scanning again. -- 2.27.0
| |