[PATCH v3 0/1] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation

[PATCH v3 0/1] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation

[Francis Laniel]

Hi.


In the kernel source code, it exists different functions which share the same
name but which have, of course, different addresses as they can be defined in
different modules:
# Kernel was compiled with CONFIG_NTFS_FS and CONFIG_NTFS3_FS as built-in.
root@vm-amd64:~# grep ntfs_file_write_iter /proc/kallsyms
ffffffff814ce3c0 t __pfx_ntfs_file_write_iter
ffffffff814ce3d0 t ntfs_file_write_iter
ffffffff814fc8a0 t __pfx_ntfs_file_write_iter
ffffffff814fc8b0 t ntfs_file_write_iter
This can be source of troubles when you create a PMU kprobe for such a function,
as it will only install one for the first address (e.g. 0xffffffff814ce3d0 in
the above).
This could lead to some troubles were BPF based tools does not report any event
because the second function is not called:
root@vm-amd64:/mnt# mount | grep /mnt
/foo.img on /mnt type ntfs3 (rw,relatime,uid=0,gid=0,iocharset=utf8)
# ig is a tool which installs a PMU kprobe on ntfs_file_write_iter().
root@vm-amd64:/mnt# ig trace fsslower -m 0 -f ntfs3 --host &> /tmp/foo &
[1] 207
root@vm-amd64:/mnt# dd if=./foo of=./bar count=3
3+0 records in
3+0 records out
1536 bytes (1.5 kB, 1.5 KiB) copied, 0.00543323 s, 283 kB/s
root@vm-amd64:/mnt# fg
ig trace fsslower -m 0 -f ntfs3 --host &> /tmp/foo
^Croot@vm-amd64:/mnt# more /tmp/foo
RUNTIME.CONTAINERNAME          RUNTIME.CONTAIN… PID              COMM
  T      BYTES     OFFSET        LAT FILE
                                                214              dd
  R        512          0        766 foo
                                                214              dd
  R        512        512          9 foo
                                                214              dd
As you can see in the above, only read events are reported and no write because
the kprobe is installed for the old ntfs_file_write_iter() and not the ntfs3
one.
The same behavior occurs with sysfs kprobe:
root@vm-amd64:/# echo 'p:probe/ntfs_file_write_iter ntfs_file_write_iter' > /sys/kernel/tracing/kprobe_events
root@vm-amd64:/# cat /sys/kernel/tracing/kprobe_events
p:probe/ntfs_file_write_iter ntfs_file_write_iter
root@vm-amd64:/# mount | grep /mnt
/foo.img on /mnt type ntfs3 (rw,relatime,uid=0,gid=0,iocharset=utf8)
root@vm-amd64:/# perf record -e probe:ntfs_file_write_iter &
[1] 210
root@vm-amd64:/# cd /mnt/
root@vm-amd64:/mnt# dd if=./foo of=./bar count=3
3+0 records in
3+0 records out
1536 bytes (1.5 kB, 1.5 KiB) copied, 0.00234793 s, 654 kB/s
root@vm-amd64:/mnt# cd -
/
root@vm-amd64:/# fg
perf record -e probe:ntfs_file_write_iter
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.056 MB perf.data ]

root@vm-amd64:/# perf report
Error:
The perf.data data has no samples!
# To display the perf.data header info, please use --header/--header-only optio>
#

In this contribution, I modified the functions creating sysfs and PMU kprobes to
test if the function name given as argument matches several symbols.
In this case, these functions return EADDRNOTAVAIL to indicate the user to use
addr and offs to remove this ambiguity.
So, when the above BPF tool is run, the following error message is printed:
root@vm-amd64:~# ig trace fsslower -m 0 -f ntfs3 --host &> /tmp/foo &
[1] 228
root@vm-amd64:~# more /tmp/foo
RUNTIME.CONTAINERNAME          RUNTIME.CONTAIN… PID              COMM
  T      BYTES     OFFSET        LAT FILE
Error: running gadget: running gadget: installing tracer: attaching kprobe: crea
ting perf_kprobe PMU (arch-specific fallback for "ntfs_file_write_iter"): token
ntfs_file_write_iter: opening perf event: cannot assign requested address
And the same with sysfs kprobe:
root@vm-amd64:/# echo 'p:probe/ntfs_file_write_iter ntfs_file_write_iter' > /sys/kernel/tracing/kprobe_events
-bash: echo: write error: Cannot assign requested address
Note that, this does not influence perf as it installs kprobes as offset on
_text:
root@vm-amd64:/# perf probe --add ntfs_file_write_iter
Added new events:
  probe:ntfs_file_write_iter (on ntfs_file_write_iter)
  probe:ntfs_file_write_iter (on ntfs_file_write_iter)
...
root@vm-amd64:/# cat /sys/kernel/tracing/kprobe_events
p:probe/ntfs_file_write_iter _text+5039088
p:probe/ntfs_file_write_iter _text+5228752

Note that, this contribution is the conclusion of a previous RFC which intended
to install a PMU kprobe for all matching symbols [1, 2].

If you see any way to improve this contribution, particularly if you have an
idea to add tests or documentation for this behavior, please share your
feedback.

Changes since:
 v1:
  * Use EADDRNOTAVAIL instead of adding a new error code.
  * Correct also this behavior for sysfs kprobe.
 v2:
  * Count the number of symbols corresponding to function name and return
  EADDRNOTAVAIL if higher than 1.
  * Return ENOENT if above count is 0, as it would be returned later by while
  registering the kprobe.

Francis Laniel (1):
  tracing/kprobes: Return EADDRNOTAVAIL when func matches several
    symbols

 kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)


