From nobody Sun Feb 8 05:42:55 2026 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A8C5BC7EE2E for ; Fri, 9 Jun 2023 22:40:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230094AbjFIWka (ORCPT ); Fri, 9 Jun 2023 18:40:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47692 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229486AbjFIWk1 (ORCPT ); Fri, 9 Jun 2023 18:40:27 -0400 Received: from mail-yb1-xb4a.google.com (mail-yb1-xb4a.google.com [IPv6:2607:f8b0:4864:20::b4a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4506C3A80 for ; Fri, 9 Jun 2023 15:40:26 -0700 (PDT) Received: by mail-yb1-xb4a.google.com with SMTP id 3f1490d57ef6-bb0cb2e67c5so2900413276.0 for ; Fri, 09 Jun 2023 15:40:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1686350425; x=1688942425; h=cc:to:from:subject:mime-version:message-id:date:from:to:cc:subject :date:message-id:reply-to; bh=VJ/oCWzA+yOfgNosg7yCARX3f3WD532lX89SwknwbFE=; b=up80m5SgFVzhZozVGd7h5+yoFepYS10VKiUM7q0OAiqDntEVj7GafQ14HZn+uPBk64 6n2gAkyi3+fXuv8Sw/b3tlssJq9ft2kCgJYnz88+DTzbvRS7vVqkJOGeKHJopvjSecWt i00r7gSkfgUm36l++A7YZ1Yl8Q0zClAmnzTOvJK0u51yoWHaJVvU6xT02NjjiEw9xLX8 Z8NawuDU+eCmx4FngQI50q4FboVe5yWrWYMFujM9BrhzzdXgyHKT10KTwONMbJMWhdR8 eb1ZWbvlDOiJ583IRtGINOq3NvFd1aeTj027/vw8u4/w/PJudjREkjY8uU2l8Ngksapy Dg2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686350425; x=1688942425; h=cc:to:from:subject:mime-version:message-id:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=VJ/oCWzA+yOfgNosg7yCARX3f3WD532lX89SwknwbFE=; b=O/pAwHqZ48WNWjF66AqmA5EardMjllVHXXLIjG5NGV0u1imfJ0ykGfxK78dIzPpqxm fVP9wF7U6UHrwd+RaiLjFG+byd3OhkdT2kblQlG6ttUFj7vktqvYjveJsZHs/6LfLoPK ah6i92vz4bcPLBEhMtVH16CvF/sDhFU1fio9FujYUux9S7pQ/d/z1w62xeWSTl0rZz8y oLh1PAY2KtVdnIOyASTJ1TRtiT7l0tHejY1d44GLrrbZIJcYHO7nUv9bEJ02WsBDWUJf DdfqKltT58Ai4A75mYSmGKUeIWCJU02OucVyv44NVxR40dyEF0lh+yZ1RLmuoZ3yQUAG oumA== X-Gm-Message-State: AC+VfDyusKw8Sc/PE4a73EoumR4i+R/Na1Z4O2FvURXd9Ay39RzMagGa ZnWmKohcALjssLY5kiiaeLQP30pBSFgJ X-Google-Smtp-Source: ACHHUZ4QD/zHlekmm6G++eWjoFjIqxdft4G6xFO/NIzF/aNyLSaHnGtFgWHuzpnDDHO4X5aO01Vp5iGLUT1G X-Received: from irogers.svl.corp.google.com ([2620:15c:2d4:203:55f2:1922:c44b:472d]) (user=irogers job=sendgmr) by 2002:a25:ab87:0:b0:bad:252:9f73 with SMTP id v7-20020a25ab87000000b00bad02529f73mr711064ybi.0.1686350425476; Fri, 09 Jun 2023 15:40:25 -0700 (PDT) Date: Fri, 9 Jun 2023 15:40:04 -0700 Message-Id: <20230609224004.180988-1-irogers@google.com> Mime-Version: 1.0 X-Mailer: git-send-email 2.41.0.162.gfafddb0af9-goog Subject: [PATCH v1] tools api fs: More thread safety for global filesystem variables From: Ian Rogers To: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, bpf@vger.kernel.org, linux-perf-users@vger.kernel.org, Jiri Olsa , Namhyung Kim , Adrian Hunter Cc: Ian Rogers Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" Multiple threads, such as with "perf top", may race to initialize a file system path like hugetlbfs. The racy initialization of the path leads to at least memory leaks. To avoid this initialize each fs for reading the mount point path with pthread_once. Mounting the file system may also be racy, so introduce a mutex over the function. This does mean that the path is being accessed with and without a mutex, which is inherently racy but hopefully benign, especially as there are fewer callers to fs__mount. Remove the fs__entries by directly using global variables, this was done as no argument like the index can be passed to the init once routine. Issue found and tested with "perf top" and address sanitizer. Signed-off-by: Ian Rogers --- tools/lib/api/fs/fs.c | 211 +++++++++++++++++------------------------- 1 file changed, 86 insertions(+), 125 deletions(-) diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c index 22d34a0be8b4..5cb0eeec2c8a 100644 --- a/tools/lib/api/fs/fs.c +++ b/tools/lib/api/fs/fs.c @@ -1,4 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 +#include #include #include #include @@ -10,6 +11,7 @@ #include #include #include +#include #include #include =20 @@ -43,7 +45,7 @@ #define BPF_FS_MAGIC 0xcafe4a11 #endif =20 -static const char * const sysfs__fs_known_mountpoints[] =3D { +static const char * const sysfs__known_mountpoints[] =3D { "/sys", 0, }; @@ -86,69 +88,70 @@ static const char * const bpf_fs__known_mountpoints[] = =3D { }; =20 struct fs { - const char *name; - const char * const *mounts; + const char * const name; + const char * const * const mounts; char *path; - bool found; - bool checked; - long magic; -}; - -enum { - FS__SYSFS =3D 0, - FS__PROCFS =3D 1, - FS__DEBUGFS =3D 2, - FS__TRACEFS =3D 3, - FS__HUGETLBFS =3D 4, - FS__BPF_FS =3D 5, + pthread_mutex_t mount_mutex; + const long magic; }; =20 #ifndef TRACEFS_MAGIC #define TRACEFS_MAGIC 0x74726163 #endif =20 -static struct fs fs__entries[] =3D { - [FS__SYSFS] =3D { - .name =3D "sysfs", - .mounts =3D sysfs__fs_known_mountpoints, - .magic =3D SYSFS_MAGIC, - .checked =3D false, - }, - [FS__PROCFS] =3D { - .name =3D "proc", - .mounts =3D procfs__known_mountpoints, - .magic =3D PROC_SUPER_MAGIC, - .checked =3D false, - }, - [FS__DEBUGFS] =3D { - .name =3D "debugfs", - .mounts =3D debugfs__known_mountpoints, - .magic =3D DEBUGFS_MAGIC, - .checked =3D false, - }, - [FS__TRACEFS] =3D { - .name =3D "tracefs", - .mounts =3D tracefs__known_mountpoints, - .magic =3D TRACEFS_MAGIC, - .checked =3D false, - }, - [FS__HUGETLBFS] =3D { - .name =3D "hugetlbfs", - .mounts =3D hugetlbfs__known_mountpoints, - .magic =3D HUGETLBFS_MAGIC, - .checked =3D false, - }, - [FS__BPF_FS] =3D { - .name =3D "bpf", - .mounts =3D bpf_fs__known_mountpoints, - .magic =3D BPF_FS_MAGIC, - .checked =3D false, - }, -}; +static void fs__init_once(struct fs *fs); +static const char *fs__mountpoint(const struct fs *fs); +static const char *fs__mount(struct fs *fs); + +#define FS(lower_name, fs_name, upper_name) \ +static struct fs fs__##lower_name =3D { \ + .name =3D #fs_name, \ + .mounts =3D lower_name##__known_mountpoints, \ + .magic =3D upper_name##_MAGIC, \ + .mount_mutex =3D PTHREAD_MUTEX_INITIALIZER, \ +}; \ + \ +static void lower_name##_init_once(void) \ +{ \ + struct fs *fs =3D &fs__##lower_name; \ + \ + fs__init_once(fs); \ +} \ + \ +const char *lower_name##__mountpoint(void) \ +{ \ + static pthread_once_t init_once =3D PTHREAD_ONCE_INIT; \ + struct fs *fs =3D &fs__##lower_name; \ + \ + pthread_once(&init_once, lower_name##_init_once); \ + return fs__mountpoint(fs); \ +} \ + \ +const char *lower_name##__mount(void) \ +{ \ + const char *mountpoint =3D lower_name##__mountpoint(); \ + struct fs *fs =3D &fs__##lower_name; \ + \ + if (mountpoint) \ + return mountpoint; \ + \ + return fs__mount(fs); \ +} \ + \ +bool lower_name##__configured(void) \ +{ \ + return lower_name##__mountpoint() !=3D NULL; \ +} + +FS(sysfs, sysfs, SYSFS); +FS(procfs, procfs, PROC_SUPER); +FS(debugfs, debugfs, DEBUGFS); +FS(tracefs, tracefs, TRACEFS); +FS(hugetlbfs, hugetlbfs, HUGETLBFS); +FS(bpf_fs, bpf, BPF_FS); =20 static bool fs__read_mounts(struct fs *fs) { - bool found =3D false; char type[100]; FILE *fp; char path[PATH_MAX + 1]; @@ -157,22 +160,17 @@ static bool fs__read_mounts(struct fs *fs) if (fp =3D=3D NULL) return false; =20 - while (!found && - fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n", + while (fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n", path, type) =3D=3D 2) { =20 if (strcmp(type, fs->name) =3D=3D 0) { - free(fs->path); fs->path =3D strdup(path); - if (!fs->path) - return false; - found =3D true; + fclose(fp); + return fs->path !=3D NULL; } } - fclose(fp); - fs->checked =3D true; - return fs->found =3D found; + return false; } =20 static int fs__valid_mount(const char *fs, long magic) @@ -194,11 +192,9 @@ static bool fs__check_mounts(struct fs *fs) ptr =3D fs->mounts; while (*ptr) { if (fs__valid_mount(*ptr, fs->magic) =3D=3D 0) { - free(fs->path); fs->path =3D strdup(*ptr); if (!fs->path) return false; - fs->found =3D true; return true; } ptr++; @@ -236,45 +232,26 @@ static bool fs__env_override(struct fs *fs) if (!override_path) return false; =20 - free(fs->path); fs->path =3D strdup(override_path); if (!fs->path) return false; - fs->found =3D true; - fs->checked =3D true; return true; } =20 -static const char *fs__get_mountpoint(struct fs *fs) +static void fs__init_once(struct fs *fs) { - if (fs__env_override(fs)) - return fs->path; - - if (fs__check_mounts(fs)) - return fs->path; - - if (fs__read_mounts(fs)) - return fs->path; - - return NULL; + if (!fs__env_override(fs) && + !fs__check_mounts(fs) && + !fs__read_mounts(fs)) { + assert(!fs->path); + } else { + assert(fs->path); + } } =20 -static const char *fs__mountpoint(int idx) +static const char *fs__mountpoint(const struct fs *fs) { - struct fs *fs =3D &fs__entries[idx]; - - if (fs->found) - 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. - * This makes the found and not found paths cost equivalent - * in case of multiple calls. - */ - if (fs->checked) - return NULL; - - return fs__get_mountpoint(fs); + return fs->path; } =20 static const char *mount_overload(struct fs *fs) @@ -289,45 +266,29 @@ static const char *mount_overload(struct fs *fs) return getenv(upper_name) ?: *fs->mounts; } =20 -static const char *fs__mount(int idx) +static const char *fs__mount(struct fs *fs) { - struct fs *fs =3D &fs__entries[idx]; const char *mountpoint; =20 - if (fs__mountpoint(idx)) - return (const char *)fs->path; + pthread_mutex_lock(&fs->mount_mutex); =20 - mountpoint =3D mount_overload(fs); + /* Check if path found inside the mutex to avoid races with other callers= of mount. */ + mountpoint =3D fs__mountpoint(fs); + if (mountpoint) + goto out; =20 - if (mount(NULL, mountpoint, fs->name, 0, NULL) < 0) - return NULL; - - return fs__check_mounts(fs) ? fs->path : NULL; -} + mountpoint =3D mount_overload(fs); =20 -#define FS(name, idx) \ -const char *name##__mountpoint(void) \ -{ \ - return fs__mountpoint(idx); \ -} \ - \ -const char *name##__mount(void) \ -{ \ - return fs__mount(idx); \ -} \ - \ -bool name##__configured(void) \ -{ \ - return name##__mountpoint() !=3D NULL; \ + if (mount(NULL, mountpoint, fs->name, 0, NULL) =3D=3D 0 && + fs__valid_mount(mountpoint, fs->magic) =3D=3D 0) { + fs->path =3D strdup(mountpoint); + mountpoint =3D fs->path; + } +out: + pthread_mutex_unlock(&fs->mount_mutex); + return mountpoint; } =20 -FS(sysfs, FS__SYSFS); -FS(procfs, FS__PROCFS); -FS(debugfs, FS__DEBUGFS); -FS(tracefs, FS__TRACEFS); -FS(hugetlbfs, FS__HUGETLBFS); -FS(bpf_fs, FS__BPF_FS); - int filename__read_int(const char *filename, int *value) { char line[64]; --=20 2.41.0.162.gfafddb0af9-goog