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

Commit Message

Francis Laniel Aug. 24, 2023, 4:08 p.m. UTC

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 <[e-mail]>
Signed-off-by: Francis Laniel <[e-mail]>
Link: https://lore.kernel.org/lkml/[e-mail]/
---
 kernel/trace/trace_kprobe.c | 61 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

Comments

Masami Hiramatsu (Google) Aug. 25, 2023, 2:46 a.m. UTC | #1

On Thu, 24 Aug 2023 18:08:59 +0200
Francis Laniel <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Francis Laniel <[e-mail]>
> Link: https://lore.kernel.org/lkml/[e-mail]/

Looks good to me!

Acked-by: Masami Hiramatsu (Google) <[e-mail]>

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) Aug. 25, 2023, 12:16 p.m. UTC | #2

On Thu, 24 Aug 2023 18:08:59 +0200
Francis Laniel <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Francis Laniel <[e-mail]>
> Link: https://lore.kernel.org/lkml/[e-mail]/
> ---

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
>

Francis Laniel Aug. 25, 2023, 12:34 p.m. UTC | #3

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 <[e-mail]> 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 <[e-mail]>
> > Signed-off-by: Francis Laniel <[e-mail]>
> > 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.

Masami Hiramatsu (Google) Aug. 25, 2023, 1:13 p.m. UTC | #4

On Fri, 25 Aug 2023 14:34:49 +0200
Francis Laniel <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > Signed-off-by: Francis Laniel <[e-mail]>
> > > 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=[e-mail]/

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.
> 
>

Francis Laniel Aug. 25, 2023, 2:14 p.m. UTC | #5

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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > > Signed-off-by: Francis Laniel <[e-mail]>
> > > > 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
> [e-mail]/
> 
> 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.

Steven Rostedt Aug. 29, 2023, 11:57 p.m. UTC | #6

On Fri, 25 Aug 2023 22:13:21 +0900
Masami Hiramatsu (Google) <[e-mail]> 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

Francis Laniel Aug. 31, 2023, 7:14 a.m. UTC | #7

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) <[e-mail]> 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.

Masami Hiramatsu (Google) Oct. 18, 2023, 6:30 a.m. UTC | #8

Hi Francis,

On Thu, 31 Aug 2023 09:14:55 +0200
Francis Laniel <[e-mail]> 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) <[e-mail]> 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.
> 
>

Francis Laniel Oct. 18, 2023, 2:43 p.m. UTC | #9

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 <[e-mail]> 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) <[e-mail]> 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.

Patch

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