On 2017/10/12 05:04AM, Masami Hiramatsu wrote:
> On Tue, 10 Oct 2017 22:32:31 +0530
> "Naveen N. Rao" <[email protected]> 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
