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

[Masami Hiramatsu]

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.

>   			/* 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.

> +
>   /* 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@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/