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

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

Hi.

Le jeudi 24 août 2023, 15:02:27 CEST Masami Hiramatsu a écrit :
> On Thu, 24 Aug 2023 12:37:34 +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
> > call, because the effectively called one has no kprobes.
> > 
> > So, this commit returns EADDRNOTAVAIL when func matches several symbols.
> > This way, user needs to use addr to remove the ambiguity.
> 
> Thanks for update the patch. I have some comments there.
> 
> > 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/ ---
> > 
> >  kernel/trace/trace_kprobe.c | 42 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 23dba01831f7..0c8dd6ba650b 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 func_name_several_symbols(char *func_name)
> 
> If this returns boolean, please use 'bool' for return type.
> Also, I think 'func_name_is_unique()' is more natural.
> 

This name sounds better but it means it will check count == 1.
I am fine with it, but please see my below comment.

> > +{
> > +	unsigned int count;
> > +
> > +	count = 0;
> > +	kallsyms_on_each_match_symbol(count_symbols, func_name, &count);
> > +
> > +	return count > 1;
> > +}
> > +
> > 
> >  static int __trace_kprobe_create(int argc, const char *argv[])
> >  {
> >  
> >  	/*
> > 
> > @@ -836,6 +855,18 @@ static int __trace_kprobe_create(int argc, const char
> > *argv[])> 
> >  		}
> >  	
> >  	}
> > 
> > +	/*
> > +	 * If user specifies KSYM, we check it does not correspond to several
> > +	 * symbols.
> > +	 * If this is the case, we return EADDRNOTAVAIL to indicate the user
> > +	 * he/she should use ADDR rather than KSYM to remove the ambiguity.
> > +	 */
> > +	if (symbol && func_name_several_symbols(symbol)) {
> 
> Then, here  will be
> 
> 	if (symbol && !func_name_is_unique(symbol)) {
> 

I am fine with the above, but it means if users gives an unknown symbol, we 
will return EADDRNOTAVAIL instead of currently returning ENOENT.
Is it OK?

> > +		ret = -EADDRNOTAVAIL;
> > +
> > +		goto error;
> > +	}
> > +
> > 
> >  	trace_probe_log_set_index(0);
> >  	if (event) {
> >  	
> >  		ret = traceprobe_parse_event_name(&event, &group, gbuf,
> > 
> > @@ -1699,6 +1730,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 +1741,16 @@ create_local_trace_kprobe(char *func, void *addr,
> > unsigned long offs,> 
> >  	int ret;
> >  	char *event;
> > 
> > +	/*
> > +	 * If user specifies func, we check that function name does not
> > +	 * correspond to several symbols.
> > +	 * If this is the case, we return EADDRNOTAVAIL to indicate the user
> > +	 * he/she should use addr and offs rather than func to remove the
> > +	 * ambiguity.
> > +	 */
> > +	if (func && func_name_several_symbols(func))
> 
> Ditto.
> 
> Thanks!
> 
> > +		return ERR_PTR(-EADDRNOTAVAIL);
> > +
> > 
> >  	/*
> >  	
> >  	 * 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.