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

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

On Thu, 24 Aug 2023 16:31:13 +0200
Francis Laniel <flaniel@linux.microsoft.com> 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@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?

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;
}

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


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