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

[Naveen N. Rao]

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?

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

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


- Naveen