Messages in this thread |  | | Date | Tue, 17 Oct 2017 13:35:59 +0530 | From | "Naveen N. Rao" <> | Subject | Re: [PATCH -tip v3 3/7] kprobes: Warn if optprobe handler tries to change execution path |
| |
On 2017/10/12 05:04AM, Masami Hiramatsu wrote: > On Tue, 10 Oct 2017 22:32:31 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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
|  |