* [PATCH] libbpf: Fix uprobe offset calculation
@ 2025-03-07 14:01 Hengqi Chen
2025-03-08 6:48 ` Yonghong Song
0 siblings, 1 reply; 7+ messages in thread
From: Hengqi Chen @ 2025-03-07 14:01 UTC (permalink / raw)
To: bpf, andrii, eddyz87, deso; +Cc: hengqi.chen
As reported on libbpf-rs issue([0]), the current implementation
may resolve symbol to a wrong offset and thus missing uprobe
event. Calculate the symbol offset from program header instead.
See the BCC implementation (which in turn used by bpftrace) and
the spec ([1]) for references.
[0]: https://github.com/libbpf/libbpf-rs/issues/1110
[1]: https://refspecs.linuxfoundation.org/elf/
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
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;
+ }
+
+ 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) {
@@ -346,7 +362,7 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
}
}
- ret = elf_sym_offset(sym);
+ ret = elf_sym_offset(elf, sym);
last_bind = cur_bind;
}
if (ret > 0)
@@ -445,7 +461,7 @@ int elf_resolve_syms_offsets(const char *binary_path, int cnt,
goto out;
while ((sym = elf_sym_iter_next(&iter))) {
- unsigned long sym_offset = elf_sym_offset(sym);
+ unsigned long sym_offset = elf_sym_offset(elf_fd.elf, sym);
int bind = GELF_ST_BIND(sym->sym.st_info);
struct symbol *found, tmp = {
.name = sym->name,
@@ -534,7 +550,7 @@ int elf_resolve_pattern_offsets(const char *binary_path, const char *pattern,
if (err)
goto out;
- offsets[cnt++] = elf_sym_offset(sym);
+ offsets[cnt++] = elf_sym_offset(elf_fd.elf, sym);
}
/* If we found anything in the first symbol section,
--
2.43.5
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] libbpf: Fix uprobe offset calculation
2025-03-07 14:01 [PATCH] libbpf: Fix uprobe offset calculation Hengqi Chen
@ 2025-03-08 6:48 ` Yonghong Song
2025-03-11 5:16 ` Hengqi Chen
0 siblings, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2025-03-08 6:48 UTC (permalink / raw)
To: Hengqi Chen, bpf, andrii, eddyz87, deso
On 3/7/25 6:01 AM, Hengqi Chen wrote:
> As reported on libbpf-rs issue([0]), the current implementation
> may resolve symbol to a wrong offset and thus missing uprobe
> event. Calculate the symbol offset from program header instead.
> See the BCC implementation (which in turn used by bpftrace) and
> the spec ([1]) for references.
>
> [0]: https://github.com/libbpf/libbpf-rs/issues/1110
> [1]: https://refspecs.linuxfoundation.org/elf/
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
Hengqi,
There are some test failures in the CI. For example,
https://github.com/kernel-patches/bpf/actions/runs/13725803997/job/38392284640?pr=8631
Please take a look.
Your below elf_sym_offset change matches some bcc implementation, but
maybe maybe this is only under certain condition?
Also, it would be great if you can add detailed description in commit message
about what is the problem and why a different approach is necessary to
fix the issue.
> ---
> 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;
> + }
> +
> + 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) {
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] libbpf: Fix uprobe offset calculation
2025-03-08 6:48 ` Yonghong Song
@ 2025-03-11 5:16 ` Hengqi Chen
2025-03-12 18:47 ` Andrii Nakryiko
0 siblings, 1 reply; 7+ messages in thread
From: Hengqi Chen @ 2025-03-11 5:16 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, andrii, eddyz87, deso
Hi Yonghong,
On Sat, Mar 8, 2025 at 2:48 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
>
> On 3/7/25 6:01 AM, Hengqi Chen wrote:
> > As reported on libbpf-rs issue([0]), the current implementation
> > may resolve symbol to a wrong offset and thus missing uprobe
> > event. Calculate the symbol offset from program header instead.
> > See the BCC implementation (which in turn used by bpftrace) and
> > the spec ([1]) for references.
> >
> > [0]: https://github.com/libbpf/libbpf-rs/issues/1110
> > [1]: https://refspecs.linuxfoundation.org/elf/
> >
> > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>
> Hengqi,
>
> There are some test failures in the CI. For example,
> https://github.com/kernel-patches/bpf/actions/runs/13725803997/job/38392284640?pr=8631
>
Yes, I've received an email from BPF CI.
It seems like the uprobe multi testcase is unhappy with this change.
> Please take a look.
> Your below elf_sym_offset change matches some bcc implementation, but
> maybe maybe this is only under certain condition?
>
Remove the `phdr.p_flags & PF_X` check fix the issue. Need more investigation.
> Also, it would be great if you can add detailed description in commit message
> about what is the problem and why a different approach is necessary to
> fix the issue.
>
Will do. Thanks.
> > ---
> > 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;
> > + }
> > +
> > + 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) {
>
> [...]
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] libbpf: Fix uprobe offset calculation
2025-03-11 5:16 ` Hengqi Chen
@ 2025-03-12 18:47 ` Andrii Nakryiko
2025-03-13 4:23 ` Hengqi Chen
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2025-03-12 18:47 UTC (permalink / raw)
To: Hengqi Chen; +Cc: Yonghong Song, bpf, andrii, eddyz87, deso
On Mon, Mar 10, 2025 at 10:16 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Hi Yonghong,
>
> On Sat, Mar 8, 2025 at 2:48 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >
> >
> >
> > On 3/7/25 6:01 AM, Hengqi Chen wrote:
> > > As reported on libbpf-rs issue([0]), the current implementation
> > > may resolve symbol to a wrong offset and thus missing uprobe
> > > event. Calculate the symbol offset from program header instead.
> > > See the BCC implementation (which in turn used by bpftrace) and
> > > the spec ([1]) for references.
> > >
> > > [0]: https://github.com/libbpf/libbpf-rs/issues/1110
> > > [1]: https://refspecs.linuxfoundation.org/elf/
> > >
> > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> >
> > Hengqi,
> >
> > There are some test failures in the CI. For example,
> > https://github.com/kernel-patches/bpf/actions/runs/13725803997/job/38392284640?pr=8631
> >
>
> Yes, I've received an email from BPF CI.
> It seems like the uprobe multi testcase is unhappy with this change.
>
> > Please take a look.
> > Your below elf_sym_offset change matches some bcc implementation, but
> > maybe maybe this is only under certain condition?
> >
>
> Remove the `phdr.p_flags & PF_X` check fix the issue. Need more investigation.
>
> > Also, it would be great if you can add detailed description in commit message
> > about what is the problem and why a different approach is necessary to
> > fix the issue.
> >
>
> Will do. Thanks.
>
> > > ---
> > > 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?
> > > + }
> > > +
> > > + 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) {
> >
> > [...]
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] libbpf: Fix uprobe offset calculation
2025-03-12 18:47 ` Andrii Nakryiko
@ 2025-03-13 4:23 ` Hengqi Chen
2025-03-13 14:30 ` Jiri Olsa
0 siblings, 1 reply; 7+ messages in thread
From: Hengqi Chen @ 2025-03-13 4:23 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: Yonghong Song, bpf, andrii, eddyz87, deso
Hi Andrii,
On Thu, Mar 13, 2025 at 2:47 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Mar 10, 2025 at 10:16 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
> >
> > Hi Yonghong,
> >
> > On Sat, Mar 8, 2025 at 2:48 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> > >
> > >
> > >
> > > On 3/7/25 6:01 AM, Hengqi Chen wrote:
> > > > As reported on libbpf-rs issue([0]), the current implementation
> > > > may resolve symbol to a wrong offset and thus missing uprobe
> > > > event. Calculate the symbol offset from program header instead.
> > > > See the BCC implementation (which in turn used by bpftrace) and
> > > > the spec ([1]) for references.
> > > >
> > > > [0]: https://github.com/libbpf/libbpf-rs/issues/1110
> > > > [1]: https://refspecs.linuxfoundation.org/elf/
> > > >
> > > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> > >
> > > Hengqi,
> > >
> > > There are some test failures in the CI. For example,
> > > https://github.com/kernel-patches/bpf/actions/runs/13725803997/job/38392284640?pr=8631
> > >
> >
> > Yes, I've received an email from BPF CI.
> > It seems like the uprobe multi testcase is unhappy with this change.
> >
> > > Please take a look.
> > > Your below elf_sym_offset change matches some bcc implementation, but
> > > maybe maybe this is only under certain condition?
> > >
> >
> > Remove the `phdr.p_flags & PF_X` check fix the issue. Need more investigation.
> >
> > > Also, it would be great if you can add detailed description in commit message
> > > about what is the problem and why a different approach is necessary to
> > > fix the issue.
> > >
> >
> > Will do. Thanks.
> >
> > > > ---
> > > > 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.
[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) {
> > >
> > > [...]
> > >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] libbpf: Fix uprobe offset calculation
2025-03-13 4:23 ` Hengqi Chen
@ 2025-03-13 14:30 ` Jiri Olsa
2025-03-13 21:11 ` Andrii Nakryiko
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Olsa @ 2025-03-13 14:30 UTC (permalink / raw)
To: Hengqi Chen; +Cc: Andrii Nakryiko, Yonghong Song, bpf, andrii, eddyz87, deso
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
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) {
> > > >
> > > > [...]
> > > >
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] libbpf: Fix uprobe offset calculation
2025-03-13 14:30 ` Jiri Olsa
@ 2025-03-13 21:11 ` Andrii Nakryiko
0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2025-03-13 21:11 UTC (permalink / raw)
To: Jiri Olsa; +Cc: Hengqi Chen, Yonghong Song, bpf, andrii, eddyz87, deso
On Thu, Mar 13, 2025 at 7:30 AM Jiri Olsa <olsajiri@gmail.com> 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) {
> > > > >
> > > > > [...]
> > > > >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-03-13 21:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-07 14:01 [PATCH] libbpf: Fix uprobe offset calculation Hengqi Chen
2025-03-08 6:48 ` Yonghong Song
2025-03-11 5:16 ` Hengqi Chen
2025-03-12 18:47 ` Andrii Nakryiko
2025-03-13 4:23 ` Hengqi Chen
2025-03-13 14:30 ` Jiri Olsa
2025-03-13 21:11 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox