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

[Naveen N. Rao]

On 2017/10/12 05:04AM, Masami Hiramatsu wrote:
> 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:
> > 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.

For x86 yes, but on powerpc, we use the original implementation of 
placing a probe at kretprobe_trampoline for catching the function 
return.

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

Yes, we're optimizing the probe at kretprobe_trampoline, so we need 
this.

> Anyway, this is just adding an warning, we can safely revert it.
> And the documentation should be updated.
> 
> Ingo, could you revert this change?

Thanks!
I will send a patch to revert this change.


- Naveen