Re: [PATCH bpf-next v2 14/15] selftests/bpf: Test attaching struct_ops to a cgroup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Test state pollution in the `inherit` subtest bypasses the intended `cgroup_bpf_inherit()` testing path because the child cgroup persists from the previous `hierarchy` subtest.
--

commit 9dd80df6f86c72623dac7d2f203da4036e4e8827
Author: Martin KaFai Lau <martin.lau@kernel.org>
Subject: selftests/bpf: Test bpf_tcp_ops cgroup generic link

This commit adds selftests for attaching bpf_tcp_ops struct_ops to cgroups via
the generic cgroup link infrastructure. The tests validate various behaviors
including execution order, query interfaces, updates, and inheritance.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops.c
> new file mode 100644
> index 000000000000..4d087bdc4613
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops.c

[ ... ]

> +static void run_hierarchy_subtest(void)
> +{
> +	struct bpf_tcp_ops *skel = NULL;
> +	struct netns_obj *ns = NULL;
> +	int parent_fd, child_fd = -1;
> +
> +	parent_fd = test__join_cgroup(CGROUP_PATH);
> +	if (!ASSERT_GE(parent_fd, 0, "join_parent_cgroup"))
> +		return;
> +
> +	child_fd = create_and_get_cgroup(CGROUP_PATH "/child");
> +	if (!ASSERT_GE(child_fd, 0, "create_child_cgroup"))
> +		goto done;
> +
> +	if (!ASSERT_OK(join_cgroup(CGROUP_PATH "/child"), "join_child_cgroup"))
> +		goto done;
> +
> +	ns = netns_new(TEST_NETNS, true);
> +	if (!ASSERT_OK_PTR(ns, "netns_new"))
> +		goto done;
> +
> +	skel = bpf_tcp_ops__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		goto done;
> +
> +	test_hierarchy(parent_fd, child_fd, skel);
> +
> +done:
> +	bpf_tcp_ops__destroy(skel);
> +	netns_free(ns);
> +	if (child_fd >= 0)
> +		close(child_fd);
> +	close(parent_fd);
> +}

[Severity: Medium]
Does this code leak the child cgroup and pollute the state for the inherit
subtest?

Because the child cgroup is not explicitly removed here (e.g., via
remove_cgroup()), the directory persists in the filesystem after the file
descriptor is closed.

When test_inherit() runs immediately afterward, its call to
create_and_get_cgroup() will re-open this existing cgroup instead of creating a
new one. This causes test_inherit() to silently bypass the intended
cgroup_bpf_inherit() testing path, and instead exercise the
cgroup_bpf_update_effective() path during the parent attachment.

[ ... ]

> +static void test_inherit(int parent_fd, struct bpf_tcp_ops *skel)
> +{
> +	struct bpf_link *plink = NULL;
> +	int child_fd = -1;
> +
> +	plink = bpf_map__attach_cgroup_opts(skel->maps.tcp_ops1, parent_fd, NULL);
> +	if (!ASSERT_OK_PTR(plink, "attach_parent"))
> +		goto done;
> +
> +	child_fd = create_and_get_cgroup(CGROUP_PATH "/child");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623175006.3136053-1-ameryhung@gmail.com?part=14