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

[Francis Laniel <flaniel@linux.microsoft.com>]

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.