Re: [PATCH ftrace/core v3 1/3] [BUGFIX]kprobes/ftrace: Recover original IP if pre_handler doesn't change it

From: Masami Hiramatsu
Date: Thu Jul 17 2014 - 21:48:01 EST


(2014/07/18 3:21), Steven Rostedt wrote:
> This looks to require an acked-by from hpa.

Oh, I see.

>
> On Tue, 15 Jul 2014 06:00:21 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx> wrote:
>
>> Recover original IP register if the pre_handler doesn't change it.
>> Since current kprobes doesn't expect that another ftrace handler
>> may change regs->ip, it sets kprobe.addr + MCOUNT_INSN_SIZE to
>> regs->ip and returns to ftrace.
>> This seems wrong behavior since kprobes can recover regs->ip
>> and safely pass it to other handler.
>
> s/other/another/
>
>>
>> This adds a code which recovers original regs->ip passed from
>
> "... adds code which recovers the original ..."
>
>> ftrace right before returning ftrace, so that another ftrace user
>
> "... returning to ftrace, ..."

Thanks, I'll update the description. :)

>
>> can change regs->ip.
>
> You say BUGFIX in the subject, but as nothing changes the regs->ip
> currently from ftrace, is this currently a real bug?

Hmm, indeed, in-tree ftrace handlers never change regs->ip except
kprobes, and for the out-of-tree modules, we need IPMODIFY flag to
avoid such conflict among them. Thus, we can not solve such problem
only by this patch. OK, I'll drop BUGFIX tag.

Thank you,

>
> -- Steve
>
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>> ---
>> arch/x86/kernel/kprobes/ftrace.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
>> index 717b02a..5f8f0b3 100644
>> --- a/arch/x86/kernel/kprobes/ftrace.c
>> +++ b/arch/x86/kernel/kprobes/ftrace.c
>> @@ -27,7 +27,7 @@
>>
>> static nokprobe_inline
>> int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
>> - struct kprobe_ctlblk *kcb)
>> + struct kprobe_ctlblk *kcb, unsigned long orig_ip)
>> {
>> /*
>> * Emulate singlestep (and also recover regs->ip)
>> @@ -39,6 +39,8 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
>> p->post_handler(p, regs, 0);
>> }
>> __this_cpu_write(current_kprobe, NULL);
>> + if (orig_ip)
>> + regs->ip = orig_ip;
>> return 1;
>> }
>>
>> @@ -46,7 +48,7 @@ int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
>> struct kprobe_ctlblk *kcb)
>> {
>> if (kprobe_ftrace(p))
>> - return __skip_singlestep(p, regs, kcb);
>> + return __skip_singlestep(p, regs, kcb, 0);
>> else
>> return 0;
>> }
>> @@ -71,13 +73,14 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
>> if (kprobe_running()) {
>> kprobes_inc_nmissed_count(p);
>> } else {
>> + unsigned long orig_ip = regs->ip;
>> /* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
>> regs->ip = ip + sizeof(kprobe_opcode_t);
>>
>> __this_cpu_write(current_kprobe, p);
>> kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>> if (!p->pre_handler || !p->pre_handler(p, regs))
>> - __skip_singlestep(p, regs, kcb);
>> + __skip_singlestep(p, regs, kcb, orig_ip);
>> /*
>> * If pre_handler returns !0, it sets regs->ip and
>> * resets current kprobe.
>>
>
>


--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@xxxxxxxxxxx


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