Re: [PATCH -tip v3 3/7] kprobes: Warn if optprobe handler tries to change execution path

[Masami Hiramatsu]

On Tue, 10 Oct 2017 22:32:31 +0530
"Naveen N. Rao" <naveen.n.rao@xxxxxxxxxxxxxxxxxx> wrote:

> On 2017/09/19 10:00AM, Masami Hiramatsu wrote:
> > Warn if optprobe handler tries to change execution path.
> > As described in Documentation/kprobes.txt, with optprobe
> > user handler can not change instruction pointer. In that
> > case user must avoid optimizing the kprobes by setting
> > post_handler or break_handler.
> 
> But, if the pre handler returns !0, does that necessarily mean that the 
> [n]ip has been modified?

It should be prohibited to be jump optimized.

> 
> In Documentation/kprobes.txt, under API Reference for register_kprobe, 
> we have:
>   User's pre-handler (kp->pre_handler)::
> 
> 	  #include <linux/kprobes.h>
> 	  #include <linux/ptrace.h>
> 	  int pre_handler(struct kprobe *p, struct pt_regs *regs);
> 
>   Called with p pointing to the kprobe associated with the breakpoint,
>   and regs pointing to the struct containing the registers saved when
>   the breakpoint was hit.  Return 0 here unless you're a Kprobes geek.
> 			   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Yeah, this part should be updated clearly. 
Actually, you can find below NOTE also in Documentation/kprobes.txt in the
end of "How Does Jump Optimization Work?"

=========
NOTE for geeks:
The jump optimization changes the kprobe's pre_handler behavior.
Without optimization, the pre_handler can change the kernel's execution
path by changing regs->ip and returning 1.  However, when the probe
is optimized, that modification is ignored.  Thus, if you want to
tweak the kernel's execution path, you need to suppress optimization,
using one of the following techniques:

- Specify an empty function for the kprobe's post_handler or break_handler.

or

- Execute 'sysctl -w debug.kprobes_optimization=n'
=========

> So, we don't seem to _require_ users to return !0 if the handler changes 
> [n]ip? Or to always change [n]ip if returning !0.
> 
> The implicit assumption seems to be that the handler returns !0 if it 
> wants to suppress executing the probed instruction since the handler has 
> already taken care of that. So, at the least, I think the message should 
> change. However...
> 
> In powerpc, we place a probe on kretprobe_trampoline and optimize it. 

Oh, what did you do?? I think kretprobe_trampoline just calls
its handler to get correct address to return and just return to it.

> This works for us (even though optprobes doesn't "honour" changes to 
> [n]ip). See commit 762df10bad6954 ("powerpc/kprobes: Optimize kprobe in 
> kretprobe_trampoline()"). With this patch, we are now seeing a warning 
> (thanks to mpe for the report):
> 
> [  520.144449] ------------[ cut here ]------------
> [  520.144676] WARNING: CPU: 2 PID: 6355 at kernel/kprobes.c:391 opt_pre_handler+0xe8/0x110
> ...
> [  520.151806] CPU: 2 PID: 6355 Comm: ftracetest Not tainted 4.14.0-rc4-gcc6-next-20171009-g49827b9 #1
> [  520.152097] task: c0000000e9ddfb80 task.stack: c0000000f881c000
> [  520.152291] NIP:  c0000000001f3b78 LR: c0000000001f3b2c CTR: c0000000002436a0
> [  520.152527] REGS: c0000000f881f7f0 TRAP: 0700   Not tainted  (4.14.0-rc4-gcc6-next-20171009-g49827b9)
> [  520.152818] MSR:  8000000100021033 <SF,ME,IR,DR,RI,LE,TM[E]>  CR: 24002824  XER: 20000000
> [  520.153080] CFAR: c0000000001f3b34 SOFTE: 0
> ...
> [  520.155113] NIP [c0000000001f3b78] opt_pre_handler+0xe8/0x110
> [  520.155320] LR [c0000000001f3b2c] opt_pre_handler+0x9c/0x110
> [  520.155510] Call Trace:
> [  520.155590] [c0000000f881fa70] [c0000000001f3b2c] opt_pre_handler+0x9c/0x110 (unreliable)
> [  520.155825] [c0000000f881fb00] [c000000000047de8] optimized_callback+0xc8/0xe0
> [  520.156047] [c0000000f881fb40] [c000000000048764] optinsn_slot+0xec/0x10000
> [  520.156238] [c0000000f881fe30] [c000000000046cb0] kretprobe_trampoline+0x0/0x10
> [  520.156452] Instruction dump:
> [  520.156570] 7fbef840 409effa4 38210090 e8010010 eb41ffd0 eb61ffd8 eb81ffe0 eba1ffe8
> [  520.156792] ebc1fff0 ebe1fff8 7c0803a6 4e800020 <0fe00000> e89e0028 3c62ffce 386362b0
> [  520.157016] ---[ end trace d8cda029528a560d ]---
> [  520.157172] Optprobe ignores instruction pointer changing.(kretprobe_trampoline+0x0/0x10)
> 
> 
> So, should this patch be reverted?

Hmm, I got it. It seems to depend on arch implementation.
Anyway, this is just adding an warning, we can safely revert it.
And the documentation should be updated.

Ingo, could you revert this change?

Thank you,

> 
> 
> - Naveen
> 


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>