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

[Frederic Weisbecker <fweisbec@gmail.com>]

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.