Re: [PATCH bpf-next v3 2/2] selftests/bpf: Add tests with stack ptr register in conditional jmp

[Yonghong Song <yonghong.song@linux.dev>]

On 5/21/25 12:02 PM, Eduard Zingerman wrote:
>
> On 2025-05-21 10:04, Yonghong Song wrote:
>
> [...]
>
>> @@ -178,4 +178,57 @@ __naked int state_loop_first_last_equal(void)
>>       );
>>   }
>>   +__used __naked static void __bpf_cond_op_r10(void)
>> +{
>> +    asm volatile (
>> +    "r2 = 2314885393468386424 ll;"
>> +    "goto +0;"
>> +    "if r2 <= r10 goto +3;"
>> +    "if r1 >= -1835016 goto +0;"
>> +    "if r2 <= 8 goto +0;"
>> +    "if r3 <= 0 goto +0;"
>> +    "exit;"
>> +    ::: __clobber_all);
>> +}
>> +
>> +SEC("?raw_tp")
>> +__success __log_level(2)
>> +__msg("8: (bd) if r2 <= r10 goto pc+3")
>> +__msg("9: (35) if r1 >= 0xffe3fff8 goto pc+0")
>> +__msg("10: (b5) if r2 <= 0x8 goto pc+0")
>> +__msg("mark_precise: frame1: last_idx 10 first_idx 0 subseq_idx -1")
>> +__msg("mark_precise: frame1: regs=r2 stack= before 9: (35) if r1 >= 
>> 0xffe3fff8 goto pc+0")
>> +__msg("mark_precise: frame1: regs=r2 stack= before 8: (bd) if r2 <= 
>> r10 goto pc+3")
>> +__msg("mark_precise: frame1: regs=r2 stack= before 7: (05) goto pc+0")
>> +__naked void bpf_cond_op_r10(void)
>> +{
>> +    asm volatile (
>> +    "r3 = 0 ll;"
>> +    "call __bpf_cond_op_r10;"
>> +    "r0 = 0;"
>> +    "exit;"
>> +    ::: __clobber_all);
>> +}
>
> This was probably a part of the repro, but I'm not sure
> this test adds much compared to test below.
> The changes do not interact with subprogram calls handling.

It does not interact with subprogram due the patch 1. Without patch 1,
the error will happen (see commit message of patch 1):

   0: (18) r3 = 0x0                      ; R3_w=0
   2: (85) call pc+2
   caller:
    R10=fp0
   callee:
    frame1: R1=ctx() R3_w=0 R10=fp0
   5: frame1: R1=ctx() R3_w=0 R10=fp0
   ; asm volatile ("                                 \ @ verifier_precision.c:184
   5: (18) r2 = 0x20202000256c6c78       ; frame1: R2_w=0x20202000256c6c78
   7: (05) goto pc+0
   8: (bd) if r2 <= r10 goto pc+3        ; frame1: R2_w=0x20202000256c6c78 R10=fp0
   9: (35) if r1 >= 0xffe3fff8 goto pc+0         ; frame1: R1=ctx()
   10: (b5) if r2 <= 0x8 goto pc+0
   mark_precise: frame1: last_idx 10 first_idx 0 subseq_idx -1
   mark_precise: frame1: regs=r2 stack= before 9: (35) if r1 >= 0xffe3fff8 goto pc+0
   mark_precise: frame1: regs=r2 stack= before 8: (bd) if r2 <= r10 goto pc+3
   mark_precise: frame1: regs=r2,r10 stack= before 7: (05) goto pc+0
   mark_precise: frame1: regs=r2,r10 stack= before 5: (18) r2 = 0x20202000256c6c78
   mark_precise: frame1: regs=r10 stack= before 2: (85) call pc+2
   BUG regs 400

>
>> +
>> +SEC("?raw_tp")
>> +__success __log_level(2)
>> +__msg("3: (bf) r3 = r10")
>> +__msg("4: (bd) if r3 <= r2 goto pc+1")
>> +__msg("5: (b5) if r2 <= 0x8 goto pc+2")
>> +__msg("mark_precise: frame0: last_idx 5 first_idx 0 subseq_idx -1")
>> +__msg("mark_precise: frame0: regs=r2 stack= before 4: (bd) if r3 <= 
>> r2 goto pc+1")
>> +__msg("mark_precise: frame0: regs=r2 stack= before 3: (bf) r3 = r10")
>> +__naked void bpf_cond_op_not_r10(void)
>> +{
>> +    asm volatile (
>> +    "r0 = 0;"
>> +    "r2 = 2314885393468386424 ll;"
>> +    "r3 = r10;"
>> +    "if r3 <= r2 goto +1;"
>> +    "if r2 <= 8 goto +2;"
>
> I think it would be good to add two more cases here:
> - dst register is pointer to stack

The previous test "r2 <= r10" should already cover this since r10 is a pointer to stack.

> - both src and dst registers are pointers to stack

I actually thought about this as well, e.g.,
    r2 = r10
    r3 = r10
    if r2 <= r3 goto +0
    if r2 <= 8 goto +0

But since r2 is actually a stack pointer, then r2 does not need
backtracking. So r2 <= r3 won't be backtracked too.

But if you feel such an example still valuable, I can add it too.

>
>> +    "r0 = 2 ll;"
>> +    "exit;"
>> +    ::: __clobber_all);
>> +}
>> +
>>   char _license[] SEC("license") = "GPL";