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

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

[Tony Ambardar]

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

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

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

[Alan Maguire]

On 22/05/2025 07:21, Tony Ambardar 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
> 
> Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")
> Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>

Great catch! Nit: should we use the btf_ptr_sz(base_btf) helper here?
That would cover the edge case where base BTF does not have ptr_sz set,
as in the absence of a base pointer size this will then determine the
pointer size from the BTF itself. The reason I ask is it seems like the
BTF raw parsing codepath may not set the pointer size, and we will use
that raw parsing to parse vmlinux BTF from /sys/kernel/btf/vmlinux (ELF
parsing uses the ELF class to set pointer size so we are good there I think)

Reviewed-by: Alan Maguire <alan.maguire@oracle.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;

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

[Andrii Nakryiko]

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?

> 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
>

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

[Tony Ambardar]

On Thu, May 22, 2025 at 09:48:30AM +0100, Alan Maguire wrote:
> On 22/05/2025 07:21, Tony Ambardar 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
> > 
> > Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")
> > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com>
> 
> Great catch! Nit: should we use the btf_ptr_sz(base_btf) helper here?
> That would cover the edge case where base BTF does not have ptr_sz set,
> as in the absence of a base pointer size this will then determine the
> pointer size from the BTF itself. The reason I ask is it seems like the
> BTF raw parsing codepath may not set the pointer size, and we will use
> that raw parsing to parse vmlinux BTF from /sys/kernel/btf/vmlinux (ELF
> parsing uses the ELF class to set pointer size so we are good there I think)
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> 

I tried to keep simple copy semantics consistent with inheritance &
heuristics working as expected, so that edge case should have been OK.
However, looking more closely after Andrii's prompting, I don't think the
original code really works... (see next email).

Regarding raw parsing, let me circle back since it's been a while I was
deep in that source.

Thanks,
Tony

> > ---
> >  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;
> 

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

[Tony Ambardar]

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
> >

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

[Andrii Nakryiko]

On Thu, May 22, 2025 at 11:58 PM Tony Ambardar <tony.ambardar@gmail.com> wrote:
>
> 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?

Yes, that eager btf->ptr_sz assignment in btf_new_empty() looks like a
bug, as you said, we should keep it at zero and let either user set it
or it should be inferred from data, eventually.

As for btf__set_pointer_size(), we either need to recursively set it,
as you said. Or, alternatively, only allow to set it through split BTF
if none of base BTFs have it explicitly set already. Not exactly sure
which semantics makes most sense, but I think we should consider both
options and pick the best one.

>
> > > 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
> > >