Re: [PATCH bpf-next v3 3/3] selftests/bpf: add test cases with CONST_PTR_TO_MAP null checks

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 6/5/25 5:25 PM, Alexei Starovoitov wrote:
> On Thu, Jun 5, 2025 at 4:40 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>>
>> On 6/5/25 10:00 AM, Alexei Starovoitov wrote:
>>> On Thu, Jun 5, 2025 at 9:42 AM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>>>>
>>>> On 6/5/25 9:24 AM, Andrii Nakryiko wrote:
>>>>> On Wed, Jun 4, 2025 at 3:28 PM Ihor Solodrai <isolodrai@meta.com> wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>> +SEC("socket")
>>>>>> +int map_ptr_is_never_null(void *ctx)
>>>>>> +{
>>>>>> +       struct bpf_map *maps[2] = { 0 };
>>>>>> +       struct bpf_map *map = NULL;
>>>>>> +       int __attribute__((aligned(8))) key = 0;
>>>>>
>>>>> aligned(8) makes any difference?
>>>>
>>>> Yes. If not aligned (explicitly or by accident), verification fails
>>>> with "math between fp pointer and register with unbounded min value is
>>>> not allowed" at maps[key]. See the log below.
>>>
>>> It's not clear to me why "aligned" changes code gen,
>>> but let's not rely on it.
>>> Try 'unsigned int key' instead.
>>> Also note that &key part pessimizes the code.
>>> Do for (...; i < 2; i++) {u32 key = i; &key }
>>> instead.
>>
>> I've tried changing the test like this:
>>
>> @@ -144,12 +144,12 @@ int map_ptr_is_never_null(void *ctx)
>>    {
>>           struct bpf_map *maps[2] = { 0 };
>>           struct bpf_map *map = NULL;
>> -       int __attribute__((aligned(8))) key = 0;
>>
>> -       for (key = 0; key < 2; key++) {
>> +       for (int i = 0; i < 2; i++) {
>> +               __u32 key = i;
>>                   map = bpf_map_lookup_elem(&map_in_map, &key);
>>                   if (map)
>> -                       maps[key] = map;
>> +                       maps[i] = map;
>>                   else
>>                           return 0;
>>           }
>>
>> This version passes verification independent of the first patch being
>> applied, which kinda defeats the purpose of the test. Verifier log
>> below:
>>
>>       0: R1=ctx() R10=fp0
>>       ; int map_ptr_is_never_null(void *ctx) @ verifier_map_in_map.c:143
>>       0: (b4) w1 = 0                        ; R1_w=0
>>       ; __u32 key = i; @ verifier_map_in_map.c:149
>>       1: (63) *(u32 *)(r10 -4) = r1
>>       mark_precise: frame0: last_idx 1 first_idx 0 subseq_idx -1
>>       mark_precise: frame0: regs=r1 stack= before 0: (b4) w1 = 0
>>       2: R1_w=0 R10=fp0 fp-8=0000????
>>       2: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
>>       3: (07) r2 += -4                      ; R2_w=fp-4
>>       ; map = bpf_map_lookup_elem(&map_in_map, &key); @
>> verifier_map_in_map.c:150
>>       4: (18) r1 = 0xff302cd6802e0a00       ;
>> R1_w=map_ptr(map=map_in_map,ks=4,vs=4)
>>       6: (85) call bpf_map_lookup_elem#1    ;
>> R0_w=map_value_or_null(id=1,map=map_in_map,ks=4,vs=4)
>>       ; if (map) @ verifier_map_in_map.c:151
>>       7: (15) if r0 == 0x0 goto pc+7        ; R0_w=map_ptr(ks=4,vs=4)
>>       8: (b4) w1 = 1                        ; R1_w=1
>>       ; __u32 key = i; @ verifier_map_in_map.c:149
>>       9: (63) *(u32 *)(r10 -4) = r1         ; R1_w=1 R10=fp0 fp-8=mmmm????
>>       10: (bf) r2 = r10                     ; R2_w=fp0 R10=fp0
>>       11: (07) r2 += -4                     ; R2_w=fp-4
>>       ; map = bpf_map_lookup_elem(&map_in_map, &key); @
>> verifier_map_in_map.c:150
>>       12: (18) r1 = 0xff302cd6802e0a00      ;
>> R1_w=map_ptr(map=map_in_map,ks=4,vs=4)
>>       14: (85) call bpf_map_lookup_elem#1   ;
>> R0=map_value_or_null(id=2,map=map_in_map,ks=4,vs=4)
>>       ; } @ verifier_map_in_map.c:164
>>       15: (b4) w0 = 0                       ; R0_w=0
>>       16: (95) exit
> 
> because the compiler removed 'if (!maps[0])' check?
> Make maps volatile then.

Making maps and/or map volatile didn't help.

> 
> It's not clear to me what the point of the 'for' loop is.
> Just one bpf_map_lookup_elem() from map_in_map should do ?

No. Using an array and loop was my attempt to put map_ptr on the
stack. But apparently this was not the reason it happened in the v1 of
the test.

> 
>>
>> If map[i] is changed back to map[key] like this:
>>
>>          for (int i = 0; i < 2; i++) {
>>                  __u32 key = i;
>>                  map = bpf_map_lookup_elem(&map_in_map, &key);
>>                  if (map)
>>                          maps[key] = map; /* change here */
>>                  else
>>                          return 0;
>>          }
>>
>> Verification fails with "invalid unbounded variable-offset write to
>> stack R2"
> 
> as it should, because both the compiler and the verifier
> see that 'key' is unbounded in maps[key] access.
> 
>>       __u32 __attribute__((aligned(8))) key = i;
>>
>> but that puts us back to square one.
>>
>> It appears that alignment becomes a problem if the variable is used as
>> array index and also it's address is passed to a helper.
> 
> I bet this alignment "workaround" is fragile.
> A different version of clang or gcc-bpf will change layout.

I agree, it's fragile.

After I fought compiler/verifier for a while I gave up and wrote a
test in asm:

     SEC("socket")
     __description("map_ptr is never null")
     __success
     __naked void map_ptr_is_never_null(void)
     {
     	asm volatile ("					\
     	r1 = 0;						\
     	*(u32*)(r10 - 4) = r1;				\
     	r2 = r10;					\
     	r2 += -4;					\
     	r1 = %[map_in_map] ll;				\
     	call %[bpf_map_lookup_elem];			\
     	if r0 != 0 goto l0_%=;				\
     	exit;						\
     l0_%=:	*(u64 *)(r10 -16) = r0;				\
     	r1 = *(u64 *)(r10 -16);				\
     	if r1 == 0 goto l1_%=;				\
     	exit;						\
     l1_%=:	r10 = 42;					\
     	exit;						\
     "	:
     	: __imm(bpf_map_lookup_elem),
     	  __imm_addr(map_in_map)
     	: __clobber_all);
     }

What must happen to reproduce the situation is: map_ptr gets on a
stack, and then loaded and compared to 0.

It looks like I accidentally forced map_ptr on the stack by using
`key` both for map lookup and array access, which triggers those
alignment problems. Without that I wasn't able to figure out simple C
code that would produce bpf with map_ptr on the stack (besides the
other test, with rbs).

I guess I should've written an asm test right away.