Re: [PATCH 18/18] tracing/kprobes: Dump the culprit kprobe in caseof kprobe recursion

[Masami Hiramatsu]

Frederic Weisbecker wrote:
> On Thu, Aug 27, 2009 at 11:52:09AM -0400, Masami Hiramatsu wrote:
> > Frederic Weisbecker wrote:
> > > On Thu, Aug 27, 2009 at 11:30:24AM -0400, Masami Hiramatsu wrote:
> > > > Hi Frederic,
> > > >
> > > > Frederic Weisbecker wrote:
> > > > > Kprobes can enter into a probing recursion, ie: a kprobe that does an
> > > > > endless loop because one of its core mechanism function used during
> > > > > probing is also probed itself.
> > > > >
> > > > > This patch helps pinpointing the kprobe that raised such recursion
> > > > > by dumping it and raising a BUG instead of a warning (we also disarm
> > > > > the kprobe to try avoiding recursion in BUG itself). Having a BUG
> > > > > instead of a warning stops the stacktrace in the right place and
> > > > > doesn't pollute the logs with hundreds of traces that eventually end
> > > > > up in a stack overflow.
> > > >
> > > > Thanks, but I also found similar bug cases.
> > > >
> > > > > Signed-off-by: Frederic Weisbecker<fweisbec@xxxxxxxxx>
> > > > > Cc: Masami Hiramatsu<mhiramat@xxxxxxxxxx>
> > > > > Cc: Ananth N Mavinakayanahalli<ananth@xxxxxxxxxx>
> > > > > ---
> > > > >     arch/x86/kernel/kprobes.c |    8 ++++++--
> > > > >     include/linux/kprobes.h   |    2 ++
> > > > >     kernel/kprobes.c          |    7 +++++++
> > > > >     3 files changed, 15 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
> > > > > index 16ae961..ecee3d2 100644
> > > > > --- a/arch/x86/kernel/kprobes.c
> > > > > +++ b/arch/x86/kernel/kprobes.c
> > > > > @@ -490,9 +490,13 @@ static int __kprobes reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
> > > >
> > > > Before this, kprobes checks p != kprobe_running(), but it's a
> > > > meaningless branch. Hitting a kprobe while KPROBES_HIT_SS always
> > > > treated as unrecoverable.
> > >
> > >
> > >
> > > Yeah, but that's the place where a probe ends up when bad reentrancy happens
> > > right?
> >
> > No, a place which is shared by kprobes and other subsystems, will cause a
> > problem.
> >
> > for example, I found an irq_return case which will be p == kprobe_running()
> > on x86-64.
> >
> > ->  <some irq occurs>
> >   ->  irq_return
> >     ->  <hit int3>
> >       ->  do_int3
> >       ->  <handling kprobe (set kprobe_running)>
> >     ->  irq_return (from do_int3)
> >       ->  <hit int3>
> >         ->  do_int3
> >         <handling kprobe (kprobe_running == p)>  <- here!
>
>
> Oh right.
>
>
> > Perhaps, the original code assumes that it will be caused by an int3
> > which another subsystem inserted on out-of-line singlestep buffer
> > if the hitting probe is same as current probe.
> >
> > However, in that case, int3 hitting address is on the out-of-line
> > buffer and should be different from first (current) int3 address.
>
>
> I see.
>
>
> > So, I think this part should also be removed.
> >
> >                if (p == kprobe_running()) {
> >                        regs->flags&= ~X86_EFLAGS_TF;
> >                        regs->flags |= kcb->kprobe_saved_flags;
> >                        return 0;
> >                } else {
> >
> > Thank you,
>
>
> So my patch is useless? Or is it also useful to detect real
> recursion? (despite of such corner cases)

Your patch is still useful! I'd like to suggest a bugfix :-).
Anyway, I'll send an update patch.

Thank you!

--
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@xxxxxxxxxx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/