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

[Francis Laniel]

Le jeudi 24 août 2023, 16:47:21 CEST Masami Hiramatsu a écrit :
> On Thu, 24 Aug 2023 16:31:13 +0200
> 
> Francis Laniel <flaniel@xxxxxxxxxxxxxxxxxxx> wrote:
> > 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > > Signed-off-by: Francis Laniel <flaniel@xxxxxxxxxxxxxxxxxxx>
> > > > Link:
> > > > https://lore.kernel.org/lkml/20230819101105.b0c104ae4494a7d1f2eea742@k
> > > > ern
> > > > 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?
> 
> Ah, good catch! Hm, then what about 'int number_of_same_symbols()' ?
> 
> 
> if (symbol) {
> 	num = number_of_same_symbols(symbol);
> 	if (num > 1)
> 		return -EADDRNOTAVAIL;
> 	else if (num == 0)
> 		return -ENOENT;
> }

Done in the v3 :D!

> Thank you,
> 
> > > > +		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.