Re: [PATCH bpf-next v2 2/2] selftests/bpf: Verify rejection of sleepable tracing prog

On 25/7/26 21:33, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The test contains dead code after the expected failure of `fentry_sleepable__load()`. If this code executes on an unfixed kernel, it intentionally triggers a kernel BUG and crashes the test environment.
> --
> 
> --- Patch [2]: [PATCH bpf-next v2 2/2] selftests/bpf: Verify rejection of sleepable tracing prog ---
> commit 44292a230f0467503b72b0ecb8c6c25244de310b
> Author: Leon Hwang <[email protected]>
> 
> selftests/bpf: Verify rejection of sleepable tracing prog
> 
> This commit adds a selftest to verify that a sleepable tracing program
> cannot attach to a '__x64_sys' prefix program target. The test attempts
> to load the program and expects the load to fail, avoiding a kernel BUG
> on unpatched kernels.
> 
>> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
>> --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> [ ... ]
>> @@ -576,6 +577,60 @@ static void test_func_replace_progmap(void)
> [ ... ]
>> +static void test_sleepable_fentry_to_xdp(void)
>> +{
> [ ... ]
>> +	err = fentry_sleepable__load(skel);
>> +	ASSERT_ERR(err, "fentry_sleepable__load");
>> +	if (err)
>> +		goto out;
>> +
>> +	skel->links.fentry_xdp = bpf_program__attach_trace(skel->progs.fentry_xdp);
>> +	if (!ASSERT_OK_PTR(skel->links.fentry_xdp, "bpf_program__attach_trace"))
>> +		goto out;
>> +
>> +	skel_xdp->links.__x64_sys_nop = bpf_program__attach_xdp(skel_xdp->progs.__x64_sys_nop,
>> +								ifindex);
>> +	if (!ASSERT_OK_PTR(skel_xdp->links.__x64_sys_nop, "bpf_program__attach_xdp"))
>> +		goto out;
>> +
>> +	err = system("ping -q -c 1 -W 1 127.0.0.1 > /dev/null");
>> +	ASSERT_OK(err, "ping");
>> +	ASSERT_ERR(skel->bss->retval, "retval");
>> +
>> +out:
>> +	fentry_sleepable__destroy(skel);
>> +	xdp_dummy__destroy(skel_xdp);
>> +}
> 
> [Severity: Medium]
> Since the test expects fentry_sleepable__load() to fail (meaning err < 0),
> won't the "if (err) goto out;" statement cause the test to immediately branch
> to out?

Correct.

When err, no need to create the links.

> 
> If this code behaves as expected, it appears all the subsequent logic for
> bpf_program__attach_trace(), bpf_program__attach_xdp(), and the ping command
> becomes unreachable dead code.
> 
> Conversely, if the test runs on an unpatched kernel where the load
> unexpectedly succeeds (err == 0), the test would fall through to the attach
> and ping logic. Could executing this sequence on an unpatched kernel trigger
> the exact kernel BUG mentioned in the commit message, potentially crashing
> the test environment instead of gracefully failing the test?
> 

Yes. But why do users run latest selftests on unpatched kernel? I guess
they are planning to patch the kernel.

Thanks,
Leon