[bpf-next,v1] libbpf: ensure new BTF objects inherit input endianness

Commit Message

Tony Ambardar Aug. 30, 2024, 9:51 a.m. UTC

The pahole master branch recently added support for "distilled BTF" based
on libbpf v1.5, but may add .BTF and .BTF.base sections with the wrong byte
order (e.g. on s390x BPF CI), which then lead to kernel Oops when loaded.

Fix by updating libbpf's btf__distill_base() and btf_new_empty() to retain
the byte order of any source BTF objects when creating new ones.

Reported-by: Song Liu <[e-mail]>
Reported-by: Eduard Zingerman <[e-mail]>
Suggested-by: Eduard Zingerman <[e-mail]>
Link: https://lore.kernel.org/bpf/[e-mail]/
Signed-off-by: Tony Ambardar <[e-mail]>
---
 tools/lib/bpf/btf.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Eduard Zingerman Aug. 30, 2024, 11:15 a.m. UTC | #1

On Fri, 2024-08-30 at 02:51 -0700, Tony Ambardar wrote:
> The pahole master branch recently added support for "distilled BTF" based
> on libbpf v1.5, but may add .BTF and .BTF.base sections with the wrong byte
> order (e.g. on s390x BPF CI), which then lead to kernel Oops when loaded.
> 
> Fix by updating libbpf's btf__distill_base() and btf_new_empty() to retain
> the byte order of any source BTF objects when creating new ones.
> 
> Reported-by: Song Liu <[e-mail]>
> Reported-by: Eduard Zingerman <[e-mail]>
> Suggested-by: Eduard Zingerman <[e-mail]>
> Link: https://lore.kernel.org/bpf/[e-mail]/
> Signed-off-by: Tony Ambardar <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

But we also need a test for this. Like the one attached.
Or Alan can share his test, which is much shorter but skips round trip to bytes and back.

[...]

Alan Maguire Aug. 30, 2024, 11:25 a.m. UTC | #2

On 30/08/2024 12:15, Eduard Zingerman wrote:
> On Fri, 2024-08-30 at 02:51 -0700, Tony Ambardar wrote:
>> The pahole master branch recently added support for "distilled BTF" based
>> on libbpf v1.5, but may add .BTF and .BTF.base sections with the wrong byte
>> order (e.g. on s390x BPF CI), which then lead to kernel Oops when loaded.
>>
>> Fix by updating libbpf's btf__distill_base() and btf_new_empty() to retain
>> the byte order of any source BTF objects when creating new ones.
>>
>> Reported-by: Song Liu <[e-mail]>
>> Reported-by: Eduard Zingerman <[e-mail]>
>> Suggested-by: Eduard Zingerman <[e-mail]>
>> Link: https://lore.kernel.org/bpf/[e-mail]/
>> Signed-off-by: Tony Ambardar <[e-mail]>
>> ---
> 
> Acked-by: Eduard Zingerman <[e-mail]>
>

Tested-by: Alan Maguire <[e-mail]>

Thanks for the fix!

> But we also need a test for this. Like the one attached.
> Or Alan can share his test, which is much shorter but skips round trip to bytes and back.
>

Eduard's test is better than mine; mine was a simple addition to
btf_endian() tests that checked split/distilled BTF matched endianness
of the originating BTF for non-native endianness. Having actual
non-native endianness _use_ as in Eduard's test is much preferred I think.

> [...]

Andrii Nakryiko Aug. 30, 2024, 3:58 p.m. UTC | #3

On Fri, Aug 30, 2024 at 4:26 AM Alan Maguire <[e-mail]> wrote:
>
> On 30/08/2024 12:15, Eduard Zingerman wrote:
> > On Fri, 2024-08-30 at 02:51 -0700, Tony Ambardar wrote:
> >> The pahole master branch recently added support for "distilled BTF" based
> >> on libbpf v1.5, but may add .BTF and .BTF.base sections with the wrong byte
> >> order (e.g. on s390x BPF CI), which then lead to kernel Oops when loaded.
> >>
> >> Fix by updating libbpf's btf__distill_base() and btf_new_empty() to retain
> >> the byte order of any source BTF objects when creating new ones.
> >>
> >> Reported-by: Song Liu <[e-mail]>
> >> Reported-by: Eduard Zingerman <[e-mail]>
> >> Suggested-by: Eduard Zingerman <[e-mail]>
> >> Link: https://lore.kernel.org/bpf/[e-mail]/
> >> Signed-off-by: Tony Ambardar <[e-mail]>
> >> ---
> >
> > Acked-by: Eduard Zingerman <[e-mail]>
> >
>
> Tested-by: Alan Maguire <[e-mail]>
>
> Thanks for the fix!
>
> > But we also need a test for this. Like the one attached.
> > Or Alan can share his test, which is much shorter but skips round trip to bytes and back.
> >
>
> Eduard's test is better than mine; mine was a simple addition to
> btf_endian() tests that checked split/distilled BTF matched endianness
> of the originating BTF for non-native endianness. Having actual
> non-native endianness _use_ as in Eduard's test is much preferred I think.

Eduard, please send it as a proper patch?

>
> > [...]

Andrii Nakryiko Aug. 30, 2024, 4 p.m. UTC | #4

On Fri, Aug 30, 2024 at 2:52 AM Tony Ambardar <[e-mail]> wrote:
>
> The pahole master branch recently added support for "distilled BTF" based
> on libbpf v1.5, but may add .BTF and .BTF.base sections with the wrong byte

