Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events

[Masami Hiramatsu <mhiramat@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4067 bytes --]

Hi Sriker and Steve,

Sorry for later, I was in a conference.

On Fri, 27 Sep 2019 19:08:53 +0530
Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> <SNIP>
> 
> > The cause was that the args array to compare between two probe events only
> > looked at one of the probe events size. If the other one had a smaller
> > number of args, it would read out of bounds memory.
> > 
> 
> I thought trace_probe_compare_arg_type() should have caught this. But looks
> like there is one case it misses. 
> 
> Currently trace_probe_compare_arg_type() is okay if the newer probe has
> lesser or equal arguments than the older probe. For all other cases, it
> seems to error out. In this case, we must be hitting where the newer probe
> has lesser arguments than older probe.
> 
> 
> > Fixes: fe60b0ce8e733 ("tracing/probe: Reject exactly same probe event")
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> >  kernel/trace/trace_kprobe.c | 2 ++
> >  kernel/trace/trace_uprobe.c | 2 ++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 402dc3ce88d3..d2543a403f25 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -537,6 +537,8 @@ static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig,
> > 
> >  	list_for_each_entry(pos, &tpe->probes, list) {
> >  		orig = container_of(pos, struct trace_kprobe, tp);
> > +		if (orig->tp.nr_args != comp->tp.nr_args)
> > +			continue;
> 
> This has a side-effect where the newer probe has same argument commands, we
> still end up appending the probe.
> 
> Lets says we already have a probe with 2 arguments, the newer probe has only
> the first argument but same fetch command, we should have erred out.
> No?

Correct.

> 
> 
> >  		if (strcmp(trace_kprobe_symbol(orig),
> >  			   trace_kprobe_symbol(comp)) ||
> >  		    trace_kprobe_offset(orig) != trace_kprobe_offset(comp))
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index dd884341f5c5..11bcafdc93e2 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -420,6 +420,8 @@ static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig,
> > 
> >  	list_for_each_entry(pos, &tpe->probes, list) {
> >  		orig = container_of(pos, struct trace_uprobe, tp);
> > +		if (orig->tp.nr_args != comp->tp.nr_args)
> > +			continue;
> >  		if (comp_inode != d_real_inode(orig->path.dentry) ||
> >  		    comp->offset != orig->offset)
> >  			continue;
> 
> How about something like this?

Thank you for pointing it out. But from what I intended, this is caused by
a bug in trace_probe_compare_arg_type() or it's caller.

                /*
                 * trace_probe_compare_arg_type() ensured that nr_args and
                 * each argument name and type are same. Let's compare comm.
                 */

If we found that 2 probes have different number of argument should not be
folded at all.
Also, we have to adjust error log position. Attached patch will fix those
errors correctly as bellow.

/sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx >> kprobe_events
/sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx >> kprobe_events
sh: write error: File exists
/sys/kernel/debug/tracing # echo p:myevent kernel_read %ax >> kprobe_events
sh: write error: File exists
/sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx %cx >> kprobe_eve
nts
sh: write error: File exists
/sys/kernel/debug/tracing # cat error_log 
[   15.917727] trace_kprobe: error: There is already the exact same probe event
  Command: p:myevent kernel_read %ax %bx
           ^
[   24.890638] trace_kprobe: error: Argument type or name is different from existing probe
  Command: p:myevent kernel_read %ax
                                     ^
[   31.480521] trace_kprobe: error: Argument type or name is different from existing probe
  Command: p:myevent kernel_read %ax %bx %cx
                                         ^

Thank you,

-- 
Masami Hiramatsu

[-- Attachment #2: tracing-probe-fix-to-check-the --]
[-- Type: application/octet-stream, Size: 1524 bytes --]

tracing/probe: Fix to check the difference of nr_args before adding probe

From: Masami Hiramatsu <mhiramat@kernel.org>

Fix to check the difference of nr_args before adding probe
on existing probes. This also may set the error log index
bigger than the number of command parameters. In that case
it sets the error position is next to the last parameter.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index baf58a3612c0..905b10af5d5c 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -178,6 +178,16 @@ void __trace_probe_log_err(int offset, int err_type)
 	if (!command)
 		return;
 
+	if (trace_probe_log.index >= trace_probe_log.argc) {
+		/**
+		 * Set the error position is next to the last arg + space.
+		 * Note that len includes the terminal null and the cursor
+		 * appaers at pos + 1.
+		 */
+		pos = len;
+		offset = 0;
+	}
+
 	/* And make a command string from argv array */
 	p = command;
 	for (i = 0; i < trace_probe_log.argc; i++) {
@@ -1084,6 +1094,12 @@ int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b)
 {
 	int i;
 
+	/* In case of more arguments */
+	if (a->nr_args < b->nr_args)
+		return a->nr_args + 1;
+	if (a->nr_args > b->nr_args)
+		return b->nr_args + 1;
+
 	for (i = 0; i < a->nr_args; i++) {
 		if ((b->nr_args <= i) ||
 		    ((a->args[i].type != b->args[i].type) ||