[bpf-next] tools/resolve_btfids: fix cross-compilation to non-host endianness

Commit Message

Viktor Malik Jan. 23, 2024, 12:07 p.m. UTC

The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
build and afterwards patched by resolve_btfids with correct values.
Since resolve_btfids always writes in host-native endianness, it relies
on libelf to do the translation when the target ELF is cross-compiled to
a different endianness (this was introduced in commit 61e8aeda9398
("bpf: Fix libelf endian handling in resolv_btfids")).

Unfortunately, the translation will corrupt the flags fields of SET8
entries because these were written during vmlinux compilation and are in
the correct endianness already. This will lead to numerous selftests
failures such as:

    $ sudo ./test_verifier 502 502
    #502/p sleepable fentry accept FAIL
    Failed to load prog 'Invalid argument'!
    bpf_fentry_test1 is not sleepable
    verification time 34 usec
    stack depth 0
    processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
    Summary: 0 PASSED, 0 SKIPPED, 1 FAILED

Since it's not possible to instruct libelf to translate just certain
values, let's manually bswap the flags in resolve_btfids when needed, so
that libelf then translates everything correctly.

Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
Signed-off-by: Viktor Malik <[e-mail]>
---
 tools/bpf/resolve_btfids/main.c | 35 +++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko Jan. 26, 2024, 11:40 p.m. UTC | #1

On Tue, Jan 23, 2024 at 4:08 AM Viktor Malik <[e-mail]> wrote:
>
> The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
> build and afterwards patched by resolve_btfids with correct values.
> Since resolve_btfids always writes in host-native endianness, it relies
> on libelf to do the translation when the target ELF is cross-compiled to
> a different endianness (this was introduced in commit 61e8aeda9398
> ("bpf: Fix libelf endian handling in resolv_btfids")).
>
> Unfortunately, the translation will corrupt the flags fields of SET8
> entries because these were written during vmlinux compilation and are in
> the correct endianness already. This will lead to numerous selftests
> failures such as:
>
>     $ sudo ./test_verifier 502 502
>     #502/p sleepable fentry accept FAIL
>     Failed to load prog 'Invalid argument'!
>     bpf_fentry_test1 is not sleepable
>     verification time 34 usec
>     stack depth 0
>     processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>     Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
>
> Since it's not possible to instruct libelf to translate just certain
> values, let's manually bswap the flags in resolve_btfids when needed, so
> that libelf then translates everything correctly.
>
> Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
> Signed-off-by: Viktor Malik <[e-mail]>
> ---
>  tools/bpf/resolve_btfids/main.c | 35 +++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 27a23196d58e..440d3d066ce4 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -646,18 +646,31 @@ static int cmp_id(const void *pa, const void *pb)
>         return *a - *b;
>  }
>
> +static int need_bswap(int elf_byte_order)
> +{
> +       return __BYTE_ORDER == __LITTLE_ENDIAN && elf_byte_order != ELFDATA2LSB ||
> +              __BYTE_ORDER == __BIG_ENDIAN && elf_byte_order != ELFDATA2MSB;

return (__BYTE_ORDER == __LITTLE_ENDIAN) != (elf_byte_order == ELFDATA2LSB);

?

> +}
> +
>  static int sets_patch(struct object *obj)
>  {
>         Elf_Data *data = obj->efile.idlist;
>         int *ptr = data->d_buf;
>         struct rb_node *next;
> +       GElf_Ehdr ehdr;
> +
> +       if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
> +               pr_err("FAILED cannot get ELF header: %s\n",
> +                       elf_errmsg(-1));
> +               return -1;
> +       }

calculate needs_bswap() once here?

>
>         next = rb_first(&obj->sets);
>         while (next) {
> -               unsigned long addr, idx;
> +               unsigned long addr, idx, flags;
>                 struct btf_id *id;
>                 int *base;
> -               int cnt;
> +               int cnt, i;
>
>                 id   = rb_entry(next, struct btf_id, rb_node);
>                 addr = id->addr[0];
> @@ -679,6 +692,24 @@ static int sets_patch(struct object *obj)
>
>                 qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
>
> +               /*
> +                * When ELF endianness does not match endianness of the host,
> +                * libelf will do the translation when updating the ELF. This,
> +                * however, corrupts SET8 flags which are already in the target
> +                * endianness. So, let's bswap them to the host endianness and
> +                * libelf will then correctly translate everything.
> +                */
> +               if (id->is_set8 && need_bswap(ehdr.e_ident[EI_DATA])) {
> +                       for (i = 0; i < cnt; i++) {
> +                               /*
> +                                * header and entries are 8-byte, flags is the
> +                                * second half of an entry
> +                                */
> +                               flags = idx + (i + 1) * 2 + 1;
> +                               ptr[flags] = bswap_32(ptr[flags]);

we are dealing with struct btf_id_set8, right? Can't we #include
include/linux/btf_ids.h and use that type for all these offset
calculations?..

I have the same question for existing code, tbh, so maybe there was
some good reason, not sure...

> +                       }
> +               }
> +
>                 next = rb_next(next);
>         }
>         return 0;
> --
> 2.43.0
>
>

