lkml.org 
[lkml]   [2019]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] tracing/probe: Test nr_args match in looking for same probe events
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
[unhandled content-type:application/octet-stream]
\
 
 \ /
  Last update: 2019-09-28 10:18    [W:0.066 / U:3.920 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site