On Mon, Mar 27, 2023 at 2:48 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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?
Because it's supposed to work correctly. Why is it weird? realloc API
is supposed to work with zeros, so I'd rather have one code path that
handles all the cases instead of avoiding calling realloc().
>
> 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)
> > >
>
