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

From: Jiri Olsa <jolsa@redhat.com>
To: Mark Wielaard <mark@klomp.org>
Cc: Yonghong Song <yhs@fb.com>,
	Tony Ambardar <tony.ambardar@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	bpf@vger.kernel.org, netdev@vger.kernel.org,
	stable@vger.kernel.org, Jiri Olsa <jolsa@kernel.org>,
	Frank Eigler <fche@redhat.com>
Subject: Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids
Date: Thu, 17 Jun 2021 11:02:21 +0200	[thread overview]
Message-ID: <YMsPnaV798ICuMbv@krava> (raw)
In-Reply-To: <YMp68Dlqwu+wuHV9@wildebeest.org>

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