Re: [PATCH bpf-next 2/2] selftests/bpf: Don't stack a bpffs in cgroup_hierarchical_stats

[sashiko-bot@kernel.org]

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