Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids

[Jiri Olsa <jolsa@redhat.com>]

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