On Thu, 2023-03-23 at 09:36 -0700, Andrii Nakryiko wrote:
> On Thu, Mar 23, 2023 at 5:57 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Wed, 2023-03-22 at 22:18 -0700, Andrii Nakryiko wrote:
> > > On Wed, Mar 22, 2023 at 9:26 PM James Hilliard
> > > <james.hilliard1@xxxxxxxxx> wrote:
> > > >
> > > > I'm seeing this gen object error with gcc does not occur in llvm for a
> > > > bpf test(which uses both linked_maps1.c and linked_maps2.c) in
> > > > bpf-next.
> > > >
> > > > I presume there is a bug in GCC which is triggering a double free bug in libbpf.
> > > >
> > >
> > > Could you somehow share .o files for linked_maps1.c and
> > > linked_maps2.c, so I can try to repro locally?
> >
> > Looking at the stack trace, second free is reported here:
> >
> > void bpf_linker__free(struct bpf_linker *linker)
> > {
> > ...
> > for (i = 1; i < linker->sec_cnt; i++) {
> > ...
> > free(sec->sec_name);
> > here --> free(sec->raw_data);
> > free(sec->sec_vars);
> > ...
> > }
> > ...
> > }
> >
> > And first free is reported here:
> >
> > static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src_sec *src)
> > {
> > ...
> > dst_align = dst->shdr->sh_addralign;
> > src_align = src->shdr->sh_addralign;
> > ...
> >
> > dst_align_sz = (dst->sec_sz + dst_align - 1) / dst_align * dst_align;
> >
> > /* no need to re-align final size */
> > dst_final_sz = dst_align_sz + src->shdr->sh_size;
> >
> > if (src->shdr->sh_type != SHT_NOBITS) {
> > here --> tmp = realloc(dst->raw_data, dst_final_sz);
> > if (!tmp)
> > return -ENOMEM;
> > dst->raw_data = tmp;
> > ...
> > }
> > ...
> > }
> >
> > The documentation says that `realloc(ptr, 0)` frees `ptr`.
> > So, I assume that issue is caused by handling of empty sections.
>
> yep, thanks for repro steps! It's a quite interesting behavior. There
> are two reallocs involved:
>
> First, dst->raw_data is NULL, dst_final_sz is 0, realloc succeeds and
> returns non-NULL pointer (which according to documentation can be
> freed with free()). All good.
>
> Second one, for second file, we have non-NULL dst->raw_data returned
> from previous realloc(), we pass it to realloc() with dst_final_sz
> still 0. But *NOW* we get NULL as a return (and original special
> pointer "helpfully" freed for us). This we handle as -ENOMEM and exit.
>
> Amazingly non-error-prone behavior, of course.
>
> > This is easy to test using object files produced by LLVM:
> >
> > $ touch empty
> > $ llvm-objcopy --add-section .foobar=empty linked_maps1.bpf.o
> > $ llvm-objcopy --add-section .foobar=empty linked_maps2.bpf.o
> > $ bpftool --debug gen object linked_maps.linked1.o linked_maps1.bpf.o linked_maps2.bpf.o
> > libbpf: linker: adding object file 'linked_maps1.bpf.o'...
> > libbpf: linker: adding object file 'linked_maps2.bpf.o'...
> > Error: failed to link 'linked_maps2.bpf.o': Cannot allocate memory (12)
> > free(): double free detected in tcache 2
> > Aborted (core dumped)
> >
> > The valgrind output also matches the one attached to the original email.
> > Something like below fixes it:
> >
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index d7069780984a..ff3833e55c55 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -1113,7 +1113,7 @@ static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst, struct src
> > /* no need to re-align final size */
> > dst_final_sz = dst_align_sz + src->shdr->sh_size;
> >
> > - if (src->shdr->sh_type != SHT_NOBITS) {
> > + if (dst_final_sz != 0 && src->shdr->sh_type != SHT_NOBITS) {
> > tmp = realloc(dst->raw_data, dst_final_sz);
> > if (!tmp)
>
> let's maybe document this quirk instead of preventing realloc() call:
>
> /* comment here explaining the quirks of realloc() API and it's
> inconsistent runtime behavior */
> if (!tmp && dst_final_sz > 0)
> return -ENOMEM;
>
> Eduard, are you going to send a proper patch for this? Thanks!
I finally got back to this and have a question which I should have
asked on Thursday, sorry. Why do you want to preserve a call to
realloc() when dst_final_sz is 0?
Suppose we have N files all merging and having an empty section,
then dst->raw_data would constantly flip-flop between 2 states:
1. special pointer to memory of size zero;
2. NULL.
This shouldn't cause any issues but is kinda weird. If realloc() is
avoided the dst->raw_data would preserve it's original state (set to
zero by add_dst_sec()).
>
> > return -ENOMEM;
> >
> >
> > BPF selftests are passing for me with this change,
> > objcopy-based reproducer no longer reports error.
> > WDYT?
> >
> > James, could you please test this patch with bpf-gcc?
> > (you will have to re-compile libbpf and bpftool,
> > I had to separately do `make -C tools/bpf/bpftool`
> > before re-building selftests for some reason)
> >
> > Thanks,
> > Eduard
> >
> > >
> > > > GCC gen object failure:
> > > >
> > > > ==2125110== Command:
> > > > /home/buildroot/bpf-next/tools/testing/selftests/bpf/tools/sbin/bpftool
> > > > --debug gen object
> > > > /home/buildroot/bpf-next/tools/testing/selftests/bpf/bpf_gcc/linked_maps.linked1.o
> > > > /home/buildroot/bpf-next/tools/testing/selftests/bpf/bpf_gcc/linked_maps1.bpf.o
> > > > /home/buildroot/bpf-next/tools/testing/selftests/bpf/bpf_gcc/linked_maps2.bpf.o
> > > > ==2125110==
> > > > libbpf: linker: adding object file
> > > > '/home/buildroot/bpf-next/tools/testing/selftests/bpf/bpf_gcc/linked_maps1.bpf.o'...
> > > > libbpf: linker: adding object file
> > > > '/home/buildroot/bpf-next/tools/testing/selftests/bpf/bpf_gcc/linked_maps2.bpf.o'...
> > > > Error: failed to link
> > > > '/home/buildroot/bpf-next/tools/testing/selftests/bpf/bpf_gcc/linked_maps2.bpf.o':
> > > > Cannot allocate memory (12)
> > > > ==2125110== Invalid free() / delete / delete[] / realloc()
> > > > ==2125110== at 0x484B0C4: free (vg_replace_malloc.c:884)
> > > > ==2125110== by 0x17F8AB: bpf_linker__free (linker.c:204)
> > > > ==2125110== by 0x12833C: do_object (gen.c:1608)
> > > > ==2125110== by 0x12CDAB: cmd_select (main.c:206)
> > > > ==2125110== by 0x129B53: do_gen (gen.c:2332)
> > > > ==2125110== by 0x12CDAB: cmd_select (main.c:206)
> > > > ==2125110== by 0x12DB9E: main (main.c:539)
> > > > ==2125110== Address 0xda4b420 is 0 bytes after a block of size 0 free'd
> > > > ==2125110== at 0x484B027: free (vg_replace_malloc.c:883)
> > > > ==2125110== by 0x484D6F8: realloc (vg_replace_malloc.c:1451)
> > > > ==2125110== by 0x181FA3: extend_sec (linker.c:1117)
> > > > ==2125110== by 0x182326: linker_append_sec_data (linker.c:1201)
> > > > ==2125110== by 0x1803DC: bpf_linker__add_file (linker.c:453)
> > > > ==2125110== by 0x12829E: do_object (gen.c:1593)
> > > > ==2125110== by 0x12CDAB: cmd_select (main.c:206)
> > > > ==2125110== by 0x129B53: do_gen (gen.c:2332)
> > > > ==2125110== by 0x12CDAB: cmd_select (main.c:206)
> > > > ==2125110== by 0x12DB9E: main (main.c:539)
> > > > ==2125110== Block was alloc'd at
> > > > ==2125110== at 0x484876A: malloc (vg_replace_malloc.c:392)
> > > > ==2125110== by 0x484D6EB: realloc (vg_replace_malloc.c:1451)
> > > > ==2125110== by 0x181FA3: extend_sec (linker.c:1117)
> > > > ==2125110== by 0x182326: linker_append_sec_data (linker.c:1201)
> > > > ==2125110== by 0x1803DC: bpf_linker__add_file (linker.c:453)
> > > > ==2125110== by 0x12829E: do_object (gen.c:1593)
> > > > ==2125110== by 0x12CDAB: cmd_select (main.c:206)
> > > > ==2125110== by 0x129B53: do_gen (gen.c:2332)
> > > > ==2125110== by 0x12CDAB: cmd_select (main.c:206)
> > > > ==2125110== by 0x12DB9E: main (main.c:539)
> >
