On Thu, Mar 23, 2023 at 6: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.
> 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)
> 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)
Yep, can confirm this patch does indeed appear to fix the issue. There
are no longer any valgrind errors for me.
>
> 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)
>
