* [PATCH bpf-next v3] bpf: make the attach target more accurate @ 2025-07-10 7:08 Menglong Dong 2025-07-10 23:24 ` Andrii Nakryiko ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Menglong Dong @ 2025-07-10 7:08 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, 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@chinatelecom.cn> --- 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; } } -- 2.39.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-10 7:08 [PATCH bpf-next v3] bpf: make the attach target more accurate Menglong Dong @ 2025-07-10 23:24 ` Andrii Nakryiko 2025-07-11 1:27 ` Menglong Dong 2025-07-11 3:10 ` Yonghong Song 2025-07-14 19:52 ` Alexei Starovoitov 2 siblings, 1 reply; 13+ messages in thread From: Andrii Nakryiko @ 2025-07-10 23:24 UTC (permalink / raw) To: Menglong Dong Cc: ast, daniel, john.fastabend, andrii, martin.lau, eddyz87, song, yonghong.song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, Menglong Dong On Thu, Jul 10, 2025 at 12:10 AM 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, 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@chinatelecom.cn> > --- > 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; #define ENOTUNIQ 76 /* Name not unique on network */ fits a bit better, no? > + ctx->addr = addr; > + > + return 0; > +} > + [...] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-10 23:24 ` Andrii Nakryiko @ 2025-07-11 1:27 ` Menglong Dong 0 siblings, 0 replies; 13+ messages in thread From: Menglong Dong @ 2025-07-11 1:27 UTC (permalink / raw) To: Andrii Nakryiko Cc: ast, daniel, john.fastabend, andrii, martin.lau, eddyz87, song, yonghong.song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, Menglong Dong On Fri, Jul 11, 2025 at 7:25 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Jul 10, 2025 at 12:10 AM 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, 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@chinatelecom.cn> > > --- > > 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; > > #define ENOTUNIQ 76 /* Name not unique on network */ > > fits a bit better, no? Yeah, this looks much better. I'll use it instead of EADDRNOTAVAIL in the next version. Thanks! Menglong Dong > > > + ctx->addr = addr; > > + > > + return 0; > > +} > > + > > [...] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-10 7:08 [PATCH bpf-next v3] bpf: make the attach target more accurate Menglong Dong 2025-07-10 23:24 ` Andrii Nakryiko @ 2025-07-11 3:10 ` Yonghong Song 2025-07-11 3:46 ` Yonghong Song 2025-07-14 19:52 ` Alexei Starovoitov 2 siblings, 1 reply; 13+ messages in thread From: Yonghong Song @ 2025-07-11 3:10 UTC (permalink / raw) To: Menglong Dong, ast, daniel Cc: john.fastabend, andrii, martin.lau, eddyz87, song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, Menglong Dong 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@chinatelecom.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. > --- > 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; > } > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-11 3:10 ` Yonghong Song @ 2025-07-11 3:46 ` Yonghong Song 2025-07-11 5:51 ` Menglong Dong 0 siblings, 1 reply; 13+ messages in thread From: Yonghong Song @ 2025-07-11 3:46 UTC (permalink / raw) To: Menglong Dong, ast, daniel Cc: john.fastabend, andrii, martin.lau, eddyz87, song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, Menglong Dong 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@chinatelecom.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. 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. > > >> --- >> 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; >> } >> } > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-11 3:46 ` Yonghong Song @ 2025-07-11 5:51 ` Menglong Dong 2025-07-11 6:37 ` Menglong Dong 2025-07-11 11:23 ` Jiri Olsa 0 siblings, 2 replies; 13+ messages in thread From: Menglong Dong @ 2025-07-11 5:51 UTC (permalink / raw) To: Yonghong Song Cc: ast, daniel, john.fastabend, andrii, martin.lau, eddyz87, song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, Menglong Dong On Fri, Jul 11, 2025 at 11:46 AM Yonghong Song <yonghong.song@linux.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@chinatelecom.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; > >> } > >> } > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-11 5:51 ` Menglong Dong @ 2025-07-11 6:37 ` Menglong Dong 2025-07-11 11:23 ` Jiri Olsa 1 sibling, 0 replies; 13+ messages in thread From: Menglong Dong @ 2025-07-11 6:37 UTC (permalink / raw) To: Yonghong Song, Jiri Olsa Cc: ast, daniel, john.fastabend, andrii, martin.lau, eddyz87, song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, Menglong Dong On Fri, Jul 11, 2025 at 1:51 PM Menglong Dong <menglong8.dong@gmail.com> wrote: > > On Fri, Jul 11, 2025 at 11:46 AM Yonghong Song <yonghong.song@linux.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@chinatelecom.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. Sorry that I forgot to Cc Jiri :/ > > > > > 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; > > >> } > > >> } > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-11 5:51 ` Menglong Dong 2025-07-11 6:37 ` Menglong Dong @ 2025-07-11 11:23 ` Jiri Olsa 2025-07-11 12:01 ` Menglong Dong 1 sibling, 1 reply; 13+ messages in thread From: Jiri Olsa @ 2025-07-11 11:23 UTC (permalink / raw) To: Menglong Dong Cc: Yonghong Song, ast, daniel, john.fastabend, andrii, martin.lau, eddyz87, song, kpsingh, sdf, haoluo, bpf, linux-kernel, Menglong Dong On Fri, Jul 11, 2025 at 01:51:31PM +0800, Menglong Dong wrote: > On Fri, Jul 11, 2025 at 11:46 AM Yonghong Song <yonghong.song@linux.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@chinatelecom.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 :/ I'm testing change below, I'll send the patch after some more testing jirka --- diff --git a/btf_encoder.c b/btf_encoder.c index 16739066caae..29ff86bac7de 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -2143,6 +2143,31 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf) return err; } +static void remove_dups(struct elf_functions *functions) +{ + struct elf_function *n = &functions->entries[0]; + bool matched = false; + int i, j; + + for (i = 0, j = 1; i < functions->cnt && j < functions->cnt; i++, j++) { + struct elf_function *a = &functions->entries[i]; + struct elf_function *b = &functions->entries[j]; + + if (!strcmp(a->name, b->name)) { + matched = true; + continue; + } + + if (!matched) + *n++ = *a; + matched = false; + } + + if (!matched) + *n++ = functions->entries[functions->cnt - 1]; + functions->cnt = n - &functions->entries[0]; +} + static int elf_functions__collect(struct elf_functions *functions) { uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab); @@ -2168,6 +2193,7 @@ static int elf_functions__collect(struct elf_functions *functions) if (functions->cnt) { qsort(functions->entries, functions->cnt, sizeof(*functions->entries), functions_cmp); + remove_dups(functions); } else { err = 0; goto out_free; ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-11 11:23 ` Jiri Olsa @ 2025-07-11 12:01 ` Menglong Dong 0 siblings, 0 replies; 13+ messages in thread From: Menglong Dong @ 2025-07-11 12:01 UTC (permalink / raw) To: Jiri Olsa Cc: Yonghong Song, ast, daniel, john.fastabend, andrii, martin.lau, eddyz87, song, kpsingh, sdf, haoluo, bpf, linux-kernel, Menglong Dong On Fri, Jul 11, 2025 at 7:23 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Fri, Jul 11, 2025 at 01:51:31PM +0800, Menglong Dong wrote: > > On Fri, Jul 11, 2025 at 11:46 AM Yonghong Song <yonghong.song@linux.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@chinatelecom.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 :/ > > I'm testing change below, I'll send the patch after some more testing Awesome, thank you :/ > > jirka > > > --- > diff --git a/btf_encoder.c b/btf_encoder.c > index 16739066caae..29ff86bac7de 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -2143,6 +2143,31 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf) > return err; > } > > +static void remove_dups(struct elf_functions *functions) > +{ > + struct elf_function *n = &functions->entries[0]; > + bool matched = false; > + int i, j; > + > + for (i = 0, j = 1; i < functions->cnt && j < functions->cnt; i++, j++) { > + struct elf_function *a = &functions->entries[i]; > + struct elf_function *b = &functions->entries[j]; > + > + if (!strcmp(a->name, b->name)) { > + matched = true; > + continue; > + } > + > + if (!matched) > + *n++ = *a; > + matched = false; > + } > + > + if (!matched) > + *n++ = functions->entries[functions->cnt - 1]; > + functions->cnt = n - &functions->entries[0]; > +} > + > static int elf_functions__collect(struct elf_functions *functions) > { > uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab); > @@ -2168,6 +2193,7 @@ static int elf_functions__collect(struct elf_functions *functions) > > if (functions->cnt) { > qsort(functions->entries, functions->cnt, sizeof(*functions->entries), functions_cmp); > + remove_dups(functions); > } else { > err = 0; > goto out_free; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-10 7:08 [PATCH bpf-next v3] bpf: make the attach target more accurate Menglong Dong 2025-07-10 23:24 ` Andrii Nakryiko 2025-07-11 3:10 ` Yonghong Song @ 2025-07-14 19:52 ` Alexei Starovoitov 2025-07-14 21:50 ` Menglong Dong 2 siblings, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2025-07-14 19:52 UTC (permalink / raw) To: Menglong Dong Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Eduard, Song Liu, Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML, Menglong Dong On Thu, Jul 10, 2025 at 12:10 AM Menglong Dong <menglong8.dong@gmail.com> wrote: > } else { > - addr = kallsyms_lookup_name(tname); > + ret = bpf_lookup_attach_addr(NULL, tname, &addr); > } Not sure why your benchmarking doesn't show the difference, but above is a big regression. kallsyms_lookup_name() is a binary search whereas your bpf_lookup_attach_addr() is linear. You should see a massive degradation in multi-kprobe attach speeds. -- pw-bot: cr ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-14 19:52 ` Alexei Starovoitov @ 2025-07-14 21:50 ` Menglong Dong 2025-07-14 22:29 ` Alexei Starovoitov 0 siblings, 1 reply; 13+ messages in thread From: Menglong Dong @ 2025-07-14 21:50 UTC (permalink / raw) To: Alexei Starovoitov, Menglong Dong Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Eduard, Song Liu, Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML, Menglong Dong On 2025/7/15 03:52, Alexei Starovoitov wrote: > On Thu, Jul 10, 2025 at 12:10 AM Menglong Dong <menglong8.dong@gmail.com> wrote: >> } else { >> - addr = kallsyms_lookup_name(tname); >> + ret = bpf_lookup_attach_addr(NULL, tname, &addr); >> } > Not sure why your benchmarking doesn't show the difference, > but above is a big regression. > kallsyms_lookup_name() is a binary search whereas your > bpf_lookup_attach_addr() is linear. > You should see a massive degradation in multi-kprobe attach speeds. Hi, Alexei. Like I said above, the benchmarking does have a difference for the symbol in the modules, which makes the attachment time increased from 0.135543s to 0.176904s for 8631 symbols. As the symbols in the modules is not plentiful, which makes the overhead slight(or not?). But for the symbol in vmlinux, bpf_lookup_attach_addr() will call kallsyms_on_each_match_symbol(), which is also a binary search, so the benchmarking has no difference, which makes sense. I thought we don't need this patch after the pahole fixes this problem. Should I send a V4? Thanks! Menglong Dong > > -- > pw-bot: cr > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-14 21:50 ` Menglong Dong @ 2025-07-14 22:29 ` Alexei Starovoitov 2025-07-14 23:32 ` Menglong Dong 0 siblings, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2025-07-14 22:29 UTC (permalink / raw) To: Menglong Dong Cc: Menglong Dong, Alexei Starovoitov, Daniel Borkmann, John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Eduard, Song Liu, Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML, Menglong Dong On Mon, Jul 14, 2025 at 2:50 PM Menglong Dong <menglong.dong@linux.dev> wrote: > > > On 2025/7/15 03:52, Alexei Starovoitov wrote: > > On Thu, Jul 10, 2025 at 12:10 AM Menglong Dong <menglong8.dong@gmail.com> wrote: > >> } else { > >> - addr = kallsyms_lookup_name(tname); > >> + ret = bpf_lookup_attach_addr(NULL, tname, &addr); > >> } > > Not sure why your benchmarking doesn't show the difference, > > but above is a big regression. > > kallsyms_lookup_name() is a binary search whereas your > > bpf_lookup_attach_addr() is linear. > > You should see a massive degradation in multi-kprobe attach speeds. > > > Hi, Alexei. Like I said above, the benchmarking does have > a difference for the symbol in the modules, which makes > the attachment time increased from 0.135543s to 0.176904s > for 8631 symbols. As the symbols in the modules > is not plentiful, which makes the overhead slight(or not?). > > But for the symbol in vmlinux, bpf_lookup_attach_addr() will > call kallsyms_on_each_match_symbol(), which is also > a binary search, so the benchmarking has no difference, > which makes sense. I see. Just curious, what was the function count in modules on your system ? cat /proc/kallsyms|grep '\['|grep -v bpf|wc -l Only now I read the diff carefully enough to realize that you're looking for duplicates across vmlinux and that one module. Why ? BTF based attachment identifies a specific module. Even if there are dups between that module and vmlinux the attachment is not ambiguous. > I thought we don't need this patch after the pahole fixes this > problem. Should I send a V4? pahole should fix it, so this change is not needed. But pahole will be removing the dups within vmlinux and within each module independently. Not across them. I don't think "across" is needed, but you somehow believe that it's necessary ? (based on this diff) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next v3] bpf: make the attach target more accurate 2025-07-14 22:29 ` Alexei Starovoitov @ 2025-07-14 23:32 ` Menglong Dong 0 siblings, 0 replies; 13+ messages in thread From: Menglong Dong @ 2025-07-14 23:32 UTC (permalink / raw) To: Alexei Starovoitov Cc: Menglong Dong, Alexei Starovoitov, Daniel Borkmann, John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Eduard, Song Liu, Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML, Menglong Dong On 2025/7/15 06:29, Alexei Starovoitov wrote: > On Mon, Jul 14, 2025 at 2:50 PM Menglong Dong <menglong.dong@linux.dev> wrote: >> >> On 2025/7/15 03:52, Alexei Starovoitov wrote: >>> On Thu, Jul 10, 2025 at 12:10 AM Menglong Dong <menglong8.dong@gmail.com> wrote: >>>> } else { >>>> - addr = kallsyms_lookup_name(tname); >>>> + ret = bpf_lookup_attach_addr(NULL, tname, &addr); >>>> } >>> Not sure why your benchmarking doesn't show the difference, >>> but above is a big regression. >>> kallsyms_lookup_name() is a binary search whereas your >>> bpf_lookup_attach_addr() is linear. >>> You should see a massive degradation in multi-kprobe attach speeds. >> >> Hi, Alexei. Like I said above, the benchmarking does have >> a difference for the symbol in the modules, which makes >> the attachment time increased from 0.135543s to 0.176904s >> for 8631 symbols. As the symbols in the modules >> is not plentiful, which makes the overhead slight(or not?). >> >> But for the symbol in vmlinux, bpf_lookup_attach_addr() will >> call kallsyms_on_each_match_symbol(), which is also >> a binary search, so the benchmarking has no difference, >> which makes sense. > I see. > Just curious, what was the function count in modules on your system ? > cat /proc/kallsyms|grep '\['|grep -v bpf|wc -l Hi, it's about 34k: cat/proc/kallsyms|grep'\['|grep-vbpf|wc-l 34740 > > Only now I read the diff carefully enough to realize that > you're looking for duplicates across vmlinux and that one module. > > Why ? > BTF based attachment identifies a specific module. > Even if there are dups between that module and vmlinux the attachment > is not ambiguous. When the module is not specified, kallsyms_lookup_name() will be called in bpf_check_attach_target() to get the address. And kallsyms_lookup_name() will lookup the symbols in the vmlinux first. If not found, it will lookup it in the modules. And in this commit, I follow this logic. So I lookup duplicates accross vmlinux and modules only when the modules is not specified, and that's reasonable. However, we always find the address in the vmlinux if module is not specified, as the btf type belong to the vmlinux. So we will never lookup the symbols in the modules if "mod" is not specified, which means bpf_lookup_attach_addr() won't lookup duplicates accross vmlinux and modules. So just forget it :/ Thanks! Menglong Dong > >> I thought we don't need this patch after the pahole fixes this >> problem. Should I send a V4? > pahole should fix it, so this change is not needed. > But pahole will be removing the dups within vmlinux and > within each module independently. Not across them. > I don't think "across" is needed, but you somehow believe that > it's necessary ? (based on this diff) ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-07-14 23:32 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-10 7:08 [PATCH bpf-next v3] bpf: make the attach target more accurate Menglong Dong 2025-07-10 23:24 ` Andrii Nakryiko 2025-07-11 1:27 ` Menglong Dong 2025-07-11 3:10 ` Yonghong Song 2025-07-11 3:46 ` Yonghong Song 2025-07-11 5:51 ` Menglong Dong 2025-07-11 6:37 ` Menglong Dong 2025-07-11 11:23 ` Jiri Olsa 2025-07-11 12:01 ` Menglong Dong 2025-07-14 19:52 ` Alexei Starovoitov 2025-07-14 21:50 ` Menglong Dong 2025-07-14 22:29 ` Alexei Starovoitov 2025-07-14 23:32 ` 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).