On Fri, Feb 24, 2017 at 10:04:51AM +0900, Masami Hiramatsu wrote: > On Thu, 23 Feb 2017 19:30:02 +0100 > Peter Zijlstra <pet...@infradead.org> 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(), 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. 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.gb6...@twins.programming.kicks-ass.net > 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? Would it not be better to do the fixup in fixup_exception()/fixup_bug()? Because then we cover all callers, not just #PF. 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().
