Re: [PATCH bpf-next v4 1/3] selftests/bpf: Introduce task local data

[Tony Ambardar <tony.ambardar@gmail.com>]

On Thu, May 22, 2025 at 09:49:23AM -0700, Amery Hung wrote:
> On Thu, May 22, 2025 at 1:36 AM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > Hi Amery,
> >
> > I'm trying out your series in an arm32 JIT testing env I'm working on.
> >
> >
> > On Thu, May 15, 2025 at 02:16:00PM -0700, Amery Hung wrote:
> >
> > > +

[...]

> > > +struct u_tld_data *dummy_data;
> > > +struct u_tld_metadata *dummy_metadata;
> >
> > I suspect I've overlooked something, but what are these 2 "dummy" globals
> > used for? The code builds OK without them, although I do see test errors
> > as noted below.
> >
> 
> Hi, sorry for the confusion. The forward declaration is to prevent
> dummy_data/metadata tld_map_value to be fwd_kind. I will explain this
> in the comment.
> 
> The BTF should look like this:
> 
> [9] STRUCT 'tld_map_value' size=16 vlen=2
>         'data' type_id=10 bits_offset=0
>         'metadata' type_id=11 bits_offset=64
> [10] PTR '(anon)' type_id=74
> [11] PTR '(anon)' type_id=73
> [57] STRUCT 'u_tld_data' size=4096 vlen=1
>         'data' type_id=58 bits_offset=0
> [61] STRUCT 'u_tld_metadata' size=4096 vlen=3
>         'cnt' type_id=62 bits_offset=0
>         'padding' type_id=64 bits_offset=8
>         'metadata' type_id=67 bits_offset=512
> [73] TYPE_TAG 'uptr' type_id=61
> [74] TYPE_TAG 'uptr' type_id=57
> 
> Without the forward declaration, the BTF will look like this:
> 
> [9] STRUCT 'tld_map_value' size=16 vlen=2
>         'data' type_id=10 bits_offset=0
>         'metadata' type_id=11 bits_offset=64
> [10] PTR '(anon)' type_id=63
> [11] PTR '(anon)' type_id=61
> [60] FWD 'u_tld_metadata' fwd_kind=struct
> [61] TYPE_TAG 'uptr' type_id=60
> [62] FWD 'u_tld_data' fwd_kind=struct
> [63] TYPE_TAG 'uptr' type_id=62
> 

Oh, I see. Yes, having some commentary/links would be helpful since this
makes for some tricky debugging. Was there ever any discussion of
alternatives to using the forward decl?

I'm afraid I missed the 'uptr' tag introduction and need to understand
the background.

> > I'll also mention the only reason I noticed these is that "bpftool gen
> > skeleton" automatically maps these to user space, but results in an
> > ASSERT() failure during build on 32-bit targets due to lack of support,
> > so dropping them avoids that.
> 
> Can you provide more details of the error?
> 

bpftool skeleton includes checks when mapping global vars to user space.
These fail on 32-bit archs for types whose sizes differ from the BPF VM
(i.e. longs, pointers):

   In file included from .../tools/testing/selftests/bpf/prog_tests/test_task_local_data.c:13:
   ./test_task_local_data.skel.h: In function ‘test_task_local_data__assert’:
   ./test_task_local_data.skel.h:531:9: error: static assertion failed: "unexpected size of \'dummy_data\'"
     531 |         _Static_assert(sizeof(s->bss->dummy_data) == 8, "unexpected size of 'dummy_data'");
         |         ^~~~~~~~~~~~~~
   ./test_task_local_data.skel.h:532:9: error: static assertion failed: "unexpected size of \'dummy_metadata\'"
     532 |         _Static_assert(sizeof(s->bss->dummy_metadata) == 8, "unexpected size of 'dummy_metadata'");
         |         ^~~~~~~~~~~~~~


