Message-ID: <1862303.tdWV9SEqCh@pwmachine>
Date:   Fri, 25 Aug 2023 16:14:25 +0200
From:   Francis Laniel <flaniel@...ux.microsoft.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v3 1/1] tracing/kprobes: Return EADDRNOTAVAIL when func matches several symbols

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 <flaniel@...ux.microsoft.com> 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 <flaniel@...ux.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@...nel.org>
> > > > Signed-off-by: Francis Laniel <flaniel@...ux.microsoft.com>
> > > > 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
> S6w@...l.gmail.com/
> 
> 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.

Powered by blists - more mailing lists