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.
