On Thu, Mar 13, 2025 at 7:30 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Thu, Mar 13, 2025 at 12:23:10PM +0800, Hengqi Chen wrote:
>
> SNIP
>
> > > > > > tools/lib/bpf/elf.c | 32 ++++++++++++++++++++++++--------
> > > > > > 1 file changed, 24 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> > > > > > index 823f83ad819c..9b561c8d1eec 100644
> > > > > > --- a/tools/lib/bpf/elf.c
> > > > > > +++ b/tools/lib/bpf/elf.c
> > > > > > @@ -260,13 +260,29 @@ static bool symbol_match(struct elf_sym_iter *iter, int sh_type, struct elf_sym
> > > > > > * for shared libs) into file offset, which is what kernel is expecting
> > > > > > * for uprobe/uretprobe attachment.
> > > > > > * See Documentation/trace/uprobetracer.rst for more details. This is done
> > > > > > - * by looking up symbol's containing section's header and using iter's virtual
> > > > > > - * address (sh_addr) and corresponding file offset (sh_offset) to transform
> > > > > > + * by looking up symbol's containing program header and using its virtual
> > > > > > + * address (p_vaddr) and corresponding file offset (p_offset) to transform
> > > > > > * sym.st_value (virtual address) into desired final file offset.
> > > > > > */
> > > > > > -static unsigned long elf_sym_offset(struct elf_sym *sym)
> > > > > > +static unsigned long elf_sym_offset(Elf *elf, struct elf_sym *sym)
> > > > > > {
> > > > > > - return sym->sym.st_value - sym->sh.sh_addr + sym->sh.sh_offset;
> > > > > > + size_t nhdrs, i;
> > > > > > + GElf_Phdr phdr;
> > > > > > +
> > > > > > + if (elf_getphdrnum(elf, &nhdrs))
> > > > > > + return -1;
> > > > > > +
> > > > > > + for (i = 0; i < nhdrs; i++) {
> > > > > > + if (!gelf_getphdr(elf, (int)i, &phdr))
> > > > > > + continue;
> > > > > > + if (phdr.p_type != PT_LOAD || !(phdr.p_flags & PF_X))
> > > > > > + continue;
> > > > > > + if (sym->sym.st_value >= phdr.p_vaddr &&
> > > > > > + sym->sym.st_value < (phdr.p_vaddr + phdr.p_memsz))
> > > > > > + return sym->sym.st_value - phdr.p_vaddr + phdr.p_offset;
> > >
> > > Hengqi,
> > >
> > > Can you please provide an example where existing code doesn't work? I think that
> > >
> > > sym->sym.st_value - sym->sh.sh_addr + sym->sh.sh_offset
> > >
> > > and
> > >
> > > sym->sym.st_value - phdr.p_vaddr + phdr.p_offset
> > >
> > >
> > > Should result in the same, and we don't need to search for a matching
> > > segment if we have an ELF symbol and its section. But maybe I'm
> > > mistaken, so can you please elaborate a bit?
> >
> > The binary ([0]) provided in the issue shows some counterexamples.
> > I could't find an authoritative documentation describing this though.
> > A modified version ([1]) of this patch could pass the CI now.
>
> yes, I tried that binary and it gives me different offsets
>
> IIUC the symbol seems to be from .eh_frame_hdr (odd?) while the new logic
> base it on offset of .text section.. I'm still not following that binary
> layout completely.. will try to check on that more later today
>
Yep, something is off with the way that this ELF file is linked.
Symbols information is just wrong.
In sections he have:
[Nr] Name Type Address Off Size
ES Flg Lk Inf Al
[ 0] NULL 0000000000000000 000000
000000 00 0 0 0
[ 1] .gnu.version VERSYM 0000000000299b38 299b38
03774a 02 A 5 0 2
[ 2] .gnu.version_r VERNEED 00000000002d1284 2d1284
000340 00 A 6 12 4
[ 3] .gnu.hash GNU_HASH 00000000002d15c8 2d15c8
0c9538 00 A 5 0 8
[ 4] .dynamic DYNAMIC 000000000b29fb10 b29db10
000380 10 WA 6 0 8
[ 5] .dynsym DYNSYM 00000000d7b03070 b2b4070
299778 18 A 6 1 8
[ 6] .dynstr STRTAB 000000000039ab00 39ab00
17c8e7e 00 A 0 0 1
[ 7] .rela.dyn RELA 0000000001b63980 1b63980
513630 18 A 5 0 8
[ 8] .rela.plt RELA 0000000002076fb0 2076fb0
000288 18 AI 5 26 8
[ 9] .rodata PROGBITS 0000000002078000 2078000
a30306 00 AMS 0 0 4096
[10] .gcc_except_table PROGBITS 0000000002aa8308 2aa8308
3cd368 00 A 0 0 4
[11] protodesc_cold PROGBITS 0000000002e75670 2e75670
007400 00 A 0 0 16
[12] .stapsdt.base PROGBITS 0000000002e7ca70 2e7ca70
000001 00 A 0 0 1
[13] .eh_frame_hdr PROGBITS 0000000002e7ca74 2e7ca74
0f35bc 00 A 0 0 4
[14] .eh_frame PROGBITS 0000000002f70030 2f70030
66f6a4 00 A 0 0 8
[15] .text PROGBITS 00000000035e0700 35df700
7a9de55 00 AX 0 0 64
[16] .init PROGBITS 000000000b07e558 b07d558
00001b 00 AX 0 0 4
[17] .fini PROGBITS 000000000b07e574 b07d574
00000d 00 AX 0 0 4
...
Symbol itself says:
Symbol table '.dynsym' contains 113573 entries:
Num: Value Size Type Bind Vis Ndx Name
...
109776: 00000000081658d0 4132 FUNC GLOBAL DEFAULT 13
_ZN7cluster15topics_frontend13create_topicsE17fragmented_vectorINS_37custom_assignable_topic_configurationELm18446744073709551615EENSt3__16chrono10time_pointIN7seastar12lowres_clockENS5_8durationIxNS4_5ratioILl1ELl1000000000EEEEEEE
...
Note how the symbol points to section #13, which is .eh_frame_hdr. But
value (virtual address) 00000000081658d0 actually belongs to section
#15 (.text):
[15] .text PROGBITS 00000000035e0700 35df700
7a9de55 00 AX 0 0 64
So symbol information has section index off by 2. And this seems to be
the case for lots of symbols, they all point to section #13.
Libbpf's logic is correct, as long as symbol information is correct.
If that index was 15, we'd calculate that 0x1000 additional offset.
So let's figure out why ELF is invalid instead of trying to "fix"
libbpf's logic, which is correct and much faster than linearly
searching through segments. Could it be that that binary was modified
post final linking to add custom sections before .text (like that
protodesc_cold)?
> jirka
>
> >
> > [0]: https://github.com/libbpf/libbpf-rs/issues/1110#issuecomment-2699221802
> > [1]: https://github.com/kernel-patches/bpf/pull/8647
> >
> > >
> > > > > > + }
> > > > > > +
> > > > > > + return -1;
> > > > > > }
> > > > > >
> > > > > > /* Find offset of function name in the provided ELF object. "binary_path" is
> > > > > > @@ -329,7 +345,7 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
> > > > > >
> > > > > > if (ret > 0) {
> > > > > > /* handle multiple matches */
> > > > > > - if (elf_sym_offset(sym) == ret) {
> > > > > > + if (elf_sym_offset(elf, sym) == ret) {
> > > > > > /* same offset, no problem */
> > > > > > continue;
> > > > > > } else if (last_bind != STB_WEAK && cur_bind != STB_WEAK) {
> > > > >
> > > > > [...]
> > > > >
> >
