* Re: [PATCH bpf-next 2/2] selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats
2026-10-08 14:02 ` [PATCH bpf-next 2/2] selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats Qiliang Yuan
@ 2026-10-08 14:11 ` Yosry Ahmed
2026-10-09 2:06 ` Qiliang Yuan
2026-10-08 14:13 ` sashiko-bot
2026-10-08 14:48 ` bot+bpf-ci
2 siblings, 1 reply; 8+ messages in thread
From: Yosry Ahmed @ 2026-10-08 14:11 UTC (permalink / raw)
To: Qiliang Yuan
Cc: Andrii Nakryiko, Eduard Zingerman, Ihor Solodrai,
Alexei Starovoitov, Daniel Borkmann, Kumar Kartikeya Dwivedi,
Martin KaFai Lau, Song Liu, Yonghong Song, Jiri Olsa,
Emil Tsalapatis, Shuah Khan, Hao Luo, bpf, linux-kselftest,
linux-kernel
On Thu, Oct 8, 2026 at 7:03 AM Qiliang Yuan <odys.yuan@gmail.com> wrote:
>
> setup_bpffs() mounts a bpffs on /sys/fs/bpf even when one is already
> there, while test_progs runs other tests that pin objects in /sys/fs/bpf
> in parallel.
>
> The new, empty bpffs hides the old one until the test ends, so those
> tests lose what they pinned or the directories they pin into.
> bpftool_maps_access then fails to pin an iterator because
> /sys/fs/bpf/test_bpftool_map doesn't exist anymore, and bpftool_metadata
> can't find the program it pinned in /sys/fs/bpf/test_metadata.
Seems like both of these tests do not mount bpffs, but rely on its
existence. If it's generally expected that bpffs is mounted when the
tests are run, should we drop the mounting code here instead?
Alternatively, should we have a common helper for creating directories
in bpffs that mounts it if it isn't already there?
>
> Mount a bpffs only if /sys/fs/bpf isn't one already.
>
> Fixes: 88886309d2e8 ("selftests/bpf: add a selftest for cgroup hierarchical stats collection")
> Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
> ---
> .../bpf/prog_tests/cgroup_hierarchical_stats.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> index a93b16ed629e9..8e0b67df700bf 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> @@ -23,8 +23,10 @@
> #include <asm-generic/errno.h>
> #include <errno.h>
> #include <sys/types.h>
> +#include <linux/magic.h>
> #include <sys/mount.h>
> #include <sys/stat.h>
> +#include <sys/vfs.h>
> #include <unistd.h>
>
> #include <test_progs.h>
> @@ -88,13 +90,22 @@ static int read_from_file(const char *path, char *buf, size_t size)
> /* mounts bpffs and mkdir for reading stats, returns 0 on success. */
> static int setup_bpffs(void)
> {
> + struct statfs st;
> int err;
>
> - /* Mount bpffs */
> - err = mount("bpf", BPFFS_ROOT, "bpf", 0, NULL);
> - mounted_bpffs = !err;
> - if (ASSERT_FALSE(err && errno != EBUSY, "mount"))
> + /*
> + * Mount bpffs unless it is already there. Mounting another one on top
> + * would hide what other tests running in parallel pin in it.
> + */
> + err = statfs(BPFFS_ROOT, &st);
> + if (!ASSERT_OK(err, "statfs " BPFFS_ROOT))
> return err;
> + if (st.f_type != BPF_FS_MAGIC) {
> + err = mount("bpf", BPFFS_ROOT, "bpf", 0, NULL);
> + if (!ASSERT_OK(err, "mount"))
> + return err;
> + mounted_bpffs = true;
> + }
>
> /* Create a directory to contain stat files in bpffs */
> err = mkdir(BPFFS_ATTACH_COUNTERS, 0755);
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH bpf-next 2/2] selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats
2026-10-08 14:11 ` Yosry Ahmed
@ 2026-10-09 2:06 ` Qiliang Yuan
0 siblings, 0 replies; 8+ messages in thread
From: Qiliang Yuan @ 2026-10-09 2:06 UTC (permalink / raw)
To: Yosry Ahmed
Cc: Andrii Nakryiko, Eduard Zingerman, Ihor Solodrai,
Alexei Starovoitov, Daniel Borkmann, Kumar Kartikeya Dwivedi,
Martin KaFai Lau, Song Liu, Yonghong Song, Jiri Olsa,
Emil Tsalapatis, Shuah Khan, Hao Luo, bpf, linux-kselftest,
linux-kernel
On Thu, Oct 8, 2026 at 7:11 AM Yosry Ahmed <yosry@kernel.org> wrote:
> Seems like both of these tests do not mount bpffs, but rely on its
> existence. If it's generally expected that bpffs is mounted when the
> tests are run, should we drop the mounting code here instead?
>
> Alternatively, should we have a common helper for creating directories
> in bpffs that mounts it if it isn't already there?
Thanks for the review. Looking further, the bpffs is mounted in the
wrong place: setup_bpffs() runs before setup_cgroups() moves the test
into a private mount namespace, so the bpffs lands in the namespace
shared with the other tests, and cleanup_bpffs() can't unmount it from
the private one. The BPF CI bot pointed that out as well.
v2 sets up the cgroups first, so that the bpffs is mounted in the
private mount namespace. The test keeps working where no bpffs is
mounted, and it no longer affects any other test, so neither dropping
the mount nor a common helper is needed for this. I kept your
Reviewed-by on the unmount fix, whose code didn't change.
v2: https://lore.kernel.org/r/20261009-selftests-bpf-cgroup-stats-bpffs-v2-0-f93a111da992@gmail.com
Thanks,
Qiliang
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats
2026-10-08 14:02 ` [PATCH bpf-next 2/2] selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats Qiliang Yuan
2026-10-08 14:11 ` Yosry Ahmed
@ 2026-10-08 14:13 ` sashiko-bot
2026-10-08 14:48 ` bot+bpf-ci
2 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-10-08 14:13 UTC (permalink / raw)
To: Qiliang Yuan; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests/bpf: stale mounted_bpffs flag causes unintended unmount of shared bpffs
- [Medium] selftests/bpf: uninitialized fds close stdin on setup error path
- [Low] bpf: selftests: spurious test failure due to unhandled EEXIST on mkdir
--
commit 73ba1f06cd40bfa82a4b5df33520980276eaa88f
Author: Qiliang Yuan <odys.yuan@gmail.com>
selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats
This commit changes the test to only mount a new BPF filesystem if one is
not already present at /sys/fs/bpf. It avoids hiding an existing mount,
which would cause parallel tests to lose access to their pinned objects.
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> index a93b16ed629e9..8e0b67df700bf 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
[ ... ]
> @@ -88,13 +90,22 @@ static int read_from_file(const char *path, char *buf, size_t size)
> /* mounts bpffs and mkdir for reading stats, returns 0 on success. */
> static int setup_bpffs(void)
> {
> + struct statfs st;
> int err;
>
> - /* Mount bpffs */
> - err = mount("bpf", BPFFS_ROOT, "bpf", 0, NULL);
> - mounted_bpffs = !err;
> - if (ASSERT_FALSE(err && errno != EBUSY, "mount"))
> + /*
> + * Mount bpffs unless it is already there. Mounting another one on top
> + * would hide what other tests running in parallel pin in it.
> + */
> + err = statfs(BPFFS_ROOT, &st);
> + if (!ASSERT_OK(err, "statfs " BPFFS_ROOT))
> return err;
[Severity: Medium]
If this statfs() check fails, does the subsequent error handling close stdin?
Looking at the error path, if setup_bpffs() returns an error here,
destroy_hierarchy() calls cleanup_cgroups(), which iterates over the
cgroups array and calls close(cgroups[i].fd) and close(root_cgroup_fd):
cleanup_cgroups() {
close(root_cgroup_fd);
for (int i = 0; i < N_CGROUPS; i++)
close(cgroups[i].fd);
...
}
Since these are static variables without explicit initialization, they
are implicitly initialized to 0. If setup_cgroups() hasn't been called
yet, wouldn't this call close(0) (stdin) multiple times?
In a parallel test runner environment, closing file descriptor 0 can lead
to subsequent opens unexpectedly getting file descriptor 0, which would then
be inadvertently closed by the remaining loop iterations, potentially
corrupting concurrent test operations.
> + if (st.f_type != BPF_FS_MAGIC) {
> + err = mount("bpf", BPFFS_ROOT, "bpf", 0, NULL);
> + if (!ASSERT_OK(err, "mount"))
> + return err;
> + mounted_bpffs = true;
> + }
[Severity: Medium]
Is it possible for the mounted_bpffs flag to remain true from a previous
test run and cause an unintended unmount?
The test runner executes tests sequentially in a single process, so BSS
variables like mounted_bpffs persist across iterations. On the first run,
if the BPFFS is not already mounted, setup_bpffs() mounts it and sets
mounted_bpffs = true.
If cleanup_bpffs() unmounts it but fails to reset mounted_bpffs to false,
on a subsequent run where BPFFS is already mounted, st.f_type ==
BPF_FS_MAGIC is true, and the block above is skipped.
Since mounted_bpffs is still true from the previous run, will
cleanup_bpffs() erroneously unmount the shared BPFFS that it did not mount,
destroying objects pinned by parallel tests?
>
> /* Create a directory to contain stat files in bpffs */
> err = mkdir(BPFFS_ATTACH_COUNTERS, 0755);
> if (!ASSERT_OK(err, "mkdir"))
> return err;
[Severity: Low]
Does this strict ASSERT_OK() check cause spurious test failures if the
directory already exists?
Since the test now reuses an existing BPFFS instead of shadowing it with
a new empty mount, if a previous test run crashes or is aborted, it leaves
behind the BPFFS_ATTACH_COUNTERS directory.
On the next run, mkdir() would return -1 with errno == EEXIST, causing
this check to fail and abort the test. Any pinned files left inside the
directory would also cause subsequent bpf_link__pin() calls to fail with
EEXIST.
Should this gracefully handle the EEXIST case and cleanup leftover state
when reusing a shared filesystem?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-selftests-bpf-cgroup-stats-bpffs-v1-0-76b9a6d1be34@gmail.com?part=2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH bpf-next 2/2] selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats
2026-10-08 14:02 ` [PATCH bpf-next 2/2] selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats Qiliang Yuan
2026-10-08 14:11 ` Yosry Ahmed
2026-10-08 14:13 ` sashiko-bot
@ 2026-10-08 14:48 ` bot+bpf-ci
2 siblings, 0 replies; 8+ messages in thread
From: bot+bpf-ci @ 2026-10-08 14:48 UTC (permalink / raw)
To: odys.yuan, andrii, eddyz87, ihor.solodrai, ast, daniel, memxor,
martin.lau, song, yonghong.song, jolsa, emil, shuah, yosry,
haoluo
Cc: bpf, linux-kselftest, linux-kernel, odys.yuan, ast, andrii,
daniel, martin.lau, eddyz87, yonghong.song, mason, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 4726 bytes --]
> selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats
TL;DR:
- Commit message may be inaccurate: bpffs mounted before the cgroup
mount-namespace unshare seems never unmounted afterwards
- Unanswered v1 question: drop the mounting code or add a common helper?
> selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats
>
> setup_bpffs() mounts a bpffs on /sys/fs/bpf even when one is already
> there, while test_progs runs other tests that pin objects in /sys/fs/bpf
> in parallel.
>
> The new, empty bpffs hides the old one until the test ends, so those
> tests lose what they pinned or the directories they pin into.
> bpftool_maps_access then fails to pin an iterator because
> /sys/fs/bpf/test_bpftool_map doesn't exist anymore, and bpftool_metadata
> can't find the program it pinned in /sys/fs/bpf/test_metadata.
>
> Mount a bpffs only if /sys/fs/bpf isn't one already.
>
> Fixes: 88886309d2e8 ("selftests/bpf: add a selftest for cgroup hierarchical stats collection")
> Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
The changelog says the new bpffs hides the old one "until the test ends".
Is that accurate?
setup_hierarchy() calls setup_bpffs() first and setup_cgroups() second:
static int setup_hierarchy(void)
{
return setup_bpffs() || setup_cgroups();
}
setup_cgroups() goes through setup_cgroup_environment(), which moves the
process into a private mount namespace:
tools/testing/selftests/bpf/cgroup_helpers.c:setup_cgroup_environment() {
...
if (unshare(CLONE_NEWNS)) {
...
if (mount("none", "/", NULL, MS_REC | MS_PRIVATE, NULL)) {
...
}
So a bpffs mounted by setup_bpffs() lands in the original mount namespace,
and the test then continues in a private copy and never switches back
(cgroup_helpers.c has no setns()).
cleanup_bpffs() runs after cleanup_cgroups() from destroy_hierarchy(), so
its umount(BPFFS_ROOT) only removes the private copy. The empty bpffs in
the original namespace seems to stay on top of the real one after the test
ends, and other processes in that namespace would keep losing their pins.
The same applies to the mounted_bpffs = true path that this patch keeps:
the umount in cleanup_bpffs() does not undo the mount done in
setup_bpffs().
The check for an existing bpffs avoids the stacking, but should the
changelog be reworded to describe this accurately? Alternatively, could the
bpffs setup be done after setup_cgroup_environment(), or the unmount be done
in the original namespace?
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> index a93b16ed629e..8e0b67df700b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
[ ... ]
> @@ -88,13 +90,22 @@ static int read_from_file(const char *path, char *buf, size_t size)
> /* mounts bpffs and mkdir for reading stats, returns 0 on success. */
> static int setup_bpffs(void)
> {
> + struct statfs st;
> int err;
>
> - /* Mount bpffs */
> - err = mount("bpf", BPFFS_ROOT, "bpf", 0, NULL);
> - mounted_bpffs = !err;
> - if (ASSERT_FALSE(err && errno != EBUSY, "mount"))
> + /*
> + * Mount bpffs unless it is already there. Mounting another one on top
> + * would hide what other tests running in parallel pin in it.
> + */
> + err = statfs(BPFFS_ROOT, &st);
> + if (!ASSERT_OK(err, "statfs " BPFFS_ROOT))
> return err;
> + if (st.f_type != BPF_FS_MAGIC) {
> + err = mount("bpf", BPFFS_ROOT, "bpf", 0, NULL);
> + if (!ASSERT_OK(err, "mount"))
> + return err;
> + mounted_bpffs = true;
> + }
Yosry Ahmed raised a design question on v1 that does not appear to have
been answered. The other tests that break (bpftool_maps_access and
bpftool_metadata) do not mount bpffs themselves, they rely on it already
existing:
> Seems like both of these tests do not mount bpffs, but rely on its
> existence. If it's generally expected that bpffs is mounted when the tests
> are run, should we drop the mounting code here instead? Alternatively,
> should we have a common helper for creating directories in bpffs that
> mounts it if it isn't already there?
Since this patch keeps the conditional mount in setup_bpffs(), should the
mounting code be dropped here, or should a common helper that creates
directories in bpffs and mounts it only when missing be introduced?
[ ... ]
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/37792241939
^ permalink raw reply [flat|nested] 8+ messages in thread