On Fri, Jun 6, 2025 at 4:52 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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.
braino. I meant two if r0 checks back to back.
Like:
r1 = %[map_in_map] ll;
call %[bpf_map_lookup_elem];
if r0 == 0 goto l0_%=;
if r0 != 0 goto l0_%=;
r10 = 42;
l0_%=: exit;
kinda obvious looking at asm that the verifier should be
smart to understand that the 2nd if is always taken.
But the smartness is only added in patch 1.
Another test is probably worth it too:
r1 = %[map_in_map] ll;
if r1 != 0 goto l0_%=;
r10 = 42;
l0_%=: exit;
