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

On 9/8/26 23:21, [email protected] wrote:
> 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.
> 

Will select the target prog according to 'first' value.

Thanks,
Leon

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