Best regards and thank you in advance.
---
[1]: https://lore.kernel.org/lkml/20230816163517.112518-1-flaniel@linux.microsoft.com/
[2]: https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kernel.org/
--
2.34.1

[PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Francis Laniel]

Previously to this commit, if func matches several symbols, a kprobe, being
either sysfs or PMU, would only be installed for the first matching address.
This could lead to some misunderstanding when some BPF code was never called
because it was attached to a function which was indeed not called, because
the effectively called one has no kprobes attached.

So, this commit returns EADDRNOTAVAIL when func matches several symbols.
This way, user needs to use address to remove the ambiguity.

Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Link: https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kernel.org/
---
 kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 23dba01831f7..2f393739e8cf 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = {
 	.priority = 1	/* Invoked after kprobe module callback */
 };
 
+static int count_symbols(void *data, unsigned long unused)
+{
+	unsigned int *count = data;
+
+	(*count)++;
+
+	return 0;
+}
+
+static unsigned int number_of_same_symbols(char *func_name)
+{
+	unsigned int count;
+
+	count = 0;
+	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
+
+	return count;
+}
+
 static int __trace_kprobe_create(int argc, const char *argv[])
 {
 	/*
@@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 		}
 	}
 
+	if (symbol) {
+		unsigned int count;
+
+		count = number_of_same_symbols(symbol);
+		if (count > 1) {
+			/*
+			 * Users should use ADDR to remove the ambiguity of
+			 * using KSYM only.
+			 */
+			ret = -EADDRNOTAVAIL;
+
+			goto error;
+		} else if (count == 0) {
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			ret = -ENOENT;
+
+			goto error;
+		}
+	}
+
 	trace_probe_log_set_index(0);
 	if (event) {
 		ret = traceprobe_parse_event_name(&event, &group, gbuf,
@@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
 }
 
 #ifdef CONFIG_PERF_EVENTS
+
 /* create a trace_kprobe, but don't add it to global lists */
 struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
@@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
 	int ret;
 	char *event;
 
+	if (func) {
+		unsigned int count;
+
+		count = number_of_same_symbols(func);
+		if (count > 1)
+			/*
+			 * Users should use addr to remove the ambiguity of
+			 * using func only.
+			 */
+			return ERR_PTR(-EADDRNOTAVAIL);
+		else if (count == 0)
+			/*
+			 * We can return ENOENT earlier than when register the
+			 * kprobe.
+			 */
+			return ERR_PTR(-ENOENT);
+	}
+
 	/*
 	 * local trace_kprobes are not added to dyn_event, so they are never
 	 * searched in find_trace_kprobe(). Therefore, there is no concern of
-- 
2.34.1

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Masami Hiramatsu]

On Thu, 24 Aug 2023 18:08:59 +0200
Francis Laniel <flaniel@linux.microsoft.com> wrote:

> Previously to this commit, if func matches several symbols, a kprobe, being
> either sysfs or PMU, would only be installed for the first matching address.
> This could lead to some misunderstanding when some BPF code was never called
> because it was attached to a function which was indeed not called, because
> the effectively called one has no kprobes attached.
> 
> So, this commit returns EADDRNOTAVAIL when func matches several symbols.
> This way, user needs to use address to remove the ambiguity.
> 
> Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
> Link: https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kernel.org/

Looks good to me!

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you!

> ---
>  kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 23dba01831f7..2f393739e8cf 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = {
>  	.priority = 1	/* Invoked after kprobe module callback */
>  };
>  
> +static int count_symbols(void *data, unsigned long unused)
> +{
> +	unsigned int *count = data;
> +
> +	(*count)++;
> +
> +	return 0;
> +}
> +
> +static unsigned int number_of_same_symbols(char *func_name)
> +{
> +	unsigned int count;
> +
> +	count = 0;
> +	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
> +
> +	return count;
> +}
> +
>  static int __trace_kprobe_create(int argc, const char *argv[])
>  {
>  	/*
> @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  		}
>  	}
>  
> +	if (symbol) {
> +		unsigned int count;
> +
> +		count = number_of_same_symbols(symbol);
> +		if (count > 1) {
> +			/*
> +			 * Users should use ADDR to remove the ambiguity of
> +			 * using KSYM only.
> +			 */
> +			ret = -EADDRNOTAVAIL;
> +
> +			goto error;
> +		} else if (count == 0) {
> +			/*
> +			 * We can return ENOENT earlier than when register the
> +			 * kprobe.
> +			 */
> +			ret = -ENOENT;
> +
> +			goto error;
> +		}
> +	}
> +
>  	trace_probe_log_set_index(0);
>  	if (event) {
>  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
>  }
>  
>  #ifdef CONFIG_PERF_EVENTS
> +
>  /* create a trace_kprobe, but don't add it to global lists */
>  struct trace_event_call *
>  create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
> @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
>  	int ret;
>  	char *event;
>  
> +	if (func) {
> +		unsigned int count;
> +
> +		count = number_of_same_symbols(func);
> +		if (count > 1)
> +			/*
> +			 * Users should use addr to remove the ambiguity of
> +			 * using func only.
> +			 */
> +			return ERR_PTR(-EADDRNOTAVAIL);
> +		else if (count == 0)
> +			/*
> +			 * We can return ENOENT earlier than when register the
> +			 * kprobe.
> +			 */
> +			return ERR_PTR(-ENOENT);
> +	}
> +
>  	/*
>  	 * local trace_kprobes are not added to dyn_event, so they are never
>  	 * searched in find_trace_kprobe(). Therefore, there is no concern of
> -- 
> 2.34.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Masami Hiramatsu]

On Thu, 24 Aug 2023 18:08:59 +0200
Francis Laniel <flaniel@linux.microsoft.com> wrote:

> Previously to this commit, if func matches several symbols, a kprobe, being
> either sysfs or PMU, would only be installed for the first matching address.
> This could lead to some misunderstanding when some BPF code was never called
> because it was attached to a function which was indeed not called, because
> the effectively called one has no kprobes attached.
> 
> So, this commit returns EADDRNOTAVAIL when func matches several symbols.
> This way, user needs to use address to remove the ambiguity.
> 
> Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
> Link: https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kernel.org/
> ---

Ah, this should be fine, but selftest (tools/testing/selftests/ftrace) fails.

 # tail 60-kprobe_module.tc-log.vsOHnF 
...
+ :
+ : 'Add an event on a module function without specifying event name'
+ :
+ echo 'p trace_printk:trace_printk_irq_work'
sh: write error: No such file or directory

Ah, the function on non-exist module should be checked too.

# tail 63-kprobe_syntax_errors.tc-log.mMLwIQ 
+ + printfwc '%s' -c
 'p '
+ pos=2
+ printf+  '%s'tr 'p ^non_exist_func'
 -d ^
+ command='p non_exist_func'
+ echo 'Test command: p non_exist_func'
Test command: p non_exist_func
+ echo
+ grep 'trace_kprobe: error:' -A 3 error_log

Also, this doesn't leave a syntax error message.

So, the below changes are needed.

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 8ab46a2a446d..1e57bc896952 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 		}
 	}
 
-	if (symbol) {
+	if (symbol && !strchr(symbol, ':')) {
 		unsigned int count;
 
 		count = number_of_same_symbols(symbol);
@@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 			 * Users should use ADDR to remove the ambiguity of
 			 * using KSYM only.
 			 */
