Re: kprobes vs __ex_table[]

[Masami Hiramatsu]

On Fri, 24 Feb 2017 10:26:46 +0100
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Fri, Feb 24, 2017 at 10:04:51AM +0900, Masami Hiramatsu wrote:
> > On Thu, 23 Feb 2017 19:30:02 +0100
> > Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > 
> > > Hi Masami,
> > > 
> > > I just wondered what would happen if I put a probe on an instruction
> > > that was listed in __ex_table[] or __bug_table[].
> > 
> > Ah, thanks for reporting, I know __ex_table issue and fixed, but
> > I didn't care about __bug_table.
> > 
> > > And it looks like it will happily do that. It will then run the
> > > instruction out-of-line, and when said instruction traps, the
> > > instruction address will not match the one listed in either __ex_table[]
> > > or __bug_table[] and badness will happen.
> > 
> > For the __ex_table[], at least on x86, kprobes already handles it in
> > kprobe_fault_handler, which restore regs->ip to original place when
> > a pagefault happens on singlestepping.
> 
> Ah, but that is only #PF, we also use __ex_table on other faults/traps,
> like #GP which would need help in do_general_protection(),

#GP is also handled via kprobe_exceptions_notify().

> and I have a
> patch (that might not ever go anywhere) that uses it on #UD (but for all
> I know we already use #UD to probe existence of instructions).
> 
> In any case, we call fixup_exception() from pretty much all exception
> vectors, not only #PF.

OK, kprobes actually already has handled notify_die via above function,
so one possible solution is extend it for those exceptions.

> 
> But see below.
> 
> > > If kprobes does indeed not check this, we should probably fix it, if it
> > > does do check this, could you point me to it?
> > 
> > Yeah, for BUG() case, as far as I can see, there is no check about that.
> 
> So I've a patch that extends __bug_table[] to WARN (like many other
> architectures already have).
> 
> http://lkml.kernel.org/r/20170223132813.GB6515@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

OK, I got it.

> 
> > So, there are 2 ways to fix it up, one is to just reject to put kprobes on
> > UD2, another is fixup trap address as we did for exceptions_table.
> > I think latter is better because if there is a divide error happening
> > on single-step, anyway we should fixup the address...
> 
> Right.
> 
> So I like the fixup idea, just not sure the current
> kprobe_fault_handler() is sufficient or correct.
> 
> It looks like it rewrites regs->ip, which would make return from fault
> return to the wrong place, no?

Hmm, when regs->ip is reset to the original place, kprobe_fault_handler()
returns 0 and normal #PF handler fixup pages etc. and retry from the
original place. This might kick kprobes again and do singlestep.

So, yes, it may not enough for other faults if those will not only check
regs->ip, but read the instruction pointed by regs->ip (as your patch).
In that case you need to use recover_probed_instruction() instead of
probe_kernel_address(). (BTW, recover_probed_instruction() uses memcpy()
without checking kernel_text, it should use probe_kernel_address().)

> 
> Would it not be better to do the fixup in fixup_exception()/fixup_bug()?
> Because then we cover all callers, not just #PF.

As I said above, kprobes already handles notify_die. But yes, we need
a special fixup call before notify_die.

> One more complication with __ex_table and optimized kprobes is that we
> need to be careful not to clobber __ex_table[].fixup. It would be very
> bad if the optimized probe were to clobber the address we let the fixup
> return to -- or that needs fixups too, _after_ running
> __ex_table[].handler().

can_optimize() takes care about that case. If the probe target function
(not only probed address) includes an exception address, it rejects
optimizing probes.

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>