Message-ID: <CADxym3YGF6jCg=J1bQs60SePEwigh7S+7yfXAdU+yc3WX9HAGQ@mail.gmail.com>
Date: Fri, 11 Jul 2025 13:51:31 +0800
From: Menglong Dong <menglong8.dong@...il.com>
To: Yonghong Song <yonghong.song@...ux.dev>
Cc: ast@...nel.org, daniel@...earbox.net, john.fastabend@...il.com, 
	andrii@...nel.org, martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org, 
	kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Menglong Dong <dongml2@...natelecom.cn>
Subject: Re: [PATCH bpf-next v3] bpf: make the attach target more accurate

On Fri, Jul 11, 2025 at 11:46 AM Yonghong Song <yonghong.song@...ux.dev> wrote:
>
>
>
> On 7/10/25 8:10 PM, Yonghong Song wrote:
> >
> >
> > On 7/10/25 12:08 AM, 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, which
> >> makes
> >> the attach target ambiguous, and the attach should fail.
> >>
> >> Introduce the function bpf_lookup_attach_addr() to do the address
> >> lookup,
> >> which will return -EADDRNOTAVAIL when the symbol is not unique.
> >>
> >> We can do the testing with following shell:
> >>
> >> for s in $(cat /proc/kallsyms | awk '{print $3}' | sort | uniq -d)
> >> do
> >>    if grep -q "^$s\$"
> >> /sys/kernel/debug/tracing/available_filter_functions
> >>    then
> >>      bpftrace -e "fentry:$s {printf(\"1\");}" -v
> >>    fi
> >> done
> >>
> >> The script will find all the duplicated symbols in /proc/kallsyms, which
> >> is also in /sys/kernel/debug/tracing/available_filter_functions, and
> >> attach them with bpftrace.
> >>
> >> After this patch, all the attaching fail with the error:
> >>
> >> The address of function xxx cannot be found
> >> or
> >> No BTF found for xxx
> >>
> >> Signed-off-by: Menglong Dong <dongml2@...natelecom.cn>
> >
> > Maybe we should prevent vmlinux BTF generation for such symbols
> > which are static and have more than one instances? This can
> > be done in pahole and downstream libbpf/kernel do not
> > need to do anything. This can avoid libbpf/kernel runtime overhead
> > since bpf_lookup_attach_addr() could be expensive as it needs
> > to go through ALL symbols, even for unique symbols.

Hi, yonghong. You are right, the best solution is to solve
this problem in the pahole, just like what Jiri said in the V2:
  https://lore.kernel.org/bpf/aG5hzvaqXi7uI4GL@krava/

I wonder will we focus the users to use the latest pahole
that supports duplicate symbols filter after we fix this problem
in pahole? If so, this patch is useless, and just ignore it. If
not, the only usage of this patch is for the users that build
the kernel with an old pahole.

>
> There is a multi-link effort:
>    https://lore.kernel.org/bpf/20250703121521.1874196-1-dongml2@chinatelecom.cn/
> which tries to do similar thing for multi-kprobe. For example, for fentry,
> multi-link may pass an array of btf_id's to the kernel. For such cases,
> this patch may cause significant performance overhead.

For the symbol in the vmlinux, there will be no additional overhead,
as the logic is the same as previous. If the symbol is in the
modules, it does have additional overhead. Following is the
testing that hooks all the symbols with fentry-multi.

Without this patch, the time to attach all the symbols:
kernel: 0.372660s for 48857 symbols
modules: 0.135543s for 8631 symbols

And with this patch, the time is:
kernel: 0.380087s for 48857 symbols
modules: 0.176904s for 8631 symbols

One more thing, is there anyone to fix the problem in pahole?
I mean, I'm not good at pahole. But if there is nobody, I still can
do this job, but I need to learn it first :/

Thanks!
Menglong Dong
>
> >
> >
> >> ---
> >> v3:
> >> - reject all the duplicated symbols
> >> 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 | 71 ++++++++++++++++++++++++++++++++++++++++---
> >>   1 file changed, 66 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 53007182b46b..bf4951154605 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -23476,6 +23476,67 @@ 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;
> >> +};
> >> +
> >> +static int symbol_callback(void *data, unsigned long addr)
> >> +{
> >> +    struct symbol_lookup_ctx *ctx = data;
> >> +
> >> +    if (ctx->addr)
> >> +        return -EADDRNOTAVAIL;
> >> +    ctx->addr = addr;
> >> +
> >> +    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, -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 +23790,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;
> >>               }
> >>           }
> >
> >
>

Powered by blists - more mailing lists