[PATCH bpf-next v2 0/2] selftests/bpf: Fix the bpffs handling of cgroup_hierarchical_stats

[PATCH bpf-next v2 0/2] selftests/bpf: Fix the bpffs handling of cgroup_hierarchical_stats

[Qiliang Yuan]

cgroup_hierarchical_stats mounts a bpffs on /sys/fs/bpf to pin its
iterators, then moves into a private mount namespace to set up its
cgroups, and unmounts the bpffs in the end if it mounted it itself.

The bpffs lands in the mount namespace shared with the tests running
in parallel, on top of the one already there, and hides what they pin,
which makes bpftool_maps_access and bpftool_metadata fail now and then.
The unmount can't remove it from there, and it checks the other way
around whether the test mounted a bpffs anyway.

Patch 1 mounts the bpffs in the private mount namespace, patch 2 makes
the cleanup unmount the bpffs that the test mounted.

Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
V1 -> V2:
- Mount the bpffs in the private mount namespace instead of skipping the
  mount when /sys/fs/bpf is a bpffs already (Yosry, CI bot)
- Put that patch first, so that the unmount fix applies to a bpffs it can
  actually unmount
- Pick up Reviewed-by from Yosry on the unmount fix

v1: https://lore.kernel.org/r/20261008-selftests-bpf-cgroup-stats-bpffs-v1-0-76b9a6d1be34@gmail.com

---
Qiliang Yuan (2):
      selftests/bpf: Keep the bpffs of cgroup_hierarchical_stats private
      selftests/bpf: Unmount the bpffs that cgroup_hierarchical_stats mounted

 tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
---
base-commit: e1d84a37cba984388988d2f1ddc84561413f0db2
change-id: 20261008-selftests-bpf-cgroup-stats-bpffs-4534db24d15c

Best regards,
-- 
Qiliang Yuan <odys.yuan@gmail.com>

[PATCH bpf-next v2 1/2] selftests/bpf: Keep the bpffs of cgroup_hierarchical_stats private

[Qiliang Yuan]

setup_hierarchy() mounts a bpffs on /sys/fs/bpf with setup_bpffs(), and
then sets up the cgroups with setup_cgroups(), which moves the test into
a private mount namespace.

The bpffs thus lands in the mount namespace that test_progs shares with
the tests running in parallel, on top of the bpffs already there, and
hides what they pin. cleanup_bpffs() later runs in the private mount
namespace and can't unmount it, so it stays there for good.
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.

Set up the cgroups first, so that the bpffs is mounted in the private
mount namespace.

Fixes: 88886309d2e8 ("selftests/bpf: add a selftest for cgroup hierarchical stats collection")
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 3bd27d2ea668c..ad19d6d41918a 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
@@ -151,7 +151,7 @@ static void cleanup_cgroups(void)
 /* Sets up cgroup hiearchary, returns 0 on success. */
 static int setup_hierarchy(void)
 {
-	return setup_bpffs() || setup_cgroups();
+	return setup_cgroups() || setup_bpffs();
 }
 
 static void destroy_hierarchy(void)

-- 
2.43.0

Re: [PATCH bpf-next v2 1/2] selftests/bpf: Keep the bpffs of cgroup_hierarchical_stats private

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: unbalanced lifecycle in destroy_hierarchy() teardown
--

commit be18155cbccd124da201337ba01f9571021f8d5c
Author: Qiliang Yuan <odys.yuan@gmail.com>

selftests/bpf: Keep the bpffs of cgroup_hierarchical_stats private

setup_hierarchy() mounts a bpffs on /sys/fs/bpf with setup_bpffs(), and
then sets up the cgroups with setup_cgroups(), which moves the test into
a private mount namespace. The patch sets up the cgroups first, so that
the bpffs is mounted in the private mount 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 3bd27d2ea668c..ad19d6d41918a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> @@ -151,7 +151,7 @@ static void cleanup_cgroups(void)
>  /* Sets up cgroup hiearchary, returns 0 on success. */
>  static int setup_hierarchy(void)
>  {
> -	return setup_bpffs() || setup_cgroups();
> +	return setup_cgroups() || setup_bpffs();
>  }
>  
>  static void destroy_hierarchy(void)

