Re: [PATCH dwarves v1] btf_encoder: handle .BTF_ids section endianness when cross-compiling

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-11-22 at 16:11 +0100, Jiri Olsa wrote:
> On Thu, Nov 21, 2024 at 11:02:18PM -0800, Eduard Zingerman wrote:
> > btf_encoder__tag_kfuncs() reads .BTF_ids section to identify a set of
> > kfuncs present in the ELF being processed. This section consists of
> > records of the following shape:
> > 
> >   struct btf_id_and_flag {
> >       uint32_t id;
> >       uint32_t flags;
> >   };
> 
> it contains pairs like above and also just id arrays with no flags, but
> that does not matter for the patch functionality, because you swap by
> u32 values anyway

Right, I'll update the description, thank you.

[...]

> > @@ -1847,11 +1848,47 @@ static int btf_encoder__tag_kfunc(struct btf_encoder *encoder, struct gobuffer *
> >  	return 0;
> >  }
> >  
> > +/* If byte order of 'elf' differs from current byte order, convert the data->d_buf.
> > + * ELF file is opened in a readonly mode, so data->d_buf cannot be modified in place.
> > + * Instead, allocate a new buffer if modification is necessary.
> > + */
> > +static int convert_idlist_endianness(Elf *elf, Elf_Data *data, bool *copied)
> > +{
> > +	int byteorder, i;
> > +	char *elf_ident;
> > +	uint32_t *tmp;
> > +
> > +	*copied = false;
> > +	elf_ident = elf_getident(elf, NULL);
> > +	if (elf_ident == NULL) {
> > +		fprintf(stderr, "Cannot get ELF identification from header\n");
> > +		return -EINVAL;
> > +	}
> > +	byteorder = elf_ident[EI_DATA];
> > +	if ((BYTE_ORDER == LITTLE_ENDIAN && byteorder == ELFDATA2LSB)
> > +	    || (BYTE_ORDER == BIG_ENDIAN && byteorder == ELFDATA2MSB))
> > +		return 0;
> > +	tmp = malloc(data->d_size);
> > +	if (tmp == NULL) {
> > +		fprintf(stderr, "Cannot allocate %lu bytes of memory\n", data->d_size);
> > +		return -ENOMEM;
> > +	}
> > +	memcpy(tmp, data->d_buf, data->d_size);
> > +	data->d_buf = tmp;
> 
> will the original data->d_buf be leaked? are we allowed to assign d_buf like that? ;-)

Well, before sending I checked using address sanitizer, and it did not complain.
As far as I understand elfutils elf_getdata.c / elf_end.c [0]:
- elf_getdata() allocates memory for full section (elf_getdata.c:333),
  before setting d_buf field of Elf_Data;
- elf_end() frees memory for full section (elf_end.c:174).

So I assumed that this is hacky but not that bad.
Given that current patch depends on implementation details it is
probably better to switch to one of the alternatives:
a. allocate new Elf_Data object using elf_newdata() API;
b. just allocate a fake instance of Elf_Data on stack in btf_encoder__tag_kfuncs().

(a) seems to be an Ok option, wdyt?

[0] b2f225d6bff8 ("Consolidate and add files to clean target variables")
    git://sourceware.org/git/elfutils.git

[...]

> >  	if (fd != -1)
> > diff --git a/lib/bpf b/lib/bpf
> > index 09b9e83..caa17bd 160000
> > --- a/lib/bpf
> > +++ b/lib/bpf
> > @@ -1 +1 @@
> > -Subproject commit 09b9e83102eb8ab9e540d36b4559c55f3bcdb95d
> > +Subproject commit caa17bdcbfc58e68eaf4d017c058e6577606bf56
> 
> I think this should not be part of the patch

Sorry, didn't notice this thing.