Re: [RFC PATCH bpf-next v7 09/11] selftests/bpf: Add test for memcg_bpf_ops hierarchies

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test reuses the identical `low_mcg_ops` map for both the parent and child overrides, failing to properly test the override mechanism and clobbering its own state during cleanup.
- [Low] The test verifies that the third attachment fails but does not check if it fails with the correct error code (-EBUSY) as claimed in the commit message.
--

commit ac1e7b9da1794e345399c9485b390263c35e79df
Author: Hui Zhu <zhuhui@kylinos.cn>
selftests/bpf: Add test for memcg_bpf_ops hierarchies

This commit adds a new selftest, test_memcg_ops_hierarchies, to validate
the behavior of attaching memcg_bpf_ops in a nested cgroup hierarchy. It
verifies the enforcement of the BPF_F_ALLOW_OVERRIDE flag across a
three-level deep cgroup subtree.

> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_ops.c b/tools/testing/selftests/bpf/prog_tests/memcg_ops.c
> index 19fd4fde22660..b4084e9327eba 100644
> --- a/tools/testing/selftests/bpf/prog_tests/memcg_ops.c
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_ops.c
> @@ -559,3 +559,76 @@ void test_memcg_ops_below_min_over_high(void)

[ ... ]

> +	skel = memcg_ops__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "memcg_ops__open_and_load"))
> +		goto cleanup;
> +
> +	map = bpf_object__find_map_by_name(skel->obj, "low_mcg_ops");
> +	if (!ASSERT_OK_PTR(map, "bpf_object__find_map_by_name low_mcg_ops"))
> +		goto cleanup;
> +
> +	opts.target_fd = first;
> +	opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_CGROUP_FD;
> +	link1 = bpf_map__attach_struct_ops_opts(map, &opts);
> +	if (!ASSERT_OK_PTR(link1, "bpf_map__attach_struct_ops_opts"))
> +		goto cleanup;
> +
> +	opts.target_fd = second;
> +	opts.flags = BPF_F_CGROUP_FD;
> +	link2 = bpf_map__attach_struct_ops_opts(map, &opts);

[Severity: Medium]
Is it intentional to reuse the identical low_mcg_ops map for both the
parent and child overrides?

Because the overriding pointer is physically identical to the overridden one,
this might not genuinely prove that the kernel substituted the operations.

Also, could this clobber the state during cleanup since the kernel's
bpf_memcg_ops_unreg() clears descendant cgroups by checking map pointer
equality?

Would it be better to retrieve a distinct map, such as high_mcg_ops, for
the child cgroup to correctly test overriding?

> +	if (!ASSERT_OK_PTR(link2, "bpf_map__attach_struct_ops_opts"))
> +		goto cleanup;
> +
> +	opts.target_fd = third;
> +	opts.flags = BPF_F_CGROUP_FD;
> +	link3 = bpf_map__attach_struct_ops_opts(map, &opts);
> +	if (!ASSERT_ERR_PTR(link3, "bpf_map__attach_struct_ops_opts"))

[Severity: Low]
The commit message states that attaching another struct_ops to the deepest
cgroup fails with -EBUSY. Does ASSERT_ERR_PTR() only check for a generic
failure pointer?

Should this test explicitly verify that errno == EBUSY to ensure the
hierarchical restriction logic is failing for the exact reason expected?

> +		goto cleanup;
> +
> +cleanup:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779760876.git.zhuhui@kylinos.cn?part=9