Jiri Olsa Jan. 28, 2024, 8:17 p.m. UTC | #2

On Fri, Jan 26, 2024 at 03:40:11PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 23, 2024 at 4:08 AM Viktor Malik <[e-mail]> wrote:
> >
> > The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
> > build and afterwards patched by resolve_btfids with correct values.
> > Since resolve_btfids always writes in host-native endianness, it relies
> > on libelf to do the translation when the target ELF is cross-compiled to
> > a different endianness (this was introduced in commit 61e8aeda9398
> > ("bpf: Fix libelf endian handling in resolv_btfids")).
> >
> > Unfortunately, the translation will corrupt the flags fields of SET8
> > entries because these were written during vmlinux compilation and are in
> > the correct endianness already. This will lead to numerous selftests
> > failures such as:
> >
> >     $ sudo ./test_verifier 502 502
> >     #502/p sleepable fentry accept FAIL
> >     Failed to load prog 'Invalid argument'!
> >     bpf_fentry_test1 is not sleepable
> >     verification time 34 usec
> >     stack depth 0
> >     processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> >     Summary: 0 PASSED, 0 SKIPPED, 1 FAILED

hum, I'd think we should have hit such bug long time ago.. set8 is
there for some time already.. nice ;-)

> >
> > Since it's not possible to instruct libelf to translate just certain
> > values, let's manually bswap the flags in resolve_btfids when needed, so
> > that libelf then translates everything correctly.
> >
> > Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
> > Signed-off-by: Viktor Malik <[e-mail]>
> > ---
> >  tools/bpf/resolve_btfids/main.c | 35 +++++++++++++++++++++++++++++++--
> >  1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > index 27a23196d58e..440d3d066ce4 100644
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> > @@ -646,18 +646,31 @@ static int cmp_id(const void *pa, const void *pb)
> >         return *a - *b;
> >  }
> >
> > +static int need_bswap(int elf_byte_order)
> > +{
> > +       return __BYTE_ORDER == __LITTLE_ENDIAN && elf_byte_order != ELFDATA2LSB ||
> > +              __BYTE_ORDER == __BIG_ENDIAN && elf_byte_order != ELFDATA2MSB;
> 
> return (__BYTE_ORDER == __LITTLE_ENDIAN) != (elf_byte_order == ELFDATA2LSB);
> 
> ?
> 
> > +}
> > +
> >  static int sets_patch(struct object *obj)
> >  {
> >         Elf_Data *data = obj->efile.idlist;
> >         int *ptr = data->d_buf;
> >         struct rb_node *next;
> > +       GElf_Ehdr ehdr;
> > +
> > +       if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
> > +               pr_err("FAILED cannot get ELF header: %s\n",
> > +                       elf_errmsg(-1));
> > +               return -1;
> > +       }
> 
> calculate needs_bswap() once here?
> 
> >
> >         next = rb_first(&obj->sets);
> >         while (next) {
> > -               unsigned long addr, idx;
> > +               unsigned long addr, idx, flags;
> >                 struct btf_id *id;
> >                 int *base;
> > -               int cnt;
> > +               int cnt, i;
> >
> >                 id   = rb_entry(next, struct btf_id, rb_node);
> >                 addr = id->addr[0];
> > @@ -679,6 +692,24 @@ static int sets_patch(struct object *obj)
> >
> >                 qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
> >
> > +               /*
> > +                * When ELF endianness does not match endianness of the host,
> > +                * libelf will do the translation when updating the ELF. This,
> > +                * however, corrupts SET8 flags which are already in the target
> > +                * endianness. So, let's bswap them to the host endianness and
> > +                * libelf will then correctly translate everything.
> > +                */
> > +               if (id->is_set8 && need_bswap(ehdr.e_ident[EI_DATA])) {
> > +                       for (i = 0; i < cnt; i++) {
> > +                               /*
> > +                                * header and entries are 8-byte, flags is the
> > +                                * second half of an entry
> > +                                */
> > +                               flags = idx + (i + 1) * 2 + 1;
> > +                               ptr[flags] = bswap_32(ptr[flags]);
> 
> we are dealing with struct btf_id_set8, right? Can't we #include
> include/linux/btf_ids.h and use that type for all these offset
> calculations?..

we could, there's tools/include/linux/btf_ids.h, which we could include
in here, we do that in selftests.. but it needs to be updated with latest
kernel updates (at least with set8 struct)

> 
> I have the same question for existing code, tbh, so maybe there was
> some good reason, not sure...

I think the test came later and I did not think of it for the resolve_btfids
itself, I guess it might make the code more readable

thanks,
jirka

> 
> > +                       }
> > +               }
> > +
> >                 next = rb_next(next);
> >         }
> >         return 0;
> > --
> > 2.43.0
> >
> >

