On Wed, Jul 9, 2025 at 8:34 PM Jiri Olsa <olsajiri@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxx/
> >
> > (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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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
> > > >
