* [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids
@ 2021-06-16 9:25 Tony Ambardar
2021-06-16 15:56 ` Yonghong Song
2021-06-18 6:14 ` [PATCH bpf v2] " Tony Ambardar
0 siblings, 2 replies; 12+ messages in thread
From: Tony Ambardar @ 2021-06-16 9:25 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko
Cc: Tony Ambardar, bpf, netdev, stable, Jiri Olsa, Yonghong Song
While patching the .BTF_ids section in vmlinux, resolve_btfids writes type
ids using host-native endianness, and relies on libelf for any required
translation when finally updating vmlinux. However, the default type of the
.BTF_ids section content is ELF_T_BYTE (i.e. unsigned char), and undergoes
no translation. This results in incorrect patched values if cross-compiling
to non-native endianness, and can manifest as kernel Oops and test failures
which are difficult to debug.
Explicitly set the type of patched data to ELF_T_WORD, allowing libelf to
transparently handle the endian conversions.
Fixes: fbbb68de80a4 ("bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object")
Cc: stable@vger.kernel.org # v5.10+
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Link: https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_PykcsoeA@mail.gmail.com/
Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com>
---
tools/bpf/resolve_btfids/main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index d636643ddd35..f32c059fbfb4 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj)
if (sets_patch(obj))
return -1;
+ /* Set type to ensure endian translation occurs. */
+ obj->efile.idlist->d_type = ELF_T_WORD;
+
elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY);
err = elf_update(obj->efile.elf, ELF_C_WRITE);
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-16 9:25 [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids Tony Ambardar @ 2021-06-16 15:56 ` Yonghong Song 2021-06-16 16:38 ` Jiri Olsa 2021-06-18 6:14 ` [PATCH bpf v2] " Tony Ambardar 1 sibling, 1 reply; 12+ messages in thread From: Yonghong Song @ 2021-06-16 15:56 UTC (permalink / raw) To: Tony Ambardar, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko Cc: bpf, netdev, stable, Jiri Olsa On 6/16/21 2:25 AM, Tony Ambardar wrote: > While patching the .BTF_ids section in vmlinux, resolve_btfids writes type > ids using host-native endianness, and relies on libelf for any required > translation when finally updating vmlinux. However, the default type of the > .BTF_ids section content is ELF_T_BYTE (i.e. unsigned char), and undergoes > no translation. This results in incorrect patched values if cross-compiling > to non-native endianness, and can manifest as kernel Oops and test failures > which are difficult to debug. > > Explicitly set the type of patched data to ELF_T_WORD, allowing libelf to > transparently handle the endian conversions. > > Fixes: fbbb68de80a4 ("bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object") > Cc: stable@vger.kernel.org # v5.10+ > Cc: Jiri Olsa <jolsa@kernel.org> > Cc: Yonghong Song <yhs@fb.com> > Link: https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_PykcsoeA@mail.gmail.com/ > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> > --- > tools/bpf/resolve_btfids/main.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > index d636643ddd35..f32c059fbfb4 100644 > --- a/tools/bpf/resolve_btfids/main.c > +++ b/tools/bpf/resolve_btfids/main.c > @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) > if (sets_patch(obj)) > return -1; > > + /* Set type to ensure endian translation occurs. */ > + obj->efile.idlist->d_type = ELF_T_WORD; The change makes sense to me as .BTF_ids contains just a list of u32's. Jiri, could you double check on this? > + > elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY); > > err = elf_update(obj->efile.elf, ELF_C_WRITE); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-16 15:56 ` Yonghong Song @ 2021-06-16 16:38 ` Jiri Olsa 2021-06-16 22:09 ` Tony Ambardar 2021-06-16 22:28 ` Mark Wielaard 0 siblings, 2 replies; 12+ messages in thread From: Jiri Olsa @ 2021-06-16 16:38 UTC (permalink / raw) To: Yonghong Song Cc: Tony Ambardar, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, bpf, netdev, stable, Jiri Olsa, Frank Eigler, Mark Wielaard On Wed, Jun 16, 2021 at 08:56:42AM -0700, Yonghong Song wrote: > > > On 6/16/21 2:25 AM, Tony Ambardar wrote: > > While patching the .BTF_ids section in vmlinux, resolve_btfids writes type > > ids using host-native endianness, and relies on libelf for any required > > translation when finally updating vmlinux. However, the default type of the > > .BTF_ids section content is ELF_T_BYTE (i.e. unsigned char), and undergoes > > no translation. This results in incorrect patched values if cross-compiling > > to non-native endianness, and can manifest as kernel Oops and test failures > > which are difficult to debug. nice catch, great libelf can do that ;-) > > > > Explicitly set the type of patched data to ELF_T_WORD, allowing libelf to > > transparently handle the endian conversions. > > > > Fixes: fbbb68de80a4 ("bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object") > > Cc: stable@vger.kernel.org # v5.10+ > > Cc: Jiri Olsa <jolsa@kernel.org> > > Cc: Yonghong Song <yhs@fb.com> > > Link: https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_PykcsoeA@mail.gmail.com/ > > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> > > --- > > tools/bpf/resolve_btfids/main.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > > index d636643ddd35..f32c059fbfb4 100644 > > --- a/tools/bpf/resolve_btfids/main.c > > +++ b/tools/bpf/resolve_btfids/main.c > > @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) > > if (sets_patch(obj)) > > return -1; > > + /* Set type to ensure endian translation occurs. */ > > + obj->efile.idlist->d_type = ELF_T_WORD; > > The change makes sense to me as .BTF_ids contains just a list of > u32's. > > Jiri, could you double check on this? the comment in ELF_T_WORD declaration suggests the size depends on elf's class? ELF_T_WORD, /* Elf32_Word, Elf64_Word, ... */ data in .BTF_ids section are allways u32 I have no idea how is this handled in libelf (perhaps it's ok), but just that comment above suggests it could be also 64 bits, cc-ing Frank and Mark for more insight thanks, jirka > > > + > > elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY); > > err = elf_update(obj->efile.elf, ELF_C_WRITE); > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-16 16:38 ` Jiri Olsa @ 2021-06-16 22:09 ` Tony Ambardar 2021-06-17 9:10 ` Jiri Olsa 2021-06-16 22:28 ` Mark Wielaard 1 sibling, 1 reply; 12+ messages in thread From: Tony Ambardar @ 2021-06-16 22:09 UTC (permalink / raw) To: Jiri Olsa Cc: Yonghong Song, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, bpf, Networking, Stable, Jiri Olsa, Frank Eigler, Mark Wielaard On Wed, 16 Jun 2021 at 09:38, Jiri Olsa <jolsa@redhat.com> wrote: > > On Wed, Jun 16, 2021 at 08:56:42AM -0700, Yonghong Song wrote: > > > > On 6/16/21 2:25 AM, Tony Ambardar wrote: > > > While patching the .BTF_ids section in vmlinux, resolve_btfids writes type > > > ids using host-native endianness, and relies on libelf for any required > > > translation when finally updating vmlinux. However, the default type of the > > > .BTF_ids section content is ELF_T_BYTE (i.e. unsigned char), and undergoes > > > no translation. This results in incorrect patched values if cross-compiling > > > to non-native endianness, and can manifest as kernel Oops and test failures > > > which are difficult to debug. > > nice catch, great libelf can do that ;-) Funny, I'd actually assumed that was your intention, but I just couldn't find where the data type was being set, so resorted to this "kludge". While there's a .BTF_ids section definition in include/linux/btf_ids.h, there's no means I can see to specify the data type either (i.e. in the gcc asm .pushsection() options). That approach would be cleaner. > > > > > > > Explicitly set the type of patched data to ELF_T_WORD, allowing libelf to > > > transparently handle the endian conversions. > > > > > > Fixes: fbbb68de80a4 ("bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object") > > > Cc: stable@vger.kernel.org # v5.10+ > > > Cc: Jiri Olsa <jolsa@kernel.org> > > > Cc: Yonghong Song <yhs@fb.com> > > > Link: https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_PykcsoeA@mail.gmail.com/ > > > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> > > > --- > > > tools/bpf/resolve_btfids/main.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > > > index d636643ddd35..f32c059fbfb4 100644 > > > --- a/tools/bpf/resolve_btfids/main.c > > > +++ b/tools/bpf/resolve_btfids/main.c > > > @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) > > > if (sets_patch(obj)) > > > return -1; > > > + /* Set type to ensure endian translation occurs. */ > > > + obj->efile.idlist->d_type = ELF_T_WORD; > > > > The change makes sense to me as .BTF_ids contains just a list of > > u32's. > > > > Jiri, could you double check on this? > > the comment in ELF_T_WORD declaration suggests the size depends on > elf's class? > > ELF_T_WORD, /* Elf32_Word, Elf64_Word, ... */ > > data in .BTF_ids section are allways u32 > I believe the Elf32/Elf64 refer to the arch since some data structures vary between the two, but ELF_T_WORD is common to both, and valid as the data type of Elf_Data struct holding the .BTF_ids contents. See elf(5): Basic types The following types are used for N-bit architectures (N=32,64, ElfN stands for Elf32 or Elf64, uintN_t stands for uint32_t or uint64_t): ... ElfN_Word uint32_t Also see the code and comments in "elf.h": /* Types for signed and unsigned 32-bit quantities. */ typedef uint32_t Elf32_Word; typedef uint32_t Elf64_Word; > I have no idea how is this handled in libelf (perhaps it's ok), > but just that comment above suggests it could be also 64 bits, > cc-ing Frank and Mark for more insight > One other area I'd like to confirm is with section compression. Is it safe to ignore this for .BTF_ids? I've done so because include/linux/btf_ids.h appears to define the section with SHF_ALLOC flag set, which is incompatible with compression based on "libelf.h" comments. Thanks for reviewing, Tony > thanks, > jirka > > > > > > + > > > elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY); > > > err = elf_update(obj->efile.elf, ELF_C_WRITE); > > > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-16 22:09 ` Tony Ambardar @ 2021-06-17 9:10 ` Jiri Olsa 2021-06-17 10:44 ` Mark Wielaard 0 siblings, 1 reply; 12+ messages in thread From: Jiri Olsa @ 2021-06-17 9:10 UTC (permalink / raw) To: Tony Ambardar Cc: Yonghong Song, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, bpf, Networking, Stable, Jiri Olsa, Frank Eigler, Mark Wielaard On Wed, Jun 16, 2021 at 03:09:13PM -0700, Tony Ambardar wrote: > On Wed, 16 Jun 2021 at 09:38, Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Wed, Jun 16, 2021 at 08:56:42AM -0700, Yonghong Song wrote: > > > > > > On 6/16/21 2:25 AM, Tony Ambardar wrote: > > > > While patching the .BTF_ids section in vmlinux, resolve_btfids writes type > > > > ids using host-native endianness, and relies on libelf for any required > > > > translation when finally updating vmlinux. However, the default type of the > > > > .BTF_ids section content is ELF_T_BYTE (i.e. unsigned char), and undergoes > > > > no translation. This results in incorrect patched values if cross-compiling > > > > to non-native endianness, and can manifest as kernel Oops and test failures > > > > which are difficult to debug. > > > > nice catch, great libelf can do that ;-) > > Funny, I'd actually assumed that was your intention, but I just > couldn't find where the > data type was being set, so resorted to this "kludge". While there's a .BTF_ids > section definition in include/linux/btf_ids.h, there's no means I can > see to specify > the data type either (i.e. in the gcc asm .pushsection() options). That approach > would be cleaner. > > > > > > > > > > > Explicitly set the type of patched data to ELF_T_WORD, allowing libelf to > > > > transparently handle the endian conversions. > > > > > > > > Fixes: fbbb68de80a4 ("bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object") > > > > Cc: stable@vger.kernel.org # v5.10+ > > > > Cc: Jiri Olsa <jolsa@kernel.org> > > > > Cc: Yonghong Song <yhs@fb.com> > > > > Link: https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_PykcsoeA@mail.gmail.com/ > > > > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> > > > > --- > > > > tools/bpf/resolve_btfids/main.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > > > > index d636643ddd35..f32c059fbfb4 100644 > > > > --- a/tools/bpf/resolve_btfids/main.c > > > > +++ b/tools/bpf/resolve_btfids/main.c > > > > @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) > > > > if (sets_patch(obj)) > > > > return -1; > > > > + /* Set type to ensure endian translation occurs. */ > > > > + obj->efile.idlist->d_type = ELF_T_WORD; > > > > > > The change makes sense to me as .BTF_ids contains just a list of > > > u32's. > > > > > > Jiri, could you double check on this? > > > > the comment in ELF_T_WORD declaration suggests the size depends on > > elf's class? > > > > ELF_T_WORD, /* Elf32_Word, Elf64_Word, ... */ > > > > data in .BTF_ids section are allways u32 > > > > I believe the Elf32/Elf64 refer to the arch since some data structures vary > between the two, but ELF_T_WORD is common to both, and valid as the > data type of Elf_Data struct holding the .BTF_ids contents. See elf(5): > > Basic types > The following types are used for N-bit architectures (N=32,64, ElfN > stands for Elf32 or Elf64, uintN_t stands for uint32_t or uint64_t): > ... > ElfN_Word uint32_t > > Also see the code and comments in "elf.h": > /* Types for signed and unsigned 32-bit quantities. */ > typedef uint32_t Elf32_Word; > typedef uint32_t Elf64_Word; ok > > > I have no idea how is this handled in libelf (perhaps it's ok), > > but just that comment above suggests it could be also 64 bits, > > cc-ing Frank and Mark for more insight > > > > One other area I'd like to confirm is with section compression. Is it safe > to ignore this for .BTF_ids? I've done so because include/linux/btf_ids.h > appears to define the section with SHF_ALLOC flag set, which is > incompatible with compression based on "libelf.h" comments. not sure what you mean.. where it wouldn't be safe? what workflow/processing thanks, jirka ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-17 9:10 ` Jiri Olsa @ 2021-06-17 10:44 ` Mark Wielaard 0 siblings, 0 replies; 12+ messages in thread From: Mark Wielaard @ 2021-06-17 10:44 UTC (permalink / raw) To: Jiri Olsa, Tony Ambardar Cc: Yonghong Song, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, bpf, Networking, Stable, Jiri Olsa, Frank Eigler On Thu, 2021-06-17 at 11:10 +0200, Jiri Olsa wrote: > On Wed, Jun 16, 2021 at 03:09:13PM -0700, Tony Ambardar wrote: > > On Wed, 16 Jun 2021 at 09:38, Jiri Olsa <jolsa@redhat.com> wrote: > > > I have no idea how is this handled in libelf (perhaps it's ok), > > > but just that comment above suggests it could be also 64 bits, > > > cc-ing Frank and Mark for more insight > > > > > > > One other area I'd like to confirm is with section compression. Is > > it safe > > to ignore this for .BTF_ids? I've done so because > > include/linux/btf_ids.h > > appears to define the section with SHF_ALLOC flag set, which is > > incompatible with compression based on "libelf.h" comments. > > not sure what you mean.. where it wouldn't be safe? > what workflow/processing I haven't looked at the code/patch, but Tony is correct that if a section has SHF_ALLOC set it cannot be a compressed section. SHF_COMPRESSED is incompatbile with SHF_ALLOC (or SHF_NOBITS) sections, because it would be unclear what a loader would need to do with them (uncompress the data first, then map it, or map the compressed data as is into memory). So ignoring whether or not a section is compressed for SHF_ALLOC sections is fine. Cheers, Mark ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-16 16:38 ` Jiri Olsa 2021-06-16 22:09 ` Tony Ambardar @ 2021-06-16 22:28 ` Mark Wielaard 2021-06-17 9:02 ` Jiri Olsa 1 sibling, 1 reply; 12+ messages in thread From: Mark Wielaard @ 2021-06-16 22:28 UTC (permalink / raw) To: Jiri Olsa Cc: Yonghong Song, Tony Ambardar, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, bpf, netdev, stable, Jiri Olsa, Frank Eigler Hoi, On Wed, Jun 16, 2021 at 06:38:33PM +0200, Jiri Olsa wrote: > > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > > > index d636643ddd35..f32c059fbfb4 100644 > > > --- a/tools/bpf/resolve_btfids/main.c > > > +++ b/tools/bpf/resolve_btfids/main.c > > > @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) > > > if (sets_patch(obj)) > > > return -1; > > > + /* Set type to ensure endian translation occurs. */ > > > + obj->efile.idlist->d_type = ELF_T_WORD; > > > > The change makes sense to me as .BTF_ids contains just a list of > > u32's. > > > > Jiri, could you double check on this? > > the comment in ELF_T_WORD declaration suggests the size depends on > elf's class? > > ELF_T_WORD, /* Elf32_Word, Elf64_Word, ... */ > > data in .BTF_ids section are allways u32 > > I have no idea how is this handled in libelf (perhaps it's ok), > but just that comment above suggests it could be also 64 bits, > cc-ing Frank and Mark for more insight It is correct to use ELF_T_WORD, which means a 32bit unsigned word. The comment is meant to explain that, but is really confusing if you don't know that Elf32_Word and Elf64_Word are the same thing (a 32bit unsigned word). This comes from being "too consistent" in defining all data types for both 32bit and 64bit ELF, even if those types are the same in both formats... Only Elf32_Addr/Elf64_Addr and Elf32_Off/Elf64_Off are different sizes. But Elf32/Elf_64_Half (16 bit), Elf32/Elf64_Word (32 bit), Elf32/Elf64_Xword (64 bit) and their Sword/Sxword (signed) variants are all identical data types in both the Elf32 and Elf64 formats. I don't really know why. It seems the original ELF spec was 32bit only and when introducing the ELF64 format "they" simply duplicated all data types whether or not those data type were actually different between the 32 and 64 bit format. Cheers, Mark ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-16 22:28 ` Mark Wielaard @ 2021-06-17 9:02 ` Jiri Olsa 2021-06-17 11:22 ` Daniel Borkmann 0 siblings, 1 reply; 12+ messages in thread From: Jiri Olsa @ 2021-06-17 9:02 UTC (permalink / raw) To: Mark Wielaard Cc: Yonghong Song, Tony Ambardar, Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko, bpf, netdev, stable, Jiri Olsa, Frank Eigler On Thu, Jun 17, 2021 at 12:28:00AM +0200, Mark Wielaard wrote: > Hoi, > > On Wed, Jun 16, 2021 at 06:38:33PM +0200, Jiri Olsa wrote: > > > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > > > > index d636643ddd35..f32c059fbfb4 100644 > > > > --- a/tools/bpf/resolve_btfids/main.c > > > > +++ b/tools/bpf/resolve_btfids/main.c > > > > @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) > > > > if (sets_patch(obj)) > > > > return -1; > > > > + /* Set type to ensure endian translation occurs. */ > > > > + obj->efile.idlist->d_type = ELF_T_WORD; > > > > > > The change makes sense to me as .BTF_ids contains just a list of > > > u32's. > > > > > > Jiri, could you double check on this? > > > > the comment in ELF_T_WORD declaration suggests the size depends on > > elf's class? > > > > ELF_T_WORD, /* Elf32_Word, Elf64_Word, ... */ > > > > data in .BTF_ids section are allways u32 > > > > I have no idea how is this handled in libelf (perhaps it's ok), > > but just that comment above suggests it could be also 64 bits, > > cc-ing Frank and Mark for more insight > > It is correct to use ELF_T_WORD, which means a 32bit unsigned word. > > The comment is meant to explain that, but is really confusing if you > don't know that Elf32_Word and Elf64_Word are the same thing (a 32bit > unsigned word). This comes from being "too consistent" in defining all > data types for both 32bit and 64bit ELF, even if those types are the > same in both formats... > > Only Elf32_Addr/Elf64_Addr and Elf32_Off/Elf64_Off are different > sizes. But Elf32/Elf_64_Half (16 bit), Elf32/Elf64_Word (32 bit), > Elf32/Elf64_Xword (64 bit) and their Sword/Sxword (signed) variants > are all identical data types in both the Elf32 and Elf64 formats. > > I don't really know why. It seems the original ELF spec was 32bit only > and when introducing the ELF64 format "they" simply duplicated all > data types whether or not those data type were actually different > between the 32 and 64 bit format. nice, thanks for details Acked-by: Jiri Olsa <jolsa@redhat.com> jirka ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-17 9:02 ` Jiri Olsa @ 2021-06-17 11:22 ` Daniel Borkmann 2021-06-18 0:20 ` Tony Ambardar 0 siblings, 1 reply; 12+ messages in thread From: Daniel Borkmann @ 2021-06-17 11:22 UTC (permalink / raw) To: Jiri Olsa, Mark Wielaard Cc: Yonghong Song, Tony Ambardar, Alexei Starovoitov, Andrii Nakryiko, bpf, netdev, stable, Jiri Olsa, Frank Eigler On 6/17/21 11:02 AM, Jiri Olsa wrote: > On Thu, Jun 17, 2021 at 12:28:00AM +0200, Mark Wielaard wrote: >> On Wed, Jun 16, 2021 at 06:38:33PM +0200, Jiri Olsa wrote: >>>>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c >>>>> index d636643ddd35..f32c059fbfb4 100644 >>>>> --- a/tools/bpf/resolve_btfids/main.c >>>>> +++ b/tools/bpf/resolve_btfids/main.c >>>>> @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) >>>>> if (sets_patch(obj)) >>>>> return -1; >>>>> + /* Set type to ensure endian translation occurs. */ >>>>> + obj->efile.idlist->d_type = ELF_T_WORD; >>>> >>>> The change makes sense to me as .BTF_ids contains just a list of >>>> u32's. >>>> >>>> Jiri, could you double check on this? >>> >>> the comment in ELF_T_WORD declaration suggests the size depends on >>> elf's class? >>> >>> ELF_T_WORD, /* Elf32_Word, Elf64_Word, ... */ >>> >>> data in .BTF_ids section are allways u32 >>> >>> I have no idea how is this handled in libelf (perhaps it's ok), >>> but just that comment above suggests it could be also 64 bits, >>> cc-ing Frank and Mark for more insight >> >> It is correct to use ELF_T_WORD, which means a 32bit unsigned word. >> >> The comment is meant to explain that, but is really confusing if you >> don't know that Elf32_Word and Elf64_Word are the same thing (a 32bit >> unsigned word). This comes from being "too consistent" in defining all >> data types for both 32bit and 64bit ELF, even if those types are the >> same in both formats... >> >> Only Elf32_Addr/Elf64_Addr and Elf32_Off/Elf64_Off are different >> sizes. But Elf32/Elf_64_Half (16 bit), Elf32/Elf64_Word (32 bit), >> Elf32/Elf64_Xword (64 bit) and their Sword/Sxword (signed) variants >> are all identical data types in both the Elf32 and Elf64 formats. >> >> I don't really know why. It seems the original ELF spec was 32bit only >> and when introducing the ELF64 format "they" simply duplicated all >> data types whether or not those data type were actually different >> between the 32 and 64 bit format. > > nice, thanks for details > > Acked-by: Jiri Olsa <jolsa@redhat.com> Tony, could you do a v2 and summarize the remainder of the discussion in here for the commit message? Would be good to explicitly document the assumptions made and why they work. Thanks everyone, Daniel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids 2021-06-17 11:22 ` Daniel Borkmann @ 2021-06-18 0:20 ` Tony Ambardar 0 siblings, 0 replies; 12+ messages in thread From: Tony Ambardar @ 2021-06-18 0:20 UTC (permalink / raw) To: Daniel Borkmann Cc: Jiri Olsa, Mark Wielaard, Yonghong Song, Alexei Starovoitov, Andrii Nakryiko, bpf, Networking, Stable, Jiri Olsa, Frank Eigler On Thu, 17 Jun 2021 at 04:22, Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 6/17/21 11:02 AM, Jiri Olsa wrote: > > On Thu, Jun 17, 2021 at 12:28:00AM +0200, Mark Wielaard wrote: > >> On Wed, Jun 16, 2021 at 06:38:33PM +0200, Jiri Olsa wrote: > >>>>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > >>>>> index d636643ddd35..f32c059fbfb4 100644 > >>>>> --- a/tools/bpf/resolve_btfids/main.c > >>>>> +++ b/tools/bpf/resolve_btfids/main.c > >>>>> @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) > >>>>> if (sets_patch(obj)) > >>>>> return -1; > >>>>> + /* Set type to ensure endian translation occurs. */ > >>>>> + obj->efile.idlist->d_type = ELF_T_WORD; > >>>> > >>>> The change makes sense to me as .BTF_ids contains just a list of > >>>> u32's. > >>>> > >>>> Jiri, could you double check on this? > >>> > >>> the comment in ELF_T_WORD declaration suggests the size depends on > >>> elf's class? > >>> > >>> ELF_T_WORD, /* Elf32_Word, Elf64_Word, ... */ > >>> > >>> data in .BTF_ids section are allways u32 > >>> > >>> I have no idea how is this handled in libelf (perhaps it's ok), > >>> but just that comment above suggests it could be also 64 bits, > >>> cc-ing Frank and Mark for more insight > >> > >> It is correct to use ELF_T_WORD, which means a 32bit unsigned word. > >> > >> The comment is meant to explain that, but is really confusing if you > >> don't know that Elf32_Word and Elf64_Word are the same thing (a 32bit > >> unsigned word). This comes from being "too consistent" in defining all > >> data types for both 32bit and 64bit ELF, even if those types are the > >> same in both formats... > >> > >> Only Elf32_Addr/Elf64_Addr and Elf32_Off/Elf64_Off are different > >> sizes. But Elf32/Elf_64_Half (16 bit), Elf32/Elf64_Word (32 bit), > >> Elf32/Elf64_Xword (64 bit) and their Sword/Sxword (signed) variants > >> are all identical data types in both the Elf32 and Elf64 formats. > >> > >> I don't really know why. It seems the original ELF spec was 32bit only > >> and when introducing the ELF64 format "they" simply duplicated all > >> data types whether or not those data type were actually different > >> between the 32 and 64 bit format. > > > > nice, thanks for details > > > > Acked-by: Jiri Olsa <jolsa@redhat.com> > > Tony, could you do a v2 and summarize the remainder of the discussion in > here for the commit message? Would be good to explicitly document the > assumptions made and why they work. Sure, Daniel, I'll update the commit details and resend. Thanks, Tony > Thanks everyone, > Daniel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf v2] bpf: fix libelf endian handling in resolv_btfids 2021-06-16 9:25 [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids Tony Ambardar 2021-06-16 15:56 ` Yonghong Song @ 2021-06-18 6:14 ` Tony Ambardar 2021-06-18 15:10 ` patchwork-bot+netdevbpf 1 sibling, 1 reply; 12+ messages in thread From: Tony Ambardar @ 2021-06-18 6:14 UTC (permalink / raw) To: Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko Cc: Tony Ambardar, bpf, netdev, linux-mips, stable, Frank Eigler, Mark Wielaard, Jiri Olsa, Yonghong Song, Jiri Olsa The vmlinux ".BTF_ids" ELF section is declared in btf_ids.h to hold a list of zero-filled BTF IDs, which is then patched at link-time with correct values by resolv_btfids. The section is flagged as "allocable" to preclude compression, but notably the section contents (BTF IDs) are untyped. When patching the BTF IDs, resolve_btfids writes in host-native endianness and relies on libelf for any required translation on reading and updating vmlinux. However, since the type of the .BTF_ids section content defaults to ELF_T_BYTE (i.e. unsigned char), no translation occurs. This results in incorrect patched values when cross-compiling to non-native endianness, and can manifest as kernel Oops and test failures which are difficult to troubleshoot [1]. Explicitly set the type of patched data to ELF_T_WORD, the architecture- neutral ELF type corresponding to the u32 BTF IDs. This enables libelf to transparently perform any needed endian conversions. Fixes: fbbb68de80a4 ("bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object") Cc: stable@vger.kernel.org # v5.10+ Cc: Frank Eigler <fche@redhat.com> Cc: Mark Wielaard <mark@klomp.org> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Yonghong Song <yhs@fb.com> Link: [1] https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_PykcsoeA@mail.gmail.com/ Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> Acked-by: Jiri Olsa <jolsa@redhat.com> --- v1 -> v2: * add context and elaborate on commit message per request * include ACK from Jiri Olsa --- tools/bpf/resolve_btfids/main.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c index d636643ddd35..f32c059fbfb4 100644 --- a/tools/bpf/resolve_btfids/main.c +++ b/tools/bpf/resolve_btfids/main.c @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj) if (sets_patch(obj)) return -1; + /* Set type to ensure endian translation occurs. */ + obj->efile.idlist->d_type = ELF_T_WORD; + elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY); err = elf_update(obj->efile.elf, ELF_C_WRITE); -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH bpf v2] bpf: fix libelf endian handling in resolv_btfids 2021-06-18 6:14 ` [PATCH bpf v2] " Tony Ambardar @ 2021-06-18 15:10 ` patchwork-bot+netdevbpf 0 siblings, 0 replies; 12+ messages in thread From: patchwork-bot+netdevbpf @ 2021-06-18 15:10 UTC (permalink / raw) To: Tony Ambardar Cc: daniel, ast, andrii, Tony.Ambardar, bpf, netdev, linux-mips, stable, fche, mark, jolsa, yhs, jolsa Hello: This patch was applied to bpf/bpf.git (refs/heads/master): On Thu, 17 Jun 2021 23:14:04 -0700 you wrote: > The vmlinux ".BTF_ids" ELF section is declared in btf_ids.h to hold a list > of zero-filled BTF IDs, which is then patched at link-time with correct > values by resolv_btfids. The section is flagged as "allocable" to preclude > compression, but notably the section contents (BTF IDs) are untyped. > > When patching the BTF IDs, resolve_btfids writes in host-native endianness > and relies on libelf for any required translation on reading and updating > vmlinux. However, since the type of the .BTF_ids section content defaults > to ELF_T_BYTE (i.e. unsigned char), no translation occurs. This results in > incorrect patched values when cross-compiling to non-native endianness, > and can manifest as kernel Oops and test failures which are difficult to > troubleshoot [1]. > > [...] Here is the summary with links: - [bpf,v2] bpf: fix libelf endian handling in resolv_btfids https://git.kernel.org/bpf/bpf/c/61e8aeda9398 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-06-18 15:10 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-16 9:25 [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids Tony Ambardar 2021-06-16 15:56 ` Yonghong Song 2021-06-16 16:38 ` Jiri Olsa 2021-06-16 22:09 ` Tony Ambardar 2021-06-17 9:10 ` Jiri Olsa 2021-06-17 10:44 ` Mark Wielaard 2021-06-16 22:28 ` Mark Wielaard 2021-06-17 9:02 ` Jiri Olsa 2021-06-17 11:22 ` Daniel Borkmann 2021-06-18 0:20 ` Tony Ambardar 2021-06-18 6:14 ` [PATCH bpf v2] " Tony Ambardar 2021-06-18 15:10 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.