| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aDBJSPSL0Oi/SniC@kodidev-ubuntu>
Date: Fri, 23 May 2025 03:09:12 -0700
From: Tony Ambardar <tony.ambardar@...il.com>
To: Amery Hung <ameryhung@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org,
alexei.starovoitov@...il.com, andrii@...nel.org,
daniel@...earbox.net, tj@...nel.org, memxor@...il.com,
martin.lau@...nel.org, kernel-team@...a.com
Subject: Re: [PATCH bpf-next v4 1/3] selftests/bpf: Introduce task local data
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@...il.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;
> > > +};
> > > +
> >
> > [...]
Powered by blists - more mailing lists