bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).