* [PATCH bpf-next v2] bpf: make the attach target more accurate
@ 2025-07-08 7:21 Menglong Dong
2025-07-08 12:17 ` Menglong Dong
2025-07-09 8:43 ` Jiri Olsa
0 siblings, 2 replies; 6+ messages in thread
From: Menglong Dong @ 2025-07-08 7:21 UTC (permalink / raw)
To: ast, daniel
Cc: john.fastabend, andrii, martin.lau, eddyz87, song, yonghong.song,
kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, Menglong Dong
For now, we lookup the address of the attach target in
bpf_check_attach_target() with find_kallsyms_symbol_value or
kallsyms_lookup_name, which is not accurate in some cases.
For example, we want to attach to the target "t_next", but there are
multiple symbols with the name "t_next" exist in the kallsyms. The one
that kallsyms_lookup_name() returned may have no ftrace record, which
makes the attach target not available. So we want the one that has ftrace
record to be returned.
Meanwhile, there may be multiple symbols with the name "t_next" in ftrace
record. In this case, the attach target is ambiguous, so the attach should
fail.
Introduce the function bpf_lookup_attach_addr() to do the address lookup,
which is able to solve this problem.
Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
v2:
- Lookup both vmlinux and modules symbols when mod is NULL, just like
kallsyms_lookup_name().
If the btf is not a modules, shouldn't we lookup on the vmlinux only?
I'm not sure if we should keep the same logic with
kallsyms_lookup_name().
- Return the kernel symbol that don't have ftrace location if the symbols
with ftrace location are not available
---
kernel/bpf/verifier.c | 77 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 72 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 53007182b46b..4bacd0abf207 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -23476,6 +23476,73 @@ static int check_non_sleepable_error_inject(u32 btf_id)
return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
}
+struct symbol_lookup_ctx {
+ const char *name;
+ unsigned long addr;
+ bool ftrace_addr;
+};
+
+static int symbol_callback(void *data, unsigned long addr)
+{
+ struct symbol_lookup_ctx *ctx = data;
+
+ ctx->addr = addr;
+ if (!ftrace_location(addr))
+ return 0;
+
+ if (ctx->ftrace_addr)
+ return -EADDRNOTAVAIL;
+ ctx->ftrace_addr = true;
+
+ return 0;
+}
+
+static int symbol_mod_callback(void *data, const char *name, unsigned long addr)
+{
+ if (strcmp(((struct symbol_lookup_ctx *)data)->name, name) != 0)
+ return 0;
+
+ return symbol_callback(data, addr);
+}
+
+/**
+ * bpf_lookup_attach_addr: Lookup address for a symbol
+ *
+ * @mod: kernel module to lookup the symbol, NULL means to lookup both vmlinux
+ * and modules symbols
+ * @sym: the symbol to resolve
+ * @addr: pointer to store the result
+ *
+ * Lookup the address of the symbol @sym. If multiple symbols with the name
+ * @sym exist, the one that has ftrace location is preferred. If more
+ * than 1 has ftrace location, -EADDRNOTAVAIL will be returned.
+ *
+ * Returns: 0 on success, -errno otherwise.
+ */
+static int bpf_lookup_attach_addr(const struct module *mod, const char *sym,
+ unsigned long *addr)
+{
+ struct symbol_lookup_ctx ctx = { .addr = 0, .name = sym };
+ const char *mod_name = NULL;
+ int err = 0;
+
+#ifdef CONFIG_MODULES
+ mod_name = mod ? mod->name : NULL;
+#endif
+ if (!mod_name)
+ err = kallsyms_on_each_match_symbol(symbol_callback, sym, &ctx);
+
+ if (!err && !ctx.addr)
+ err = module_kallsyms_on_each_symbol(mod_name, symbol_mod_callback,
+ &ctx);
+
+ if (!ctx.addr)
+ err = -ENOENT;
+ *addr = err ? 0 : ctx.addr;
+
+ return err;
+}
+
int bpf_check_attach_target(struct bpf_verifier_log *log,
const struct bpf_prog *prog,
const struct bpf_prog *tgt_prog,
@@ -23729,18 +23796,18 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
if (btf_is_module(btf)) {
mod = btf_try_get_module(btf);
if (mod)
- addr = find_kallsyms_symbol_value(mod, tname);
+ ret = bpf_lookup_attach_addr(mod, tname, &addr);
else
- addr = 0;
+ ret = -ENOENT;
} else {
- addr = kallsyms_lookup_name(tname);
+ ret = bpf_lookup_attach_addr(NULL, tname, &addr);
}
- if (!addr) {
+ if (ret) {
module_put(mod);
bpf_log(log,
"The address of function %s cannot be found\n",
tname);
- return -ENOENT;
+ return ret;
}
}
--
2.39.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2] bpf: make the attach target more accurate
2025-07-08 7:21 [PATCH bpf-next v2] bpf: make the attach target more accurate Menglong Dong
@ 2025-07-08 12:17 ` Menglong Dong
2025-07-09 8:43 ` Jiri Olsa
1 sibling, 0 replies; 6+ messages in thread
From: Menglong Dong @ 2025-07-08 12:17 UTC (permalink / raw)
To: ast, daniel
Cc: john.fastabend, andrii, martin.lau, eddyz87, song, yonghong.song,
kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, Menglong Dong
On Tue, Jul 8, 2025 at 3:23 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
>
> For now, we lookup the address of the attach target in
> bpf_check_attach_target() with find_kallsyms_symbol_value or
> kallsyms_lookup_name, which is not accurate in some cases.
>
> For example, we want to attach to the target "t_next", but there are
> multiple symbols with the name "t_next" exist in the kallsyms. The one
> that kallsyms_lookup_name() returned may have no ftrace record, which
> makes the attach target not available. So we want the one that has ftrace
> record to be returned.
>
> Meanwhile, there may be multiple symbols with the name "t_next" in ftrace
> record. In this case, the attach target is ambiguous, so the attach should
> fail.
>
> Introduce the function bpf_lookup_attach_addr() to do the address lookup,
> which is able to solve this problem.
>
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
> v2:
> - Lookup both vmlinux and modules symbols when mod is NULL, just like
> kallsyms_lookup_name().
>
> If the btf is not a modules, shouldn't we lookup on the vmlinux only?
> I'm not sure if we should keep the same logic with
> kallsyms_lookup_name().
>
> - Return the kernel symbol that don't have ftrace location if the symbols
> with ftrace location are not available
> ---
> kernel/bpf/verifier.c | 77 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 53007182b46b..4bacd0abf207 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -23476,6 +23476,73 @@ static int check_non_sleepable_error_inject(u32 btf_id)
> return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
> }
>
> +struct symbol_lookup_ctx {
> + const char *name;
> + unsigned long addr;
> + bool ftrace_addr;
> +};
> +
> +static int symbol_callback(void *data, unsigned long addr)
> +{
> + struct symbol_lookup_ctx *ctx = data;
> +
Oops, the logic here is wrong, it should be:
static int symbol_callback(void *data, unsigned long addr)
{
struct symbol_lookup_ctx *ctx = data;
if (!ctx->ftrace_addr)
ctx->addr = addr;
if (!ftrace_location(addr))
return 0;
if (ctx->ftrace_addr)
return -EADDRNOTAVAIL;
ctx->ftrace_addr = true;
return 0;
}
I'll send the V3 after more feedback :/
> + ctx->addr = addr;
> + if (!ftrace_location(addr))
> + return 0;
> +
> + if (ctx->ftrace_addr)
> + return -EADDRNOTAVAIL;
> + ctx->ftrace_addr = true;
> +
> + return 0;
> +}
> +
> +static int symbol_mod_callback(void *data, const char *name, unsigned long addr)
> +{
> + if (strcmp(((struct symbol_lookup_ctx *)data)->name, name) != 0)
> + return 0;
> +
> + return symbol_callback(data, addr);
> +}
> +
> +/**
> + * bpf_lookup_attach_addr: Lookup address for a symbol
> + *
> + * @mod: kernel module to lookup the symbol, NULL means to lookup both vmlinux
> + * and modules symbols
> + * @sym: the symbol to resolve
> + * @addr: pointer to store the result
> + *
> + * Lookup the address of the symbol @sym. If multiple symbols with the name
> + * @sym exist, the one that has ftrace location is preferred. If more
> + * than 1 has ftrace location, -EADDRNOTAVAIL will be returned.
> + *
> + * Returns: 0 on success, -errno otherwise.
> + */
> +static int bpf_lookup_attach_addr(const struct module *mod, const char *sym,
> + unsigned long *addr)
> +{
> + struct symbol_lookup_ctx ctx = { .addr = 0, .name = sym };
> + const char *mod_name = NULL;
> + int err = 0;
> +
> +#ifdef CONFIG_MODULES
> + mod_name = mod ? mod->name : NULL;
> +#endif
> + if (!mod_name)
> + err = kallsyms_on_each_match_symbol(symbol_callback, sym, &ctx);
> +
> + if (!err && !ctx.addr)
> + err = module_kallsyms_on_each_symbol(mod_name, symbol_mod_callback,
> + &ctx);
> +
> + if (!ctx.addr)
> + err = -ENOENT;
> + *addr = err ? 0 : ctx.addr;
> +
> + return err;
> +}
> +
> int bpf_check_attach_target(struct bpf_verifier_log *log,
> const struct bpf_prog *prog,
> const struct bpf_prog *tgt_prog,
> @@ -23729,18 +23796,18 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> if (btf_is_module(btf)) {
> mod = btf_try_get_module(btf);
> if (mod)
> - addr = find_kallsyms_symbol_value(mod, tname);
> + ret = bpf_lookup_attach_addr(mod, tname, &addr);
> else
> - addr = 0;
> + ret = -ENOENT;
> } else {
> - addr = kallsyms_lookup_name(tname);
> + ret = bpf_lookup_attach_addr(NULL, tname, &addr);
> }
> - if (!addr) {
> + if (ret) {
> module_put(mod);
> bpf_log(log,
> "The address of function %s cannot be found\n",
> tname);
> - return -ENOENT;
> + return ret;
> }
> }
>
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2] bpf: make the attach target more accurate
2025-07-08 7:21 [PATCH bpf-next v2] bpf: make the attach target more accurate Menglong Dong
2025-07-08 12:17 ` Menglong Dong
@ 2025-07-09 8:43 ` Jiri Olsa
2025-07-09 10:33 ` Menglong Dong
1 sibling, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2025-07-09 8:43 UTC (permalink / raw)
To: Menglong Dong
Cc: ast, daniel, john.fastabend, andrii, martin.lau, eddyz87, song,
yonghong.song, kpsingh, sdf, haoluo, bpf, linux-kernel,
Menglong Dong
On Tue, Jul 08, 2025 at 03:21:40PM +0800, Menglong Dong wrote:
> For now, we lookup the address of the attach target in
> bpf_check_attach_target() with find_kallsyms_symbol_value or
> kallsyms_lookup_name, which is not accurate in some cases.
>
> For example, we want to attach to the target "t_next", but there are
> multiple symbols with the name "t_next" exist in the kallsyms. The one
> that kallsyms_lookup_name() returned may have no ftrace record, which
> makes the attach target not available. So we want the one that has ftrace
> record to be returned.
>
> Meanwhile, there may be multiple symbols with the name "t_next" in ftrace
> record. In this case, the attach target is ambiguous, so the attach should
> fail.
could you reproduce this somehow (bpftrace/selftest) for some symbol?
I'd think pahole now filters all such symbols out of BTF and you need
BTF func record to load the program in the first place
jirka
>
> Introduce the function bpf_lookup_attach_addr() to do the address lookup,
> which is able to solve this problem.
>
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
> v2:
> - Lookup both vmlinux and modules symbols when mod is NULL, just like
> kallsyms_lookup_name().
>
> If the btf is not a modules, shouldn't we lookup on the vmlinux only?
> I'm not sure if we should keep the same logic with
> kallsyms_lookup_name().
>
> - Return the kernel symbol that don't have ftrace location if the symbols
> with ftrace location are not available
> ---
> kernel/bpf/verifier.c | 77 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 53007182b46b..4bacd0abf207 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -23476,6 +23476,73 @@ static int check_non_sleepable_error_inject(u32 btf_id)
> return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
> }
>
> +struct symbol_lookup_ctx {
> + const char *name;
> + unsigned long addr;
> + bool ftrace_addr;
> +};
> +
> +static int symbol_callback(void *data, unsigned long addr)
> +{
> + struct symbol_lookup_ctx *ctx = data;
> +
> + ctx->addr = addr;
> + if (!ftrace_location(addr))
> + return 0;
> +
> + if (ctx->ftrace_addr)
> + return -EADDRNOTAVAIL;
> + ctx->ftrace_addr = true;
> +
> + return 0;
> +}
> +
> +static int symbol_mod_callback(void *data, const char *name, unsigned long addr)
> +{
> + if (strcmp(((struct symbol_lookup_ctx *)data)->name, name) != 0)
> + return 0;
> +
> + return symbol_callback(data, addr);
> +}
> +
> +/**
> + * bpf_lookup_attach_addr: Lookup address for a symbol
> + *
> + * @mod: kernel module to lookup the symbol, NULL means to lookup both vmlinux
> + * and modules symbols
> + * @sym: the symbol to resolve
> + * @addr: pointer to store the result
> + *
> + * Lookup the address of the symbol @sym. If multiple symbols with the name
> + * @sym exist, the one that has ftrace location is preferred. If more
> + * than 1 has ftrace location, -EADDRNOTAVAIL will be returned.
> + *
> + * Returns: 0 on success, -errno otherwise.
> + */
> +static int bpf_lookup_attach_addr(const struct module *mod, const char *sym,
> + unsigned long *addr)
> +{
> + struct symbol_lookup_ctx ctx = { .addr = 0, .name = sym };
> + const char *mod_name = NULL;
> + int err = 0;
> +
> +#ifdef CONFIG_MODULES
> + mod_name = mod ? mod->name : NULL;
> +#endif
> + if (!mod_name)
> + err = kallsyms_on_each_match_symbol(symbol_callback, sym, &ctx);
> +
> + if (!err && !ctx.addr)
> + err = module_kallsyms_on_each_symbol(mod_name, symbol_mod_callback,
> + &ctx);
> +
> + if (!ctx.addr)
> + err = -ENOENT;
> + *addr = err ? 0 : ctx.addr;
> +
> + return err;
> +}
> +
> int bpf_check_attach_target(struct bpf_verifier_log *log,
> const struct bpf_prog *prog,
> const struct bpf_prog *tgt_prog,
> @@ -23729,18 +23796,18 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> if (btf_is_module(btf)) {
> mod = btf_try_get_module(btf);
> if (mod)
> - addr = find_kallsyms_symbol_value(mod, tname);
> + ret = bpf_lookup_attach_addr(mod, tname, &addr);
> else
> - addr = 0;
> + ret = -ENOENT;
> } else {
> - addr = kallsyms_lookup_name(tname);
> + ret = bpf_lookup_attach_addr(NULL, tname, &addr);
> }
> - if (!addr) {
> + if (ret) {
> module_put(mod);
> bpf_log(log,
> "The address of function %s cannot be found\n",
> tname);
> - return -ENOENT;
> + return ret;
> }
> }
>
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2] bpf: make the attach target more accurate
2025-07-09 8:43 ` Jiri Olsa
@ 2025-07-09 10:33 ` Menglong Dong
2025-07-09 12:34 ` Jiri Olsa
0 siblings, 1 reply; 6+ messages in thread
From: Menglong Dong @ 2025-07-09 10:33 UTC (permalink / raw)
To: Jiri Olsa
Cc: ast, daniel, john.fastabend, andrii, martin.lau, eddyz87, song,
yonghong.song, kpsingh, sdf, haoluo, bpf, linux-kernel,
Menglong Dong
On Wed, Jul 9, 2025 at 4:43 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Jul 08, 2025 at 03:21:40PM +0800, Menglong Dong wrote:
> > For now, we lookup the address of the attach target in
> > bpf_check_attach_target() with find_kallsyms_symbol_value or
> > kallsyms_lookup_name, which is not accurate in some cases.
> >
> > For example, we want to attach to the target "t_next", but there are
> > multiple symbols with the name "t_next" exist in the kallsyms. The one
> > that kallsyms_lookup_name() returned may have no ftrace record, which
> > makes the attach target not available. So we want the one that has ftrace
> > record to be returned.
> >
> > Meanwhile, there may be multiple symbols with the name "t_next" in ftrace
> > record. In this case, the attach target is ambiguous, so the attach should
> > fail.
>
> could you reproduce this somehow (bpftrace/selftest) for some symbol?
> I'd think pahole now filters all such symbols out of BTF and you need
> BTF func record to load the program in the first place
Hi, what's the version of pahole that does such filtering? I have
compiled the latest pahole, and such symbols exist. The version
of the pahole is v1.30
pahole --version
v1.30
It can be reproduced easily, just try to attach to the symbol t_next.
The "t_next" has multiple definition:
bpftrace -e 'fentry:t_next {printf("1");}'
Attaching 1 probe...
ERROR: Error attaching probe: fentry:vmlinux:t_next
This is the symbol information of t_next:
cat /proc/kallsyms | grep ' t_next'
ffffffff8142d9c0 t t_next
ffffffff81440e80 t t_next
ffffffff8144f1f0 t t_next
ffffffff8145ae90 t t_next
ffffffff81735b30 t t_next
cat /tracing/available_filter_functions | grep '^t_next'
t_next
The related patch is here:
https://lore.kernel.org/bpf/CADxym3Y-Jbzp0FupUgBDJB99GhsbDHyuV71Q6m9xyTpFze4ESg@mail.gmail.com/
(I just distclean and rebuild the kernel, the problem still exists)
Thanks!
Menglong Dong
>
> jirka
>
>
> >
> > Introduce the function bpf_lookup_attach_addr() to do the address lookup,
> > which is able to solve this problem.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> > v2:
> > - Lookup both vmlinux and modules symbols when mod is NULL, just like
> > kallsyms_lookup_name().
> >
> > If the btf is not a modules, shouldn't we lookup on the vmlinux only?
> > I'm not sure if we should keep the same logic with
> > kallsyms_lookup_name().
> >
> > - Return the kernel symbol that don't have ftrace location if the symbols
> > with ftrace location are not available
> > ---
> > kernel/bpf/verifier.c | 77 ++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 72 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 53007182b46b..4bacd0abf207 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -23476,6 +23476,73 @@ static int check_non_sleepable_error_inject(u32 btf_id)
> > return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
> > }
> >
> > +struct symbol_lookup_ctx {
> > + const char *name;
> > + unsigned long addr;
> > + bool ftrace_addr;
> > +};
> > +
> > +static int symbol_callback(void *data, unsigned long addr)
> > +{
> > + struct symbol_lookup_ctx *ctx = data;
> > +
> > + ctx->addr = addr;
> > + if (!ftrace_location(addr))
> > + return 0;
> > +
> > + if (ctx->ftrace_addr)
> > + return -EADDRNOTAVAIL;
> > + ctx->ftrace_addr = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int symbol_mod_callback(void *data, const char *name, unsigned long addr)
> > +{
> > + if (strcmp(((struct symbol_lookup_ctx *)data)->name, name) != 0)
> > + return 0;
> > +
> > + return symbol_callback(data, addr);
> > +}
> > +
> > +/**
> > + * bpf_lookup_attach_addr: Lookup address for a symbol
> > + *
> > + * @mod: kernel module to lookup the symbol, NULL means to lookup both vmlinux
> > + * and modules symbols
> > + * @sym: the symbol to resolve
> > + * @addr: pointer to store the result
> > + *
> > + * Lookup the address of the symbol @sym. If multiple symbols with the name
> > + * @sym exist, the one that has ftrace location is preferred. If more
> > + * than 1 has ftrace location, -EADDRNOTAVAIL will be returned.
> > + *
> > + * Returns: 0 on success, -errno otherwise.
> > + */
> > +static int bpf_lookup_attach_addr(const struct module *mod, const char *sym,
> > + unsigned long *addr)
> > +{
> > + struct symbol_lookup_ctx ctx = { .addr = 0, .name = sym };
> > + const char *mod_name = NULL;
> > + int err = 0;
> > +
> > +#ifdef CONFIG_MODULES
> > + mod_name = mod ? mod->name : NULL;
> > +#endif
> > + if (!mod_name)
> > + err = kallsyms_on_each_match_symbol(symbol_callback, sym, &ctx);
> > +
> > + if (!err && !ctx.addr)
> > + err = module_kallsyms_on_each_symbol(mod_name, symbol_mod_callback,
> > + &ctx);
> > +
> > + if (!ctx.addr)
> > + err = -ENOENT;
> > + *addr = err ? 0 : ctx.addr;
> > +
> > + return err;
> > +}
> > +
> > int bpf_check_attach_target(struct bpf_verifier_log *log,
> > const struct bpf_prog *prog,
> > const struct bpf_prog *tgt_prog,
> > @@ -23729,18 +23796,18 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> > if (btf_is_module(btf)) {
> > mod = btf_try_get_module(btf);
> > if (mod)
> > - addr = find_kallsyms_symbol_value(mod, tname);
> > + ret = bpf_lookup_attach_addr(mod, tname, &addr);
> > else
> > - addr = 0;
> > + ret = -ENOENT;
> > } else {
> > - addr = kallsyms_lookup_name(tname);
> > + ret = bpf_lookup_attach_addr(NULL, tname, &addr);
> > }
> > - if (!addr) {
> > + if (ret) {
> > module_put(mod);
> > bpf_log(log,
> > "The address of function %s cannot be found\n",
> > tname);
> > - return -ENOENT;
> > + return ret;
> > }
> > }
> >
> > --
> > 2.39.5
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2] bpf: make the attach target more accurate
2025-07-09 10:33 ` Menglong Dong
@ 2025-07-09 12:34 ` Jiri Olsa
2025-07-10 6:52 ` Menglong Dong
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2025-07-09 12:34 UTC (permalink / raw)
To: Menglong Dong, Alan Maguire, Ihor Solodrai
Cc: Jiri Olsa, ast, daniel, john.fastabend, andrii, martin.lau,
eddyz87, song, yonghong.song, kpsingh, sdf, haoluo, bpf,
linux-kernel, Menglong Dong
On Wed, Jul 09, 2025 at 06:33:08PM +0800, Menglong Dong wrote:
> On Wed, Jul 9, 2025 at 4:43 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Tue, Jul 08, 2025 at 03:21:40PM +0800, Menglong Dong wrote:
> > > For now, we lookup the address of the attach target in
> > > bpf_check_attach_target() with find_kallsyms_symbol_value or
> > > kallsyms_lookup_name, which is not accurate in some cases.
> > >
> > > For example, we want to attach to the target "t_next", but there are
> > > multiple symbols with the name "t_next" exist in the kallsyms. The one
> > > that kallsyms_lookup_name() returned may have no ftrace record, which
> > > makes the attach target not available. So we want the one that has ftrace
> > > record to be returned.
> > >
> > > Meanwhile, there may be multiple symbols with the name "t_next" in ftrace
> > > record. In this case, the attach target is ambiguous, so the attach should
> > > fail.
> >
> > could you reproduce this somehow (bpftrace/selftest) for some symbol?
> > I'd think pahole now filters all such symbols out of BTF and you need
> > BTF func record to load the program in the first place
>
> Hi, what's the version of pahole that does such filtering? I have
> compiled the latest pahole, and such symbols exist. The version
> of the pahole is v1.30
>
> pahole --version
> v1.30
>
> It can be reproduced easily, just try to attach to the symbol t_next.
> The "t_next" has multiple definition:
>
> bpftrace -e 'fentry:t_next {printf("1");}'
> Attaching 1 probe...
> ERROR: Error attaching probe: fentry:vmlinux:t_next
>
> This is the symbol information of t_next:
>
> cat /proc/kallsyms | grep ' t_next'
> ffffffff8142d9c0 t t_next
> ffffffff81440e80 t t_next
> ffffffff8144f1f0 t t_next
> ffffffff8145ae90 t t_next
> ffffffff81735b30 t t_next
>
> cat /tracing/available_filter_functions | grep '^t_next'
> t_next
>
> The related patch is here:
> https://lore.kernel.org/bpf/CADxym3Y-Jbzp0FupUgBDJB99GhsbDHyuV71Q6m9xyTpFze4ESg@mail.gmail.com/
>
> (I just distclean and rebuild the kernel, the problem still exists)
>
ah right.. I guess they all have same prototype, so the current pahole
filter won't trigger
I wonder pahole could filter out functions that have multiple instances
with different address, because those can't be resolved properly in
trampoline attachment
or we could mitigate that in runtime with your change
Alan, Ihor, any idea?
thanks,
jirka
> Thanks!
> Menglong Dong
>
> >
> > jirka
> >
> >
> > >
> > > Introduce the function bpf_lookup_attach_addr() to do the address lookup,
> > > which is able to solve this problem.
> > >
> > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > > ---
> > > v2:
> > > - Lookup both vmlinux and modules symbols when mod is NULL, just like
> > > kallsyms_lookup_name().
> > >
> > > If the btf is not a modules, shouldn't we lookup on the vmlinux only?
> > > I'm not sure if we should keep the same logic with
> > > kallsyms_lookup_name().
> > >
> > > - Return the kernel symbol that don't have ftrace location if the symbols
> > > with ftrace location are not available
> > > ---
> > > kernel/bpf/verifier.c | 77 ++++++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 72 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 53007182b46b..4bacd0abf207 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -23476,6 +23476,73 @@ static int check_non_sleepable_error_inject(u32 btf_id)
> > > return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
> > > }
> > >
> > > +struct symbol_lookup_ctx {
> > > + const char *name;
> > > + unsigned long addr;
> > > + bool ftrace_addr;
> > > +};
> > > +
> > > +static int symbol_callback(void *data, unsigned long addr)
> > > +{
> > > + struct symbol_lookup_ctx *ctx = data;
> > > +
> > > + ctx->addr = addr;
> > > + if (!ftrace_location(addr))
> > > + return 0;
> > > +
> > > + if (ctx->ftrace_addr)
> > > + return -EADDRNOTAVAIL;
> > > + ctx->ftrace_addr = true;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int symbol_mod_callback(void *data, const char *name, unsigned long addr)
> > > +{
> > > + if (strcmp(((struct symbol_lookup_ctx *)data)->name, name) != 0)
> > > + return 0;
> > > +
> > > + return symbol_callback(data, addr);
> > > +}
> > > +
> > > +/**
> > > + * bpf_lookup_attach_addr: Lookup address for a symbol
> > > + *
> > > + * @mod: kernel module to lookup the symbol, NULL means to lookup both vmlinux
> > > + * and modules symbols
> > > + * @sym: the symbol to resolve
> > > + * @addr: pointer to store the result
> > > + *
> > > + * Lookup the address of the symbol @sym. If multiple symbols with the name
> > > + * @sym exist, the one that has ftrace location is preferred. If more
> > > + * than 1 has ftrace location, -EADDRNOTAVAIL will be returned.
> > > + *
> > > + * Returns: 0 on success, -errno otherwise.
> > > + */
> > > +static int bpf_lookup_attach_addr(const struct module *mod, const char *sym,
> > > + unsigned long *addr)
> > > +{
> > > + struct symbol_lookup_ctx ctx = { .addr = 0, .name = sym };
> > > + const char *mod_name = NULL;
> > > + int err = 0;
> > > +
> > > +#ifdef CONFIG_MODULES
> > > + mod_name = mod ? mod->name : NULL;
> > > +#endif
> > > + if (!mod_name)
> > > + err = kallsyms_on_each_match_symbol(symbol_callback, sym, &ctx);
> > > +
> > > + if (!err && !ctx.addr)
> > > + err = module_kallsyms_on_each_symbol(mod_name, symbol_mod_callback,
> > > + &ctx);
> > > +
> > > + if (!ctx.addr)
> > > + err = -ENOENT;
> > > + *addr = err ? 0 : ctx.addr;
> > > +
> > > + return err;
> > > +}
> > > +
> > > int bpf_check_attach_target(struct bpf_verifier_log *log,
> > > const struct bpf_prog *prog,
> > > const struct bpf_prog *tgt_prog,
> > > @@ -23729,18 +23796,18 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> > > if (btf_is_module(btf)) {
> > > mod = btf_try_get_module(btf);
> > > if (mod)
> > > - addr = find_kallsyms_symbol_value(mod, tname);
> > > + ret = bpf_lookup_attach_addr(mod, tname, &addr);
> > > else
> > > - addr = 0;
> > > + ret = -ENOENT;
> > > } else {
> > > - addr = kallsyms_lookup_name(tname);
> > > + ret = bpf_lookup_attach_addr(NULL, tname, &addr);
> > > }
> > > - if (!addr) {
> > > + if (ret) {
> > > module_put(mod);
> > > bpf_log(log,
> > > "The address of function %s cannot be found\n",
> > > tname);
> > > - return -ENOENT;
> > > + return ret;
> > > }
> > > }
> > >
> > > --
> > > 2.39.5
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2] bpf: make the attach target more accurate
2025-07-09 12:34 ` Jiri Olsa
@ 2025-07-10 6:52 ` Menglong Dong
0 siblings, 0 replies; 6+ messages in thread
From: Menglong Dong @ 2025-07-10 6:52 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alan Maguire, Ihor Solodrai, ast, daniel, john.fastabend, andrii,
martin.lau, eddyz87, song, yonghong.song, kpsingh, sdf, haoluo,
bpf, linux-kernel, Menglong Dong
On Wed, Jul 9, 2025 at 8:34 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Jul 09, 2025 at 06:33:08PM +0800, Menglong Dong wrote:
> > On Wed, Jul 9, 2025 at 4:43 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Tue, Jul 08, 2025 at 03:21:40PM +0800, Menglong Dong wrote:
> > > > For now, we lookup the address of the attach target in
> > > > bpf_check_attach_target() with find_kallsyms_symbol_value or
> > > > kallsyms_lookup_name, which is not accurate in some cases.
> > > >
> > > > For example, we want to attach to the target "t_next", but there are
> > > > multiple symbols with the name "t_next" exist in the kallsyms. The one
> > > > that kallsyms_lookup_name() returned may have no ftrace record, which
> > > > makes the attach target not available. So we want the one that has ftrace
> > > > record to be returned.
> > > >
> > > > Meanwhile, there may be multiple symbols with the name "t_next" in ftrace
> > > > record. In this case, the attach target is ambiguous, so the attach should
> > > > fail.
> > >
> > > could you reproduce this somehow (bpftrace/selftest) for some symbol?
> > > I'd think pahole now filters all such symbols out of BTF and you need
> > > BTF func record to load the program in the first place
> >
> > Hi, what's the version of pahole that does such filtering? I have
> > compiled the latest pahole, and such symbols exist. The version
> > of the pahole is v1.30
> >
> > pahole --version
> > v1.30
> >
> > It can be reproduced easily, just try to attach to the symbol t_next.
> > The "t_next" has multiple definition:
> >
> > bpftrace -e 'fentry:t_next {printf("1");}'
> > Attaching 1 probe...
> > ERROR: Error attaching probe: fentry:vmlinux:t_next
> >
> > This is the symbol information of t_next:
> >
> > cat /proc/kallsyms | grep ' t_next'
> > ffffffff8142d9c0 t t_next
> > ffffffff81440e80 t t_next
> > ffffffff8144f1f0 t t_next
> > ffffffff8145ae90 t t_next
> > ffffffff81735b30 t t_next
> >
> > cat /tracing/available_filter_functions | grep '^t_next'
> > t_next
> >
> > The related patch is here:
> > https://lore.kernel.org/bpf/CADxym3Y-Jbzp0FupUgBDJB99GhsbDHyuV71Q6m9xyTpFze4ESg@mail.gmail.com/
> >
> > (I just distclean and rebuild the kernel, the problem still exists)
> >
>
> ah right.. I guess they all have same prototype, so the current pahole
> filter won't trigger
>
> I wonder pahole could filter out functions that have multiple instances
> with different address, because those can't be resolved properly in
> trampoline attachment
>
> or we could mitigate that in runtime with your change
Anyway, I'm going to send the V3, as this version has
problem. Then, let's see how it goes :/
>
> Alan, Ihor, any idea?
>
> thanks,
> jirka
>
>
> > Thanks!
> > Menglong Dong
> >
> > >
> > > jirka
> > >
> > >
> > > >
> > > > Introduce the function bpf_lookup_attach_addr() to do the address lookup,
> > > > which is able to solve this problem.
> > > >
> > > > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > > > ---
> > > > v2:
> > > > - Lookup both vmlinux and modules symbols when mod is NULL, just like
> > > > kallsyms_lookup_name().
> > > >
> > > > If the btf is not a modules, shouldn't we lookup on the vmlinux only?
> > > > I'm not sure if we should keep the same logic with
> > > > kallsyms_lookup_name().
> > > >
> > > > - Return the kernel symbol that don't have ftrace location if the symbols
> > > > with ftrace location are not available
> > > > ---
> > > > kernel/bpf/verifier.c | 77 ++++++++++++++++++++++++++++++++++++++++---
> > > > 1 file changed, 72 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index 53007182b46b..4bacd0abf207 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -23476,6 +23476,73 @@ static int check_non_sleepable_error_inject(u32 btf_id)
> > > > return btf_id_set_contains(&btf_non_sleepable_error_inject, btf_id);
> > > > }
> > > >
> > > > +struct symbol_lookup_ctx {
> > > > + const char *name;
> > > > + unsigned long addr;
> > > > + bool ftrace_addr;
> > > > +};
> > > > +
> > > > +static int symbol_callback(void *data, unsigned long addr)
> > > > +{
> > > > + struct symbol_lookup_ctx *ctx = data;
> > > > +
> > > > + ctx->addr = addr;
> > > > + if (!ftrace_location(addr))
> > > > + return 0;
> > > > +
> > > > + if (ctx->ftrace_addr)
> > > > + return -EADDRNOTAVAIL;
> > > > + ctx->ftrace_addr = true;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int symbol_mod_callback(void *data, const char *name, unsigned long addr)
> > > > +{
> > > > + if (strcmp(((struct symbol_lookup_ctx *)data)->name, name) != 0)
> > > > + return 0;
> > > > +
> > > > + return symbol_callback(data, addr);
> > > > +}
> > > > +
> > > > +/**
> > > > + * bpf_lookup_attach_addr: Lookup address for a symbol
> > > > + *
> > > > + * @mod: kernel module to lookup the symbol, NULL means to lookup both vmlinux
> > > > + * and modules symbols
> > > > + * @sym: the symbol to resolve
> > > > + * @addr: pointer to store the result
> > > > + *
> > > > + * Lookup the address of the symbol @sym. If multiple symbols with the name
> > > > + * @sym exist, the one that has ftrace location is preferred. If more
> > > > + * than 1 has ftrace location, -EADDRNOTAVAIL will be returned.
> > > > + *
> > > > + * Returns: 0 on success, -errno otherwise.
> > > > + */
> > > > +static int bpf_lookup_attach_addr(const struct module *mod, const char *sym,
> > > > + unsigned long *addr)
> > > > +{
> > > > + struct symbol_lookup_ctx ctx = { .addr = 0, .name = sym };
> > > > + const char *mod_name = NULL;
> > > > + int err = 0;
> > > > +
> > > > +#ifdef CONFIG_MODULES
> > > > + mod_name = mod ? mod->name : NULL;
> > > > +#endif
> > > > + if (!mod_name)
> > > > + err = kallsyms_on_each_match_symbol(symbol_callback, sym, &ctx);
> > > > +
> > > > + if (!err && !ctx.addr)
> > > > + err = module_kallsyms_on_each_symbol(mod_name, symbol_mod_callback,
> > > > + &ctx);
> > > > +
> > > > + if (!ctx.addr)
> > > > + err = -ENOENT;
> > > > + *addr = err ? 0 : ctx.addr;
> > > > +
> > > > + return err;
> > > > +}
> > > > +
> > > > int bpf_check_attach_target(struct bpf_verifier_log *log,
> > > > const struct bpf_prog *prog,
> > > > const struct bpf_prog *tgt_prog,
> > > > @@ -23729,18 +23796,18 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> > > > if (btf_is_module(btf)) {
> > > > mod = btf_try_get_module(btf);
> > > > if (mod)
> > > > - addr = find_kallsyms_symbol_value(mod, tname);
> > > > + ret = bpf_lookup_attach_addr(mod, tname, &addr);
> > > > else
> > > > - addr = 0;
> > > > + ret = -ENOENT;
> > > > } else {
> > > > - addr = kallsyms_lookup_name(tname);
> > > > + ret = bpf_lookup_attach_addr(NULL, tname, &addr);
> > > > }
> > > > - if (!addr) {
> > > > + if (ret) {
> > > > module_put(mod);
> > > > bpf_log(log,
> > > > "The address of function %s cannot be found\n",
> > > > tname);
> > > > - return -ENOENT;
> > > > + return ret;
> > > > }
> > > > }
> > > >
> > > > --
> > > > 2.39.5
> > > >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-10 6:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-08 7:21 [PATCH bpf-next v2] bpf: make the attach target more accurate Menglong Dong
2025-07-08 12:17 ` Menglong Dong
2025-07-09 8:43 ` Jiri Olsa
2025-07-09 10:33 ` Menglong Dong
2025-07-09 12:34 ` Jiri Olsa
2025-07-10 6:52 ` Menglong Dong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).