Re: [PATCH bpf-next v2 20/23] selftests/bpf: Add precision backtracking test for stack arguments

On 5/9/26 3:52 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Commit message and code comment incorrectly claim that the subprogram performs a bounds check.
> - [Medium] Non-deterministic branching in the test program can lead to intermittent flaky failures by returning 36 instead of the asserted 25.
> --
>
> commit 953824567df7ff801d99eb6df49d76d95736325d
> Author: Yonghong Song <[email protected]>
>
> selftests/bpf: Add precision backtracking test for stack arguments
>
> This commit adds a test to verify precision backtracking across BPF-to-BPF
> calls involving stack arguments. The test aims to ensure precision propagates
> from a kfunc's size argument back through a subprogram frame.
>
>> The test passes a size value as incoming stack arg (arg6) to a
>> subprog, which bounds-checks it and forwards it as the mem__sz
>> parameter (outgoing arg7) to bpf_kfunc_call_stack_arg_mem.
> Is the description about bounds-checking accurate?

The description is not really reflecting the code due to code change
but commit message is not. Will fix.

>
> Looking at the inline assembly for subprog_call_mem_kfunc, it appears the
> subprogram simply reads the size from the incoming stack slot and immediately
> writes it to the outgoing stack slot without any bounds checking logic.
>
>> diff --git a/tools/testing/selftests/bpf/progs/stack_arg_precision.c b/tools/testing/selftests/bpf/progs/stack_arg_precision.c
>> new file mode 100644
>> index 0000000000000..c94905f07dcc1
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/stack_arg_precision.c
>> @@ -0,0 +1,137 @@
> [ ... ]
>
>> +/*
>> + * Test precision backtracking across bpf-to-bpf call for kfunc stack arg.
>> + * subprog_call_mem_kfunc receives a size as incoming stack arg (arg6),
>> + * bounds-checks it, then passes it as mem__sz (arg7) to
>> + * bpf_kfunc_call_stack_arg_mem.
>> + *
>> + * 1+2+3+4+5+(1+2+3+4) = 25
>> + */
> This isn't a bug, but should the comment be updated to match the
> implementation? As mentioned above, the bounds check seems to be missing from
> the assembly below.

You are right. Because there are two different values (4 or 6), there is
no guarantee that the sum of values will be 25.

>
>> +__naked __noinline __used
>> +static long subprog_call_mem_kfunc(long a, long b, long c, long d, long e, long size)
>> +{
>> +	asm volatile (
>> +		"r1 = *(u64 *)(r11 + 8);"	/* r1 = incoming arg6 (size) */
>> +		"r2 = 0x0807060504030201 ll;"	/* r2 = buf contents */
>> +		"*(u64 *)(r10 - 8) = r2;"	/* store buf to stack */
>> +		"r2 = r10;"
>> +		"r2 += -8;"			/* r2 = &buf */
>> +		"*(u64 *)(r11 - 8) = r2;"	/* outgoing arg6 = buf */
>> +		"*(u64 *)(r11 - 16) = r1;"	/* outgoing arg7 = size */
> [ ... ]
>
>> +__naked void stack_arg_precision_bpf2bpf(void)
>> +{
>> +	asm volatile (
>> +		"call %[bpf_get_prandom_u32];"
>> +		"r6 = r0;"
>> +		"r1 = 1;"
>> +		"r2 = 2;"
>> +		"r3 = 3;"
>> +		"r4 = 4;"
>> +		"r5 = 5;"
>> +		"if r6 < 2 goto l0_%=;"
>> +		"*(u64 *)(r11 - 8) = 4;"
>> +		"goto l1_%=;"
>> +	"l0_%=:"
>> +		"*(u64 *)(r11 - 8) = 6;"
>> +	"l1_%=:"
>> +		"call subprog_call_mem_kfunc;"
>> +		"exit;"
>> +		:: __imm(bpf_get_prandom_u32)
>> +		: __clobber_all
>> +	);
>> +}
> Could this lead to intermittent test failures?
>
> The test asserts a strict return value of 25:
>
> __success __retval(25)
>
> If bpf_get_prandom_u32() happens to return 0 or 1, the size argument is set to
> 6 instead of 4. In that case, bpf_kfunc_call_stack_arg_mem() will sum up 6
> bytes instead of 4, returning 36 instead of 25, which would cause the test to
> fail.

Yes, I will remove '__retval(25)'.