Viktor Malik Jan. 29, 2024, 10:09 a.m. UTC | #3

On 1/28/24 21:17, Jiri Olsa wrote:
> On Fri, Jan 26, 2024 at 03:40:11PM -0800, Andrii Nakryiko wrote:
>> On Tue, Jan 23, 2024 at 4:08 AM Viktor Malik <[e-mail]> wrote:
>>>
>>> The .BTF_ids section is pre-filled with zeroed BTF ID entries during the
>>> build and afterwards patched by resolve_btfids with correct values.
>>> Since resolve_btfids always writes in host-native endianness, it relies
>>> on libelf to do the translation when the target ELF is cross-compiled to
>>> a different endianness (this was introduced in commit 61e8aeda9398
>>> ("bpf: Fix libelf endian handling in resolv_btfids")).
>>>
>>> Unfortunately, the translation will corrupt the flags fields of SET8
>>> entries because these were written during vmlinux compilation and are in
>>> the correct endianness already. This will lead to numerous selftests
>>> failures such as:
>>>
>>>     $ sudo ./test_verifier 502 502
>>>     #502/p sleepable fentry accept FAIL
>>>     Failed to load prog 'Invalid argument'!
>>>     bpf_fentry_test1 is not sleepable
>>>     verification time 34 usec
>>>     stack depth 0
>>>     processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>>     Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
> 
> hum, I'd think we should have hit such bug long time ago.. set8 is
> there for some time already.. nice ;-)
> 
>>>
>>> Since it's not possible to instruct libelf to translate just certain
>>> values, let's manually bswap the flags in resolve_btfids when needed, so
>>> that libelf then translates everything correctly.
>>>
>>> Fixes: ef2c6f370a63 ("tools/resolve_btfids: Add support for 8-byte BTF sets")
>>> Signed-off-by: Viktor Malik <[e-mail]>
>>> ---
>>>  tools/bpf/resolve_btfids/main.c | 35 +++++++++++++++++++++++++++++++--
>>>  1 file changed, 33 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>>> index 27a23196d58e..440d3d066ce4 100644
>>> --- a/tools/bpf/resolve_btfids/main.c
>>> +++ b/tools/bpf/resolve_btfids/main.c
>>> @@ -646,18 +646,31 @@ static int cmp_id(const void *pa, const void *pb)
>>>         return *a - *b;
>>>  }
>>>
>>> +static int need_bswap(int elf_byte_order)
>>> +{
>>> +       return __BYTE_ORDER == __LITTLE_ENDIAN && elf_byte_order != ELFDATA2LSB ||
>>> +              __BYTE_ORDER == __BIG_ENDIAN && elf_byte_order != ELFDATA2MSB;
>>
>> return (__BYTE_ORDER == __LITTLE_ENDIAN) != (elf_byte_order == ELFDATA2LSB);
>>
>> ?
>>

It seemed to me a bit less readable this way, but sure, no problem with
this form either.

>>> +}
>>> +
>>>  static int sets_patch(struct object *obj)
>>>  {
>>>         Elf_Data *data = obj->efile.idlist;
>>>         int *ptr = data->d_buf;
>>>         struct rb_node *next;
>>> +       GElf_Ehdr ehdr;
>>> +
>>> +       if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
>>> +               pr_err("FAILED cannot get ELF header: %s\n",
>>> +                       elf_errmsg(-1));
>>> +               return -1;
>>> +       }
>>
>> calculate needs_bswap() once here?

Good idea, will do.

