On Fri, Jun 6, 2025 at 4:38 PM Ihor Solodrai <ihor.solodrai@xxxxxxxxx> wrote:
>
> On 6/5/25 5:25 PM, Alexei Starovoitov wrote:
> > On Thu, Jun 5, 2025 at 4:40 PM Ihor Solodrai <ihor.solodrai@xxxxxxxxx> wrote:
> >>
> >> On 6/5/25 10:00 AM, Alexei Starovoitov wrote:
> >>> On Thu, Jun 5, 2025 at 9:42 AM Ihor Solodrai <ihor.solodrai@xxxxxxxxx> wrote:
> >>>>
> >>>> On 6/5/25 9:24 AM, Andrii Nakryiko wrote:
> >>>>> On Wed, Jun 4, 2025 at 3:28 PM Ihor Solodrai <isolodrai@xxxxxxxx> 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.
Yeah. For this kind of sequence asm is the right answer.
It's not clear to me why you want to spill/fill it.
CONST_PTR_TO_MAP is already in is_spillable_regtype()
before this patch.
The test needs to validate that reg_not_null() is working.
So just:
r1 = %[map_in_map] ll;
call %[bpf_map_lookup_elem];
if r0 == 0 goto l0_%=;
exit;
l0_%=: r10 = 42;
exit;
would do the job without spill/fill.
