lkml.org 
[lkml]   [2009]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 18/18] tracing/kprobes: Dump the culprit kprobe in case of kprobe recursion
    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!


    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.

    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,

    >
    >
    >
    >>> /* A probe has been hit in the codepath leading up
    >>> * to, or just after, single-stepping of a probed
    >>> * instruction. This entire codepath should strictly
    >>> - * reside in .kprobes.text section. Raise a warning
    >>> - * to highlight this peculiar case.
    >>> + * reside in .kprobes.text section.
    >>> + * Raise a BUG or we'll continue in an endless
    >>> + * reentering loop and eventually a stack overflow.
    >>> */
    >>> + arch_disarm_kprobe(p);
    >>> + dump_kprobe(p);
    >>> + BUG();
    >>> }
    >>> default:
    >>> /* impossible cases */
    >>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
    >>> index bcd9c07..87eb79c 100644
    >>> --- a/include/linux/kprobes.h
    >>> +++ b/include/linux/kprobes.h
    >>> @@ -296,6 +296,8 @@ void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
    >>> int disable_kprobe(struct kprobe *kp);
    >>> int enable_kprobe(struct kprobe *kp);
    >>>
    >>> +void dump_kprobe(struct kprobe *kp);
    >>> +
    >>> #else /* !CONFIG_KPROBES: */
    >>>
    >>> static inline int kprobes_built_in(void)
    >>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
    >>> index ef177d6..f72e96c 100644
    >>> --- a/kernel/kprobes.c
    >>> +++ b/kernel/kprobes.c
    >>> @@ -1141,6 +1141,13 @@ static void __kprobes kill_kprobe(struct kprobe *p)
    >>> arch_remove_kprobe(p);
    >>> }
    >>>
    >>> +void __kprobes dump_kprobe(struct kprobe *kp)
    >>> +{
    >>> + printk(KERN_WARNING "Dumping kprobe:\n");
    >>> + printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
    >>> + kp->symbol_name, kp->addr, kp->offset);
    >>> +}
    >>
    >> Since kp->symbol_name + kp->offset = kp->addr, I recommend to show it
    >> as "Kprobe at %s+%x:<%p>\n", kp->symbol_name, kp->offset, kp->addr.
    >
    >
    > Ok I'll fix this, thanks.
    >
    >
    >>> +
    >>> /* Module notifier call back, checking kprobes on the module */
    >>> static int __kprobes kprobes_module_callback(struct notifier_block *nb,
    >>> unsigned long val, void *data)
    >>
    >> Thank you,
    >>
    --
    Masami Hiramatsu

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

    e-mail: mhiramat@redhat.com


    \
     
     \ /
      Last update: 2009-08-27 17:51    [W:2.506 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site