[Severity: Low]
Does destroy_hierarchy() need to be updated to reverse the teardown order?

Because the setup order has been changed to initialize cgroups before bpffs,
the test is placed in a private mount namespace before mounting the BPF
filesystem. 

However, destroy_hierarchy() continues to execute teardown in FIFO order
(cleanup_cgroups() followed by cleanup_bpffs()) instead of LIFO order.

While cleanup_cgroups() does not fully destroy the namespace until process
exit (preventing immediate crashes during cleanup_bpffs()), should
cleanup_bpffs() be called before cleanup_cgroups() to properly mirror the
new initialization order?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-selftests-bpf-cgroup-stats-bpffs-v2-0-f93a111da992@gmail.com?part=1

[PATCH bpf-next v2 2/2] selftests/bpf: Unmount the bpffs that cgroup_hierarchical_stats mounted

[Qiliang Yuan]

setup_bpffs() mounts a bpffs on /sys/fs/bpf and records in mounted_bpffs
whether it did, so that cleanup_bpffs() unmounts only a bpffs that the
test mounted itself.

cleanup_bpffs() checks the flag the other way around. As mounting on
/sys/fs/bpf succeeds even when a bpffs is already there, the test never
unmounts the bpffs it mounted, and every run leaves another one stacked
on /sys/fs/bpf of the mount namespace that the test_progs worker keeps
running later tests in.

Unmount the bpffs only when setup_bpffs() mounted it.

Fixes: 88886309d2e8 ("selftests/bpf: add a selftest for cgroup hierarchical stats collection")
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
Reviewed-by: Yosry Ahmed <yosry@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 ad19d6d41918a..cc6ab2c263da1 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
@@ -110,7 +110,7 @@ static void cleanup_bpffs(void)
 	ASSERT_OK(rmdir(BPFFS_ATTACH_COUNTERS), "rmdir "BPFFS_ATTACH_COUNTERS);
 
 	/* Unmount bpffs, if it wasn't already mounted when we started */
-	if (mounted_bpffs)
+	if (!mounted_bpffs)
 		return;
 
 	ASSERT_OK(umount(BPFFS_ROOT), "unmount bpffs");

-- 
2.43.0

Re: [PATCH bpf-next v2 2/2] selftests/bpf: Unmount the bpffs that cgroup_hierarchical_stats mounted

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: unconditional rmdir in cleanup_bpffs triggers spurious assertion failures
--

commit 34f23b53e236bfae8b2ad74d5d89f8806c601dbb
Author: Qiliang Yuan <odys.yuan@gmail.com>

selftests/bpf: Unmount the bpffs that cgroup_hierarchical_stats mounted

This commit fixes a logic error in cleanup_bpffs() where a locally mounted
bpffs was not correctly unmounted. By inverting the check on mounted_bpffs,
the test ensures it cleans up its own mounts rather than leaking them.

> 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 ad19d6d41918a..cc6ab2c263da1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c
> @@ -110,7 +110,7 @@ static void cleanup_bpffs(void)
>  	ASSERT_OK(rmdir(BPFFS_ATTACH_COUNTERS), "rmdir "BPFFS_ATTACH_COUNTERS);

[Severity: Low]
In cleanup_bpffs() within
tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c, does this
unconditional rmdir() trigger spurious assertion failures during early setup
error paths?

When setup fails (e.g., in setup_cgroups() or the initial mount() call), the
BPFFS_ATTACH_COUNTERS directory is never created. During the teardown path
(hierarchy_cleanup() -> destroy_hierarchy() -> cleanup_bpffs()), rmdir() is
unconditionally called and wrapped in ASSERT_OK().

If the directory does not exist, rmdir() returns -ENOENT, triggering a
secondary assertion failure. This clutters the test output and masks the
original setup failure.

>  
>  	/* Unmount bpffs, if it wasn't already mounted when we started */
> -	if (mounted_bpffs)
> +	if (!mounted_bpffs)
>  		return;
>  
>  	ASSERT_OK(umount(BPFFS_ROOT), "unmount bpffs");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-selftests-bpf-cgroup-stats-bpffs-v2-0-f93a111da992@gmail.com?part=2