Re: [PATCH bpf-next 11/13] selftests/bpf: Add tracing_multi bpf prog rollback test

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `test_rollback_bpf_prog` test hardcodes executing `target_1` to verify rollback behavior, leading to spurious test failures if `target_2` happens to have a lower file descriptor.
--

--- Patch [11]: [PATCH bpf-next 11/13] selftests/bpf: Add tracing_multi bpf prog rollback test ---
commit b065372f3722241a5632ef72df356c018b778825
Author: Leon Hwang <[email protected]>

selftests/bpf: Add tracing_multi bpf prog rollback test

This commit adds a test to verify that prepared trampolines are correctly
reverted when a BPF program multi-attach fails and triggers a rollback.

> diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_multi.c b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> index 070266a864d4f..a4d9934ad1dc3 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tracing_multi.c
[ ... ]
> @@ -377,6 +377,65 @@ static void test_cookies_bpf_prog(void)
[ ... ]
> +static void test_rollback_bpf_prog(void)
> +{
> +	int token_fd, prog_fd, fd = -1, pre_fd = -1;
[ ... ]
> +	first = target_fds[0] < target_fds[1] ? 0 : 1;
[ ... ]
> +	fd = bpf_link_create(prog_fd, 0, BPF_TRACE_FENTRY_MULTI, &lopts);
> +	if (!ASSERT_EQ(fd, -EBUSY, "bpf_link_create"))
> +		goto cleanup;
> +
> +	if (run_bpf_target(skel->progs.target_1, first + 1, "rollback_target"))

[Severity: Medium]
Does this test accidentally hardcode the execution of target_1 while expecting
a dynamic return value based on file descriptor ordering?

If the file descriptors are assigned such that target_fds[1] < target_fds[0],
the variable first becomes 1. The test then executes target_1 but expects a
return value of first + 1 (which is 2).

Since target_1 always returns 1, will this cause a spurious test failure?

Additionally, if first == 1, target_1 would still have a trampoline attached
via pre_fd. This would cause test_result_fentry to increment and fail the
subsequent ASSERT_EQ() check.

> +		goto cleanup;
> +	ASSERT_EQ(skel->bss->test_result_fentry, 0, "test_result_fentry");
> +
> +cleanup:
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11