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 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


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"

     VERIFIER LOG:
     =============
     arg#0 reference type('UNKNOWN ') size cannot be determined: -22
     0: R1=ctx() R10=fp0
     ; int map_ptr_is_never_null(void *ctx) @ verifier_map_in_map.c:143
     0: (b7) r1 = 0                        ; R1_w=0
     ; struct bpf_map *maps[2] = { 0 }; @ verifier_map_in_map.c:145
     1: (7b) *(u64 *)(r10 -8) = r1         ; R1_w=0 R10=fp0 fp-8_w=0
     2: (7b) *(u64 *)(r10 -16) = r1        ; R1_w=0 R10=fp0 fp-16_w=0
     3: (b4) w1 = 0                        ; R1_w=0
     ; __u32 key = i; @ verifier_map_in_map.c:149
     4: (63) *(u32 *)(r10 -20) = r1        ; R1_w=0 R10=fp0 fp-24=0000????
     5: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
     6: (07) r2 += -20                     ; R2_w=fp-20
     ; map = bpf_map_lookup_elem(&map_in_map, &key); @ 
verifier_map_in_map.c:150
     7: (18) r1 = 0xff1f1a9b829ae400       ; 
R1_w=map_ptr(map=map_in_map,ks=4,vs=4)
     9: (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
     10: (15) if r0 == 0x0 goto pc+24      ; R0_w=map_ptr(ks=4,vs=4)
     ; maps[key] = map; @ verifier_map_in_map.c:152
     11: (61) r1 = *(u32 *)(r10 -20)       ; R1_w=0 R10=fp0 fp-24=0000????
     12: (67) r1 <<= 3                     ; R1_w=0
     13: (bf) r2 = r10                     ; R2_w=fp0 R10=fp0
     14: (07) r2 += -16                    ; R2_w=fp-16
     15: (0f) r2 += r1                     ; R1_w=0 R2_w=fp-16
     16: (7b) *(u64 *)(r2 +0) = r0         ; R0_w=map_ptr(ks=4,vs=4) 
R2_w=fp-16 fp-16_w=map_ptr(ks=4,vs=4)
     17: (b4) w1 = 1                       ; R1_w=1
     ; __u32 key = i; @ verifier_map_in_map.c:149
     18: (63) *(u32 *)(r10 -20) = r1       ; R1_w=1 R10=fp0 fp-24=mmmm????
     19: (bf) r2 = r10                     ; R2_w=fp0 R10=fp0
     20: (07) r2 += -20                    ; R2_w=fp-20
     ; map = bpf_map_lookup_elem(&map_in_map, &key); @ 
verifier_map_in_map.c:150
     21: (18) r1 = 0xff1f1a9b829ae400      ; 
R1_w=map_ptr(map=map_in_map,ks=4,vs=4)
     23: (85) call bpf_map_lookup_elem#1   ; 
R0=map_value_or_null(id=2,map=map_in_map,ks=4,vs=4)
     ; if (map) @ verifier_map_in_map.c:151
     24: (15) if r0 == 0x0 goto pc+10      ; R0=map_ptr(ks=4,vs=4)
     ; maps[key] = map; @ verifier_map_in_map.c:152
     25: (61) r1 = *(u32 *)(r10 -20)       ; 
R1_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) 
R10=fp0 fp-24=mmmm????
     26: (67) r1 <<= 3                     ; 
R1_w=scalar(smin=0,smax=umax=0x7fffffff8,smax32=0x7ffffff8,umax32=0xfffffff8,var_off=(0x0; 
0x7fffffff8))
     27: (bf) r2 = r10                     ; R2_w=fp0 R10=fp0
     28: (07) r2 += -16                    ; R2_w=fp-16
     29: (0f) r2 += r1                     ; 
R1_w=scalar(smin=0,smax=umax=0x7fffffff8,smax32=0x7ffffff8,umax32=0xfffffff8,var_off=(0x0; 
0x7fffffff8)) 
R2_w=fp(off=-16,smin=0,smax=umax=0x7fffffff8,smax32=0x7ffffff8,umax32=0xfffffff8,var_off=(0x0; 
0x7fffffff8))
     30: (7b) *(u64 *)(r2 +0) = r0
     invalid unbounded variable-offset write to stack R2

... which can be "fixed" by aligning the local key variable

     __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.

And if different vars are used for array access and map lookup, then
map_ptr nullness issue doesn't reproduce.

Any suggestions?