Messages in this thread | | | Date | Thu, 27 Aug 2009 18:45:55 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 18/18] tracing/kprobes: Dump the culprit kprobe in case of kprobe recursion |
| |
On Thu, Aug 27, 2009 at 12:45:21PM -0400, Masami Hiramatsu wrote: > 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@gmail.com> >>>>>> Cc: Masami Hiramatsu<mhiramat@redhat.com> >>>>>> Cc: Ananth N Mavinakayanahalli<ananth@in.ibm.com> >>>>>> --- >>>>>> 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! >
Ah ok, I was just confused :) Well, then it's dangerous because it also detect false positives.
That does not seem easy to fix.
| |