Re: [PATCH bpf-next v1] libbpf: Fix inheritance of BTF pointer size

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

On Thu, May 22, 2025 at 09:37:39AM -0700, Andrii Nakryiko wrote:
> On Wed, May 21, 2025 at 11:21 PM Tony Ambardar <tony.ambardar@gmail.com> wrote:
> >
> > Update btf_new_empty() to copy the pointer size from a provided base BTF.
> > This ensures split BTF works properly and fixes test failures seen on
> > 32-bit targets:
> >
> >   root@qemu-armhf:/usr/libexec/kselftests-bpf# ./test_progs -a btf_split
> >   __test_btf_split:PASS:empty_main_btf 0 nsec
> >   __test_btf_split:PASS:main_ptr_sz 0 nsec
> >   __test_btf_split:PASS:empty_split_btf 0 nsec
> >   __test_btf_split:FAIL:inherit_ptr_sz unexpected inherit_ptr_sz: actual 4 != expected 8
> >   [...]
> >   #41/1    btf_split/single_split:FAIL
> >
> 
> Hm... can you debug it a little bit, please? I see that
> btf__pointer_size() on split BTF will do determine_ptr_size() call,
> which will do
> 
> if (btf->base_btf && btf->base_btf->ptr_sz > 0)
>     return btf->base_btf->ptr_sz;
> 
> So it looks intentional (though I can't claim I remember much of the
> details by now) that we don't proactively cache btf->ptr_sz when
> creating a new split BTF, but it should have resolved into base's
> pointer size. And if it doesn't, let's try to understand why?
> 

Because ptr_sz of new splits is initialized in btf_new_empty() with

    btf->ptr_sz = sizeof(void *);

and base BTF ignored. Thus btf->ptr_sz is non-zero, determine_ptr_size()
does not get called from btf__pointer_size(), and tests pass because BPF
CI validates only 64-bit targets with no 32-bit coverage.

Even with my patch, the ptr_sz code seems problematic and open to abuse.
It appears btf__set_pointer_size() can separately apply different ptr
sizes to base and split BTF, and btf__pointer_size() will likewise return
them.

Thinking out loud, maybe we just set btf->ptr_sz = 0 for all splits. Then
make btf__set_pointer_size() recur to update only the ultimate base BTF,
and btf__pointer_size() does the same, calling determine_ptr_size() if
base BTF ptr_sz == 0. That keeps ptr_sz consistent across multiple splits
I think. Oh, and then add 32-bit and cross-endian CI targets... :-)

WDYT?
 
> > Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> > ---
> >  tools/lib/bpf/btf.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 8d0d0b645a75..b1977888b35e 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -995,6 +995,7 @@ static struct btf *btf_new_empty(struct btf *base_btf)
> >
> >         if (base_btf) {
> >                 btf->base_btf = base_btf;
> > +               btf->ptr_sz = base_btf->ptr_sz;
> >                 btf->start_id = btf__type_cnt(base_btf);
> >                 btf->start_str_off = base_btf->hdr->str_len + base_btf->start_str_off;
> >                 btf->swapped_endian = base_btf->swapped_endian;
> > --
> > 2.34.1
> >