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