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)
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)
