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

Masami Hiramatsu
2009-08-27T15:48:49Z

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