Re: [PATCH bpf v1 2/2] selftests/bpf: Test cgroup link replace with BPF_F_PREORDER

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing error code propagation on `bpf_link_create` failures causes the test function to incorrectly return success.
--

commit 5273c7a6df31aa862a2601f91f12e4d696020f49
Author: Amery Hung <ameryhung@gmail.com>

selftests/bpf: Test cgroup link replace with BPF_F_PREORDER

This commit adds a cgroup_preorder test case that attaches both a normal
and a BPF_F_PREORDER program to a cgroup. It then replaces the normal
link's program via bpf_link_update() to verify the effective order changes
correctly without array corruption.

> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_preorder.c b/tools/testing/selftests/bpf/prog_tests/cgroup_preorder.c
> index d4d583872fa26..80a17203c4fd5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_preorder.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_preorder.c

[ ... ]

> @@ -102,6 +102,75 @@ static int run_getsockopt_test(int cg_parent, int cg_child, int sock_fd, bool al
> +static int run_link_replace_test(int cgroup_fd, int sock_fd)
> +{
> +	LIBBPF_OPTS(bpf_link_create_opts, create_opts);
> +	struct cgroup_preorder *skel = NULL;
> +	int normal_link = -1, preorder_link = -1;
> +	int atype, err = 0;
> +	__u8 *result, buf = 0x00;
> +	socklen_t optlen = 1;
> +
> +	skel = cgroup_preorder__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "cgroup_preorder__open_and_load"))
> +		return -1;
> +
> +	err = setsockopt(sock_fd, SOL_IP, IP_TOS, &buf, 1);
> +	if (!ASSERT_OK(err, "setsockopt"))
> +		goto close_skel;
> +
> +	atype = bpf_program__expected_attach_type(skel->progs.child);
> +
> +	create_opts.flags = 0;
> +	normal_link = bpf_link_create(bpf_program__fd(skel->progs.child),
> +				      cgroup_fd, atype, &create_opts);
> +	if (!ASSERT_GE(normal_link, 0, "create_normal_link"))
> +		goto close_skel;

[Severity: Medium]
Will run_link_replace_test() incorrectly return success if this first 
bpf_link_create() fails?

The err variable is 0 here from the previously successful setsockopt() call.
If normal_link fails, we jump to close_skel and return err, causing the 
caller's ASSERT_OK() check to see a successful test run despite the failure.

> +
> +	create_opts.flags = BPF_F_PREORDER;
> +	preorder_link = bpf_link_create(bpf_program__fd(skel->progs.child_2),
> +					cgroup_fd, atype, &create_opts);
> +	if (!ASSERT_GE(preorder_link, 0, "create_preorder_link"))
> +		goto close_links;

[Severity: Medium]
Similarly, should err be set to a negative value before jumping to
close_links if this second bpf_link_create() fails?

> +
> +	result = skel->bss->result;

[ ... ]

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