On a whim, I tried making the dummy_xxxxxxx vars static but this still
leaves the fwd_kind and leads to verifier rejection.

> >
> >
> > 24: (85) call pc+25
> > caller:
> >  R6_w=map_value(map=tld_key_map,ks=4,vs=6) R7=1 R10=fp0 fp-8_w=map_value(map=tld_key_map,ks=4,vs=6) fp-16=map_value(map=tld_data_map,ks=4,vs=16)
> > callee:
> >  frame1: R1_w=fp[0]-16 R2_w=map_value(map=.rodata.str1.1,ks=4,vs=30) R10=fp0
> > 50: frame1: R1_w=fp[0]-16 R2_w=map_value(map=.rodata.str1.1,ks=4,vs=30) R10=fp0
> > ; static u16 __tld_fetch_key(struct tld_object *tld_obj, const char *name) @ task_local_data.bpf.h:163
> > 50: (7b) *(u64 *)(r10 -16) = r2       ; frame1: R2_w=map_value(map=.rodata.str1.1,ks=4,vs=30) R10=fp0 fp-16_w=map_value(map=.rodata.str1.1,ks=4,vs=30)
> > 51: (b4) w7 = 0                       ; frame1: R7_w=0
> > ; if (!tld_obj->data_map || !tld_obj->data_map->metadata) @ task_local_data.bpf.h:169
> > 52: (79) r1 = *(u64 *)(r1 +0)         ; frame1: R1=map_value(map=tld_data_map,ks=4,vs=16) fp-16=map_value(map=.rodata.str1.1,ks=4,vs=30)
> > 53: (15) if r1 == 0x0 goto pc+36      ; frame1: R1=map_value(map=tld_data_map,ks=4,vs=16)
> > 54: (79) r6 = *(u64 *)(r1 +8)         ; frame1: R1=map_value(map=tld_data_map,ks=4,vs=16) R6_w=scalar()
> > 55: (15) if r6 == 0x0 goto pc+34      ; frame1: R6_w=scalar(umin=1)
> > ; cnt = tld_obj->data_map->metadata->cnt; @ task_local_data.bpf.h:172
> > 56: (71) r8 = *(u8 *)(r6 +0)
> > R6 invalid mem access 'scalar'
> > processed 29 insns (limit 1000000) max_states_per_insn 0 total_states 3 peak_states 3 mark_read 1
> > -- END PROG LOAD LOG --
> > libbpf: prog 'task_init': failed to load: -EACCES
> > libbpf: failed to load object 'test_task_local_data'
> > libbpf: failed to load BPF skeleton 'test_task_local_data': -EACCES
> > test_task_local_data_basic:FAIL:skel_open_and_load unexpected error: -13
> > #409/1   task_local_data/task_local_data_basic:FAIL
> >
> >
> > I'm unsure if this verifier error is related to the dummy pointers, but
> > it does seem there's a pointer issue...
> >
> 
> The error is exactly caused by removing the dummy_xxx.
> 
> > Further thoughts or suggestions (from anyone) would be most welcome.
> >
> > Thanks,
> > Tony
> >
> > > +
> > > +struct tld_metadata {
> > > +     char name[TLD_NAME_LEN];
> > > +     __u16 size;
> > > +};
> > > +
> > > +struct u_tld_metadata {
> > > +     __u8 cnt;
> > > +     __u8 padding[63];
> > > +     struct tld_metadata metadata[TLD_DATA_CNT];
> > > +};
> > > +
> > > +struct u_tld_data {
> > > +     char data[TLD_DATA_SIZE];
> > > +};
> > > +
> > > +struct tld_map_value {
> > > +     struct u_tld_data __uptr *data;
> > > +     struct u_tld_metadata __uptr *metadata;
> > > +};
> > > +
> > > +struct tld_object {
> > > +     struct tld_map_value *data_map;
> > > +     struct tld_keys *key_map;
> > > +};
> > > +
> >
> > [...]