+			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
 			ret = -EADDRNOTAVAIL;
 
 			goto error;
@@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
 			 * We can return ENOENT earlier than when register the
 			 * kprobe.
 			 */
+			trace_probe_log_err(0, BAD_PROBE_ADDR);
 			ret = -ENOENT;
 
 			goto error;
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 7f929482e8d4..a4f478448eef 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(BAD_MAXACT,		"Invalid maxactive number"),		\
 	C(MAXACT_TOO_BIG,	"Maxactive is too big"),		\
 	C(BAD_PROBE_ADDR,	"Invalid probed address or symbol"),	\
+	C(NON_UNIQ_SYMBOL,	"The symbol is not unique"),		\
 	C(BAD_RETPROBE,		"Retprobe address must be an function entry"), \
 	C(NO_TRACEPOINT,	"Tracepoint is not found"),		\
 	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \

Thank you,

>  kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 23dba01831f7..2f393739e8cf 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb = {
>  	.priority = 1	/* Invoked after kprobe module callback */
>  };
>  
> +static int count_symbols(void *data, unsigned long unused)
> +{
> +	unsigned int *count = data;
> +
> +	(*count)++;
> +
> +	return 0;
> +}
> +
> +static unsigned int number_of_same_symbols(char *func_name)
> +{
> +	unsigned int count;
> +
> +	count = 0;
> +	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
> +
> +	return count;
> +}
> +
>  static int __trace_kprobe_create(int argc, const char *argv[])
>  {
>  	/*
> @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  		}
>  	}
>  
> +	if (symbol) {
> +		unsigned int count;
> +
> +		count = number_of_same_symbols(symbol);
> +		if (count > 1) {
> +			/*
> +			 * Users should use ADDR to remove the ambiguity of
> +			 * using KSYM only.
> +			 */

			

> +			ret = -EADDRNOTAVAIL;
> +
> +			goto error;
> +		} else if (count == 0) {
> +			/*
> +			 * We can return ENOENT earlier than when register the
> +			 * kprobe.
> +			 */
> +			ret = -ENOENT;
> +
> +			goto error;
> +		}
> +	}
> +
>  	trace_probe_log_set_index(0);
>  	if (event) {
>  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
>  }
>  
>  #ifdef CONFIG_PERF_EVENTS
> +
>  /* create a trace_kprobe, but don't add it to global lists */
>  struct trace_event_call *
>  create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
> @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
>  	int ret;
>  	char *event;
>  
> +	if (func) {
> +		unsigned int count;
> +
> +		count = number_of_same_symbols(func);
> +		if (count > 1)
> +			/*
> +			 * Users should use addr to remove the ambiguity of
> +			 * using func only.
> +			 */
> +			return ERR_PTR(-EADDRNOTAVAIL);
> +		else if (count == 0)
> +			/*
> +			 * We can return ENOENT earlier than when register the
> +			 * kprobe.
> +			 */
> +			return ERR_PTR(-ENOENT);
> +	}
> +
>  	/*
>  	 * local trace_kprobes are not added to dyn_event, so they are never
>  	 * searched in find_trace_kprobe(). Therefore, there is no concern of
> -- 
> 2.34.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Francis Laniel]

Hi.

Le vendredi 25 août 2023, 14:16:49 CEST Masami Hiramatsu a écrit :
> On Thu, 24 Aug 2023 18:08:59 +0200
> 
> Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > Previously to this commit, if func matches several symbols, a kprobe,
> > being
> > either sysfs or PMU, would only be installed for the first matching
> > address. This could lead to some misunderstanding when some BPF code was
> > never called because it was attached to a function which was indeed not
> > called, because the effectively called one has no kprobes attached.
> > 
> > So, this commit returns EADDRNOTAVAIL when func matches several symbols.
> > This way, user needs to use address to remove the ambiguity.
> > 
> > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
> > Link:
> > https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kern
> > el.org/ ---
> 
> Ah, this should be fine, but selftest (tools/testing/selftests/ftrace)
> fails.
> 
>  # tail 60-kprobe_module.tc-log.vsOHnF
> ...
> + :
> + : 'Add an event on a module function without specifying event name'
> + :
> + echo 'p trace_printk:trace_printk_irq_work'
> sh: write error: No such file or directory
> 
> Ah, the function on non-exist module should be checked too.
> 
> # tail 63-kprobe_syntax_errors.tc-log.mMLwIQ
> + + printfwc '%s' -c
>  'p '
> + pos=2
> + printf+  '%s'tr 'p ^non_exist_func'
>  -d ^
> + command='p non_exist_func'
> + echo 'Test command: p non_exist_func'
> Test command: p non_exist_func
> + echo
> + grep 'trace_kprobe: error:' -A 3 error_log
> 
> Also, this doesn't leave a syntax error message.
> 
> So, the below changes are needed.

Excellent catch! Thank you, I will apply this patch and send v4 right after.
Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?
Maybe it should go inside LTP? As this would need having a kernel compiled 
with a name pointing to several symbols?

Also, should some man pages somewhere be updated to reflect the case kprobe can 
return EADDRNOTAVAIL?

> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 8ab46a2a446d..1e57bc896952 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const char
> *argv[]) }
>  	}
> 
> -	if (symbol) {
> +	if (symbol && !strchr(symbol, ':')) {
>  		unsigned int count;
> 
>  		count = number_of_same_symbols(symbol);
> @@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const char
> *argv[]) * Users should use ADDR to remove the ambiguity of
>  			 * using KSYM only.
>  			 */
> +			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
>  			ret = -EADDRNOTAVAIL;
> 
>  			goto error;
> @@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const char
> *argv[]) * We can return ENOENT earlier than when register the
>  			 * kprobe.
>  			 */
> +			trace_probe_log_err(0, BAD_PROBE_ADDR);
>  			ret = -ENOENT;
> 
>  			goto error;
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 7f929482e8d4..a4f478448eef 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct
> trace_event_call *event_call, C(BAD_MAXACT,		"Invalid maxactive
> number"),		\
>  	C(MAXACT_TOO_BIG,	"Maxactive is too big"),		\
>  	C(BAD_PROBE_ADDR,	"Invalid probed address or symbol"),	\
> +	C(NON_UNIQ_SYMBOL,	"The symbol is not unique"),		\
>  	C(BAD_RETPROBE,		"Retprobe address must be an function 
entry"), \
>  	C(NO_TRACEPOINT,	"Tracepoint is not found"),		\
>  	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \
> 
> Thank you,
> 
> >  kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 23dba01831f7..2f393739e8cf 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb =
> > {> 
> >  	.priority = 1	/* Invoked after kprobe module callback */
> >  
> >  };
> > 
> > +static int count_symbols(void *data, unsigned long unused)
> > +{
> > +	unsigned int *count = data;
> > +
> > +	(*count)++;
> > +
> > +	return 0;
> > +}
> > +
> > +static unsigned int number_of_same_symbols(char *func_name)
> > +{
> > +	unsigned int count;
> > +
> > +	count = 0;
> > +	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
> > +
> > +	return count;
> > +}
> > +
> > 
> >  static int __trace_kprobe_create(int argc, const char *argv[])
> >  {
> >  
> >  	/*
> > 
> > @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char
> > *argv[])> 
> >  		}
> >  	
> >  	}
> > 
> > +	if (symbol) {
> > +		unsigned int count;
> > +
> > +		count = number_of_same_symbols(symbol);
> > +		if (count > 1) {
> > +			/*
> > +			 * Users should use ADDR to remove the ambiguity of
> > +			 * using KSYM only.
> > +			 */
> > 
> > 
> > 
> > +			ret = -EADDRNOTAVAIL;
> > +
> > +			goto error;
> > +		} else if (count == 0) {
> > +			/*
> > +			 * We can return ENOENT earlier than when register the
> > +			 * kprobe.
> > +			 */
> > +			ret = -ENOENT;
> > +
> > +			goto error;
> > +		}
> > +	}
> > +
> > 
> >  	trace_probe_log_set_index(0);
> >  	if (event) {
> >  	
> >  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> > 
> > @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct
> > trace_kprobe *tk)> 
> >  }
> >  
> >  #ifdef CONFIG_PERF_EVENTS
> > 
> > +
> > 
> >  /* create a trace_kprobe, but don't add it to global lists */
> >  struct trace_event_call *
> >  create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
> > 
> > @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr,
> > unsigned long offs,> 
> >  	int ret;
> >  	char *event;
> > 
> > +	if (func) {
> > +		unsigned int count;
> > +
> > +		count = number_of_same_symbols(func);
> > +		if (count > 1)
> > +			/*
> > +			 * Users should use addr to remove the ambiguity of
> > +			 * using func only.
> > +			 */
> > +			return ERR_PTR(-EADDRNOTAVAIL);
> > +		else if (count == 0)
> > +			/*
> > +			 * We can return ENOENT earlier than when register the
> > +			 * kprobe.
> > +			 */
> > +			return ERR_PTR(-ENOENT);
> > +	}
> > +
> > 
> >  	/*
> >  	
> >  	 * local trace_kprobes are not added to dyn_event, so they are never
> >  	 * searched in find_trace_kprobe(). Therefore, there is no concern of

Best regards.

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Masami Hiramatsu]

On Fri, 25 Aug 2023 14:34:49 +0200
Francis Laniel <flaniel@linux.microsoft.com> wrote:

> Hi.
> 
> Le vendredi 25 août 2023, 14:16:49 CEST Masami Hiramatsu a écrit :
> > On Thu, 24 Aug 2023 18:08:59 +0200
> > 
> > Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > > Previously to this commit, if func matches several symbols, a kprobe,
> > > being
> > > either sysfs or PMU, would only be installed for the first matching
> > > address. This could lead to some misunderstanding when some BPF code was
> > > never called because it was attached to a function which was indeed not
> > > called, because the effectively called one has no kprobes attached.
> > > 
> > > So, this commit returns EADDRNOTAVAIL when func matches several symbols.
> > > This way, user needs to use address to remove the ambiguity.
> > > 
> > > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
> > > Link:
> > > https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@kern
> > > el.org/ ---
> > 
> > Ah, this should be fine, but selftest (tools/testing/selftests/ftrace)
> > fails.
> > 
> >  # tail 60-kprobe_module.tc-log.vsOHnF
> > ...
> > + :
> > + : 'Add an event on a module function without specifying event name'
> > + :
> > + echo 'p trace_printk:trace_printk_irq_work'
> > sh: write error: No such file or directory
> > 
> > Ah, the function on non-exist module should be checked too.
> > 
> > # tail 63-kprobe_syntax_errors.tc-log.mMLwIQ
> > + + printfwc '%s' -c
> >  'p '
> > + pos=2
> > + printf+  '%s'tr 'p ^non_exist_func'
> >  -d ^
> > + command='p non_exist_func'
> > + echo 'Test command: p non_exist_func'
> > Test command: p non_exist_func
> > + echo
> > + grep 'trace_kprobe: error:' -A 3 error_log
> > 
> > Also, this doesn't leave a syntax error message.
> > 
> > So, the below changes are needed.
> 
> Excellent catch! Thank you, I will apply this patch and send v4 right after.
> Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?

Hmm, in that case, you need to change something in tracefs/README so that
we can identify the kernel has different behavior. Or we have to change
this is a "Fix" for backporting.

> Maybe it should go inside LTP? As this would need having a kernel compiled 
> with a name pointing to several symbols?

For this tracing feature, I rather like to use tools/testing/selftests/ftrace
to test it. And it is used on all stable kernel, that is why we need to add
some change on tracefs/README or something.

But I would like to wait for Alessandro's work. After his work, in this time
we need to probe all the same-name symbols as your original patch does.
This is because 1:n mapping can happen as Alessandro pointed in

https://lore.kernel.org/all/CAPp5cGQsRdB0+KHR1wX2bDDdc5sTzSNPA417PNJb0ypmV=yS6w@mail.gmail.com/

But if his feature is configurable (and maybe so), we need to keep this
version... We have many options.

- this normal kallsyms: the same-name symbols should not be used.
- enhanced kallsyms (if 1:n symbol has the same suffix): the same name symbols
   should be probed at once.
- enhanced kallsyms (if 1:n symbol has different suffix): the same-name symbol
   must not exist.


> 
> Also, should some man pages somewhere be updated to reflect the case kprobe can 
> return EADDRNOTAVAIL?

No, it is a tracefs interface and we don't have man pages yet.

Thank you,

> 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 8ab46a2a446d..1e57bc896952 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const char
> > *argv[]) }
> >  	}
> > 
> > -	if (symbol) {
> > +	if (symbol && !strchr(symbol, ':')) {
> >  		unsigned int count;
> > 
> >  		count = number_of_same_symbols(symbol);
> > @@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const char
> > *argv[]) * Users should use ADDR to remove the ambiguity of
> >  			 * using KSYM only.
> >  			 */
> > +			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> >  			ret = -EADDRNOTAVAIL;
> > 
> >  			goto error;
> > @@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const char
> > *argv[]) * We can return ENOENT earlier than when register the
> >  			 * kprobe.
> >  			 */
> > +			trace_probe_log_err(0, BAD_PROBE_ADDR);
> >  			ret = -ENOENT;
> > 
> >  			goto error;
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 7f929482e8d4..a4f478448eef 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct
> > trace_event_call *event_call, C(BAD_MAXACT,		"Invalid maxactive
> > number"),		\
> >  	C(MAXACT_TOO_BIG,	"Maxactive is too big"),		\
> >  	C(BAD_PROBE_ADDR,	"Invalid probed address or symbol"),	\
> > +	C(NON_UNIQ_SYMBOL,	"The symbol is not unique"),		\
> >  	C(BAD_RETPROBE,		"Retprobe address must be an function 
> entry"), \
> >  	C(NO_TRACEPOINT,	"Tracepoint is not found"),		\
> >  	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \
> > 
> > Thank you,
> > 
> > >  kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > > 
> > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > index 23dba01831f7..2f393739e8cf 100644
> > > --- a/kernel/trace/trace_kprobe.c
> > > +++ b/kernel/trace/trace_kprobe.c
> > > @@ -705,6 +705,25 @@ static struct notifier_block trace_kprobe_module_nb =
> > > {> 
> > >  	.priority = 1	/* Invoked after kprobe module callback */
> > >  
> > >  };
> > > 
> > > +static int count_symbols(void *data, unsigned long unused)
> > > +{
> > > +	unsigned int *count = data;
> > > +
> > > +	(*count)++;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static unsigned int number_of_same_symbols(char *func_name)
> > > +{
> > > +	unsigned int count;
> > > +
> > > +	count = 0;
> > > +	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
> > > +
> > > +	return count;
> > > +}
> > > +
> > > 
> > >  static int __trace_kprobe_create(int argc, const char *argv[])
> > >  {
> > >  
> > >  	/*
> > > 
> > > @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const char
> > > *argv[])> 
> > >  		}
> > >  	
> > >  	}
> > > 
> > > +	if (symbol) {
> > > +		unsigned int count;
> > > +
> > > +		count = number_of_same_symbols(symbol);
> > > +		if (count > 1) {
> > > +			/*
> > > +			 * Users should use ADDR to remove the ambiguity of
> > > +			 * using KSYM only.
> > > +			 */
> > > 
> > > 
> > > 
> > > +			ret = -EADDRNOTAVAIL;
> > > +
> > > +			goto error;
> > > +		} else if (count == 0) {
> > > +			/*
> > > +			 * We can return ENOENT earlier than when register the
> > > +			 * kprobe.
> > > +			 */
> > > +			ret = -ENOENT;
> > > +
> > > +			goto error;
> > > +		}
> > > +	}
> > > +
> > > 
> > >  	trace_probe_log_set_index(0);
> > >  	if (event) {
> > >  	
> > >  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> > > 
> > > @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct
> > > trace_kprobe *tk)> 
> > >  }
> > >  
> > >  #ifdef CONFIG_PERF_EVENTS
> > > 
> > > +
> > > 
> > >  /* create a trace_kprobe, but don't add it to global lists */
> > >  struct trace_event_call *
> > >  create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
> > > 
> > > @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void *addr,
> > > unsigned long offs,> 
> > >  	int ret;
> > >  	char *event;
> > > 
> > > +	if (func) {
> > > +		unsigned int count;
> > > +
> > > +		count = number_of_same_symbols(func);
> > > +		if (count > 1)
> > > +			/*
> > > +			 * Users should use addr to remove the ambiguity of
> > > +			 * using func only.
> > > +			 */
> > > +			return ERR_PTR(-EADDRNOTAVAIL);
> > > +		else if (count == 0)
> > > +			/*
> > > +			 * We can return ENOENT earlier than when register the
> > > +			 * kprobe.
> > > +			 */
> > > +			return ERR_PTR(-ENOENT);
> > > +	}
> > > +
> > > 
> > >  	/*
> > >  	
> > >  	 * local trace_kprobes are not added to dyn_event, so they are never
> > >  	 * searched in find_trace_kprobe(). Therefore, there is no concern of
> 
> Best regards.
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Francis Laniel]

Le vendredi 25 août 2023, 15:13:21 CEST Masami Hiramatsu a écrit :
> On Fri, 25 Aug 2023 14:34:49 +0200
> 
> Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > Hi.
> > 
> > Le vendredi 25 août 2023, 14:16:49 CEST Masami Hiramatsu a écrit :
> > > On Thu, 24 Aug 2023 18:08:59 +0200
> > > 
> > > Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > > > Previously to this commit, if func matches several symbols, a kprobe,
> > > > being
> > > > either sysfs or PMU, would only be installed for the first matching
> > > > address. This could lead to some misunderstanding when some BPF code
> > > > was
> > > > never called because it was attached to a function which was indeed
> > > > not
> > > > called, because the effectively called one has no kprobes attached.
> > > > 
> > > > So, this commit returns EADDRNOTAVAIL when func matches several
> > > > symbols.
> > > > This way, user needs to use address to remove the ambiguity.
> > > > 
> > > > Suggested-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
> > > > Link:
> > > > https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@k
> > > > ern
> > > > el.org/ ---
> > > 
> > > Ah, this should be fine, but selftest (tools/testing/selftests/ftrace)
> > > fails.
> > > 
> > >  # tail 60-kprobe_module.tc-log.vsOHnF
> > > 
> > > ...
> > > + :
> > > + : 'Add an event on a module function without specifying event name'
> > > + :
> > > + echo 'p trace_printk:trace_printk_irq_work'
> > > sh: write error: No such file or directory
> > > 
> > > Ah, the function on non-exist module should be checked too.
> > > 
> > > # tail 63-kprobe_syntax_errors.tc-log.mMLwIQ
> > > + + printfwc '%s' -c
> > > 
> > >  'p '
> > > 
> > > + pos=2
> > > + printf+  '%s'tr 'p ^non_exist_func'
> > > 
> > >  -d ^
> > > 
> > > + command='p non_exist_func'
> > > + echo 'Test command: p non_exist_func'
> > > Test command: p non_exist_func
> > > + echo
> > > + grep 'trace_kprobe: error:' -A 3 error_log
> > > 
> > > Also, this doesn't leave a syntax error message.
> > > 
> > > So, the below changes are needed.
> > 
> > Excellent catch! Thank you, I will apply this patch and send v4 right
> > after. Regarding test, do you think I can add a test for the
> > EADDRNOTAVAIL case?
> Hmm, in that case, you need to change something in tracefs/README so that
> we can identify the kernel has different behavior. Or we have to change
> this is a "Fix" for backporting.

Oops, sorry I sent the v4 with a test but as a separated commit, so we can 
just ignore it for the moment.

> > Maybe it should go inside LTP? As this would need having a kernel compiled
> > with a name pointing to several symbols?
> 
> For this tracing feature, I rather like to use
> tools/testing/selftests/ftrace to test it. And it is used on all stable
> kernel, that is why we need to add some change on tracefs/README or
> something.
> 
> But I would like to wait for Alessandro's work. After his work, in this time
> we need to probe all the same-name symbols as your original patch does.
> This is because 1:n mapping can happen as Alessandro pointed in
> 
> https://lore.kernel.org/all/CAPp5cGQsRdB0+KHR1wX2bDDdc5sTzSNPA417PNJb0ypmV=y
> S6w@mail.gmail.com/
> 
> But if his feature is configurable (and maybe so), we need to keep this
> version... We have many options.
> 
> - this normal kallsyms: the same-name symbols should not be used.
> - enhanced kallsyms (if 1:n symbol has the same suffix): the same name
> symbols should be probed at once.
> - enhanced kallsyms (if 1:n symbol has different suffix): the same-name
> symbol must not exist.

I understand!
In future case, we could still have a test and change its behavior (i.e. 
potentially skipping it) when KALLSYMS_ALIAS is set.

> > Also, should some man pages somewhere be updated to reflect the case
> > kprobe can return EADDRNOTAVAIL?
> 
> No, it is a tracefs interface and we don't have man pages yet.

I was more thinking to the PMU counterpart as it is created through 
perf_event_open()?

> Thank you,
> 
> > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > index 8ab46a2a446d..1e57bc896952 100644
> > > --- a/kernel/trace/trace_kprobe.c
> > > +++ b/kernel/trace/trace_kprobe.c
> > > @@ -855,7 +855,7 @@ static int __trace_kprobe_create(int argc, const
> > > char
> > > *argv[]) }
> > > 
> > >  	}
> > > 
> > > -	if (symbol) {
> > > +	if (symbol && !strchr(symbol, ':')) {
> > > 
> > >  		unsigned int count;
> > >  		
> > >  		count = number_of_same_symbols(symbol);
> > > 
> > > @@ -864,6 +864,7 @@ static int __trace_kprobe_create(int argc, const
> > > char
> > > *argv[]) * Users should use ADDR to remove the ambiguity of
> > > 
> > >  			 * using KSYM only.
> > >  			 */
> > > 
> > > +			trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> > > 
> > >  			ret = -EADDRNOTAVAIL;
> > >  			
> > >  			goto error;
> > > 
> > > @@ -872,6 +873,7 @@ static int __trace_kprobe_create(int argc, const
> > > char
> > > *argv[]) * We can return ENOENT earlier than when register the
> > > 
> > >  			 * kprobe.
> > >  			 */
> > > 
> > > +			trace_probe_log_err(0, BAD_PROBE_ADDR);
> > > 
> > >  			ret = -ENOENT;
> > >  			
> > >  			goto error;
> > > 
> > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > > index 7f929482e8d4..a4f478448eef 100644
> > > --- a/kernel/trace/trace_probe.h
> > > +++ b/kernel/trace/trace_probe.h
> > > @@ -450,6 +450,7 @@ extern int traceprobe_define_arg_fields(struct
> > > trace_event_call *event_call, C(BAD_MAXACT,		"Invalid maxactive
> > > number"),		\
> > > 
> > >  	C(MAXACT_TOO_BIG,	"Maxactive is too big"),		\
> > >  	C(BAD_PROBE_ADDR,	"Invalid probed address or symbol"),	\
> > > 
> > > +	C(NON_UNIQ_SYMBOL,	"The symbol is not unique"),		\
> > > 
> > >  	C(BAD_RETPROBE,		"Retprobe address must be an function
> > 
> > entry"), \
> > 
> > >  	C(NO_TRACEPOINT,	"Tracepoint is not found"),		\
> > >  	C(BAD_ADDR_SUFFIX,	"Invalid probed address suffix"), \
> > > 
> > > Thank you,
> > > 
> > > >  kernel/trace/trace_kprobe.c | 61
> > > >  +++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 61 insertions(+)
> > > > 
> > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > > > index 23dba01831f7..2f393739e8cf 100644
> > > > --- a/kernel/trace/trace_kprobe.c
> > > > +++ b/kernel/trace/trace_kprobe.c
> > > > @@ -705,6 +705,25 @@ static struct notifier_block
> > > > trace_kprobe_module_nb =
> > > > {>
> > > > 
> > > >  	.priority = 1	/* Invoked after kprobe module callback */
> > > >  
> > > >  };
> > > > 
> > > > +static int count_symbols(void *data, unsigned long unused)
> > > > +{
> > > > +	unsigned int *count = data;
> > > > +
> > > > +	(*count)++;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static unsigned int number_of_same_symbols(char *func_name)
> > > > +{
> > > > +	unsigned int count;
> > > > +
> > > > +	count = 0;
> > > > +	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
> > > > +
> > > > +	return count;
> > > > +}
> > > > +
> > > > 
> > > >  static int __trace_kprobe_create(int argc, const char *argv[])
> > > >  {
> > > >  
> > > >  	/*
> > > > 
> > > > @@ -836,6 +855,29 @@ static int __trace_kprobe_create(int argc, const
> > > > char
> > > > *argv[])>
> > > > 
> > > >  		}
> > > >  	
> > > >  	}
> > > > 
> > > > +	if (symbol) {
> > > > +		unsigned int count;
> > > > +
> > > > +		count = number_of_same_symbols(symbol);
> > > > +		if (count > 1) {
> > > > +			/*
> > > > +			 * Users should use ADDR to remove the ambiguity of
> > > > +			 * using KSYM only.
> > > > +			 */
> > > > 
> > > > 
> > > > 
> > > > +			ret = -EADDRNOTAVAIL;
> > > > +
> > > > +			goto error;
> > > > +		} else if (count == 0) {
> > > > +			/*
> > > > +			 * We can return ENOENT earlier than when register the
> > > > +			 * kprobe.
> > > > +			 */
> > > > +			ret = -ENOENT;
> > > > +
> > > > +			goto error;
> > > > +		}
> > > > +	}
> > > > +
> > > > 
> > > >  	trace_probe_log_set_index(0);
> > > >  	if (event) {
> > > >  	
> > > >  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> > > > 
> > > > @@ -1699,6 +1741,7 @@ static int unregister_kprobe_event(struct
> > > > trace_kprobe *tk)>
> > > > 
> > > >  }
> > > >  
> > > >  #ifdef CONFIG_PERF_EVENTS
> > > > 
> > > > +
> > > > 
> > > >  /* create a trace_kprobe, but don't add it to global lists */
> > > >  struct trace_event_call *
> > > >  create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
> > > > 
> > > > @@ -1709,6 +1752,24 @@ create_local_trace_kprobe(char *func, void
> > > > *addr,
> > > > unsigned long offs,>
> > > > 
> > > >  	int ret;
> > > >  	char *event;
> > > > 
> > > > +	if (func) {
> > > > +		unsigned int count;
> > > > +
> > > > +		count = number_of_same_symbols(func);
> > > > +		if (count > 1)
> > > > +			/*
> > > > +			 * Users should use addr to remove the ambiguity of
> > > > +			 * using func only.
> > > > +			 */
> > > > +			return ERR_PTR(-EADDRNOTAVAIL);
> > > > +		else if (count == 0)
> > > > +			/*
> > > > +			 * We can return ENOENT earlier than when register the
> > > > +			 * kprobe.
> > > > +			 */
> > > > +			return ERR_PTR(-ENOENT);
> > > > +	}
> > > > +
> > > > 
> > > >  	/*
> > > >  	
> > > >  	 * local trace_kprobes are not added to dyn_event, so they are never
> > > >  	 * searched in find_trace_kprobe(). Therefore, there is no concern
> > > >  	 of
> > 
> > Best regards.

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Steven Rostedt]

On Fri, 25 Aug 2023 22:13:21 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > Excellent catch! Thank you, I will apply this patch and send v4 right after.
> > Regarding test, do you think I can add a test for the EADDRNOTAVAIL case?  
> 
> Hmm, in that case, you need to change something in tracefs/README so that
> we can identify the kernel has different behavior. Or we have to change
> this is a "Fix" for backporting.

I prefer this to be a Fix and backported.

Thanks,

-- Steve

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Francis Laniel]

Hi.

Le mercredi 30 août 2023, 01:57:19 CEST Steven Rostedt a écrit :
> On Fri, 25 Aug 2023 22:13:21 +0900
> 
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > > Excellent catch! Thank you, I will apply this patch and send v4 right
> > > after. Regarding test, do you think I can add a test for the
> > > EADDRNOTAVAIL case?> 
> > Hmm, in that case, you need to change something in tracefs/README so that
> > we can identify the kernel has different behavior. Or we have to change
> > this is a "Fix" for backporting.
> 
> I prefer this to be a Fix and backported.

This makes sense, I will send v5 to stable mailing list too!
 
> Thanks,
> 
> -- Steve

Best regards.

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Masami Hiramatsu]

Hi Francis,

On Thu, 31 Aug 2023 09:14:55 +0200
Francis Laniel <flaniel@linux.microsoft.com> wrote:

> Hi.
> 
> Le mercredi 30 août 2023, 01:57:19 CEST Steven Rostedt a écrit :
> > On Fri, 25 Aug 2023 22:13:21 +0900
> > 
> > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > > > Excellent catch! Thank you, I will apply this patch and send v4 right
> > > > after. Regarding test, do you think I can add a test for the
> > > > EADDRNOTAVAIL case?> 
> > > Hmm, in that case, you need to change something in tracefs/README so that
> > > we can identify the kernel has different behavior. Or we have to change
> > > this is a "Fix" for backporting.
> > 
> > I prefer this to be a Fix and backported.
> 
> This makes sense, I will send v5 to stable mailing list too!

I missed this a while. did you send v5 ? I could not find in my mbox.

Thank you,

>  
> > Thanks,
> > 
> > -- Steve
> 
> Best regards.
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

[Francis Laniel]

Hi!

Le mercredi 18 octobre 2023, 09:30:20 EEST Masami Hiramatsu a écrit :
> Hi Francis,
> 
> On Thu, 31 Aug 2023 09:14:55 +0200
> 
> Francis Laniel <flaniel@linux.microsoft.com> wrote:
> > Hi.
> > 
> > Le mercredi 30 août 2023, 01:57:19 CEST Steven Rostedt a écrit :
> > > On Fri, 25 Aug 2023 22:13:21 +0900
> > > 
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > > > > Excellent catch! Thank you, I will apply this patch and send v4
> > > > > right
> > > > > after. Regarding test, do you think I can add a test for the
> > > > > EADDRNOTAVAIL case?>
> > > > 
> > > > Hmm, in that case, you need to change something in tracefs/README so
> > > > that
> > > > we can identify the kernel has different behavior. Or we have to
> > > > change
> > > > this is a "Fix" for backporting.
> > > 
> > > I prefer this to be a Fix and backported.
> > 
> > This makes sense, I will send v5 to stable mailing list too!
> 
> I missed this a while. did you send v5 ? I could not find in my mbox.

Sorry, I took a bit of time before sending the v5 as I wanted to wait for 
Alessandro patchset to be merged first.
As it seems more work is needed on his contribution I think we can go with 
this fix first.

> Thank you,
> 
> > > Thanks,
> > > 
> > > -- Steve
> > 
> > Best regards.

Best regards.