there is no libbpf v1.5 release, are we talking about using unreleased
master branch?

> order (e.g. on s390x BPF CI), which then lead to kernel Oops when loaded.
>
> Fix by updating libbpf's btf__distill_base() and btf_new_empty() to retain
> the byte order of any source BTF objects when creating new ones.
>
> Reported-by: Song Liu <[e-mail]>
> Reported-by: Eduard Zingerman <[e-mail]>
> Suggested-by: Eduard Zingerman <[e-mail]>
> Link: https://lore.kernel.org/bpf/[e-mail]/
> Signed-off-by: Tony Ambardar <[e-mail]>
> ---
>  tools/lib/bpf/btf.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 064cfe126c09..7726b7c6d40a 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -996,6 +996,7 @@ static struct btf *btf_new_empty(struct btf *base_btf)
>                 btf->base_btf = base_btf;
>                 btf->start_id = btf__type_cnt(base_btf);
>                 btf->start_str_off = base_btf->hdr->str_len;
> +               btf->swapped_endian = base_btf->swapped_endian;
>         }
>
>         /* +1 for empty string at offset 0 */
> @@ -5554,6 +5555,10 @@ int btf__distill_base(const struct btf *src_btf, struct btf **new_base_btf,
>         new_base = btf__new_empty();
>         if (!new_base)
>                 return libbpf_err(-ENOMEM);
> +       err = btf__set_endianness(new_base, btf__endianness(src_btf));
> +       if (err < 0)
> +               goto done;

This error check is really unnecessary and paranoid, because the only
way btf__set_endianness() can fail is if the provided endianness enum
is corrupted (some invalid int cast to enum). But in this case we are
getting it from libbpf itself, which will always be correct. So I
think I'll drop the error check while applying.

> +
>         dist.id_map = calloc(n, sizeof(*dist.id_map));
>         if (!dist.id_map) {
>                 err = -ENOMEM;
> --
> 2.34.1
>

Andrii Nakryiko Aug. 30, 2024, 5:51 p.m. UTC | #5

On Fri, Aug 30, 2024 at 9:00 AM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Fri, Aug 30, 2024 at 2:52 AM Tony Ambardar <[e-mail]> wrote:
> >
> > The pahole master branch recently added support for "distilled BTF" based
> > on libbpf v1.5, but may add .BTF and .BTF.base sections with the wrong byte
>
> there is no libbpf v1.5 release, are we talking about using unreleased
> master branch?
>
> > order (e.g. on s390x BPF CI), which then lead to kernel Oops when loaded.
> >
> > Fix by updating libbpf's btf__distill_base() and btf_new_empty() to retain
> > the byte order of any source BTF objects when creating new ones.
> >
> > Reported-by: Song Liu <[e-mail]>
> > Reported-by: Eduard Zingerman <[e-mail]>
> > Suggested-by: Eduard Zingerman <[e-mail]>
> > Link: https://lore.kernel.org/bpf/[e-mail]/
> > Signed-off-by: Tony Ambardar <[e-mail]>
> > ---
> >  tools/lib/bpf/btf.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >

Also added

Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")
Fixes: 58e185a0dc35 ("libbpf: Add btf__distill_base() creating split
BTF with distilled base BTF")

> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 064cfe126c09..7726b7c6d40a 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -996,6 +996,7 @@ static struct btf *btf_new_empty(struct btf *base_btf)
> >                 btf->base_btf = base_btf;
> >                 btf->start_id = btf__type_cnt(base_btf);
> >                 btf->start_str_off = base_btf->hdr->str_len;
> > +               btf->swapped_endian = base_btf->swapped_endian;
> >         }
> >
> >         /* +1 for empty string at offset 0 */
> > @@ -5554,6 +5555,10 @@ int btf__distill_base(const struct btf *src_btf, struct btf **new_base_btf,
> >         new_base = btf__new_empty();
> >         if (!new_base)
> >                 return libbpf_err(-ENOMEM);
> > +       err = btf__set_endianness(new_base, btf__endianness(src_btf));
> > +       if (err < 0)
> > +               goto done;
>
> This error check is really unnecessary and paranoid, because the only
> way btf__set_endianness() can fail is if the provided endianness enum
> is corrupted (some invalid int cast to enum). But in this case we are
> getting it from libbpf itself, which will always be correct. So I
> think I'll drop the error check while applying.
>
> > +
> >         dist.id_map = calloc(n, sizeof(*dist.id_map));
> >         if (!dist.id_map) {
> >                 err = -ENOMEM;
> > --
> > 2.34.1
> >

Patch

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 064cfe126c09..7726b7c6d40a 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -996,6 +996,7 @@  static struct btf *btf_new_empty(struct btf *base_btf)
 		btf->base_btf = base_btf;
 		btf->start_id = btf__type_cnt(base_btf);
 		btf->start_str_off = base_btf->hdr->str_len;
+		btf->swapped_endian = base_btf->swapped_endian;
 	}
 
 	/* +1 for empty string at offset 0 */
@@ -5554,6 +5555,10 @@  int btf__distill_base(const struct btf *src_btf, struct btf **new_base_btf,
 	new_base = btf__new_empty();
 	if (!new_base)
 		return libbpf_err(-ENOMEM);
+	err = btf__set_endianness(new_base, btf__endianness(src_btf));
+	if (err < 0)
+		goto done;
+
 	dist.id_map = calloc(n, sizeof(*dist.id_map));
 	if (!dist.id_map) {
 		err = -ENOMEM;