[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20190928011748.599255f6ffc9a4831e1efd2c@kernel.org>
Date: Sat, 28 Sep 2019 01:17:48 -0700
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
LKML <linux-kernel@...r.kernel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Naveen Rao <naveen.n.rao@...ux.vnet.ibm.com>,
Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Subject: Re: [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@...ux.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@...dmis.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
Download attachment "tracing-probe-fix-to-check-the" of type "application/octet-stream" (1524 bytes)
Powered by blists - more mailing lists