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

[bot+bpf-ci]

> 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@xxxxxxxxx>

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