>>>
>>>         next = rb_first(&obj->sets);
>>>         while (next) {
>>> -               unsigned long addr, idx;
>>> +               unsigned long addr, idx, flags;
>>>                 struct btf_id *id;
>>>                 int *base;
>>> -               int cnt;
>>> +               int cnt, i;
>>>
>>>                 id   = rb_entry(next, struct btf_id, rb_node);
>>>                 addr = id->addr[0];
>>> @@ -679,6 +692,24 @@ static int sets_patch(struct object *obj)
>>>
>>>                 qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
>>>
>>> +               /*
>>> +                * When ELF endianness does not match endianness of the host,
>>> +                * libelf will do the translation when updating the ELF. This,
>>> +                * however, corrupts SET8 flags which are already in the target
>>> +                * endianness. So, let's bswap them to the host endianness and
>>> +                * libelf will then correctly translate everything.
>>> +                */
>>> +               if (id->is_set8 && need_bswap(ehdr.e_ident[EI_DATA])) {
>>> +                       for (i = 0; i < cnt; i++) {
>>> +                               /*
>>> +                                * header and entries are 8-byte, flags is the
>>> +                                * second half of an entry
>>> +                                */
>>> +                               flags = idx + (i + 1) * 2 + 1;
>>> +                               ptr[flags] = bswap_32(ptr[flags]);
>>
>> we are dealing with struct btf_id_set8, right? Can't we #include
>> include/linux/btf_ids.h and use that type for all these offset
>> calculations?..
> 
> we could, there's tools/include/linux/btf_ids.h, which we could include
> in here, we do that in selftests.. but it needs to be updated with latest
> kernel updates (at least with set8 struct)
> 
>>
>> I have the same question for existing code, tbh, so maybe there was
>> some good reason, not sure...
> 
> I think the test came later and I did not think of it for the resolve_btfids
> itself, I guess it might make the code more readable

Agreed, let's use that. I'll also refactor the existing code using types
from btf_ids.h for v2 of this patchset.

Viktor

> 
> thanks,
> jirka
> 
>>
>>> +                       }
>>> +               }
>>> +
>>>                 next = rb_next(next);
>>>         }
>>>         return 0;
>>> --
>>> 2.43.0
>>>
>>>
>

Patch

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index 27a23196d58e..440d3d066ce4 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -646,18 +646,31 @@  static int cmp_id(const void *pa, const void *pb)
 	return *a - *b;
 }
 
+static int need_bswap(int elf_byte_order)
+{
+	return __BYTE_ORDER == __LITTLE_ENDIAN && elf_byte_order != ELFDATA2LSB ||
+	       __BYTE_ORDER == __BIG_ENDIAN && elf_byte_order != ELFDATA2MSB;
+}
+
 static int sets_patch(struct object *obj)
 {
 	Elf_Data *data = obj->efile.idlist;
 	int *ptr = data->d_buf;
 	struct rb_node *next;
+	GElf_Ehdr ehdr;
+
+	if (gelf_getehdr(obj->efile.elf, &ehdr) == NULL) {
+		pr_err("FAILED cannot get ELF header: %s\n",
+			elf_errmsg(-1));
+		return -1;
+	}
 
 	next = rb_first(&obj->sets);
 	while (next) {
-		unsigned long addr, idx;
+		unsigned long addr, idx, flags;
 		struct btf_id *id;
 		int *base;
-		int cnt;
+		int cnt, i;
 
 		id   = rb_entry(next, struct btf_id, rb_node);
 		addr = id->addr[0];
@@ -679,6 +692,24 @@  static int sets_patch(struct object *obj)
 
 		qsort(base, cnt, id->is_set8 ? sizeof(uint64_t) : sizeof(int), cmp_id);
 
+		/*
+		 * When ELF endianness does not match endianness of the host,
+		 * libelf will do the translation when updating the ELF. This,
+		 * however, corrupts SET8 flags which are already in the target
+		 * endianness. So, let's bswap them to the host endianness and
+		 * libelf will then correctly translate everything.
+		 */
+		if (id->is_set8 && need_bswap(ehdr.e_ident[EI_DATA])) {
+			for (i = 0; i < cnt; i++) {
+				/*
+				 * header and entries are 8-byte, flags is the
+				 * second half of an entry
+				 */
+				flags = idx + (i + 1) * 2 + 1;
+				ptr[flags] = bswap_32(ptr[flags]);
+			}
+		}
+
 		next = rb_next(next);
 	}
 	return 0;