On Fri, 10 Feb 2017 23:21:13 +0000 Russell King - ARM Linux <[email protected]> wrote:
> On Sat, Feb 11, 2017 at 07:33:16AM +0900, Masami Hiramatsu wrote: > > On Fri, 10 Feb 2017 11:34:45 +0900 > > Masami Hiramatsu <[email protected]> wrote: > > > Ah, in_nmi() means FIQ on arm :) > > > OK, but actually it is too late to check it in the enter of > > > trampoline_handler() since we don't know where is the real > > > return address at that point. So I'll check that in setup site > > > - kretprobe_pre_handler(). > > > > Hmm, pre_handler_kretprobe() already checked in_nmi(). > > So, I think this will no problem on FIQ too. > > I don't blame you for missing that - the tracing and probes code is (at > least to me) quite a maze of code. > > From what I can tell, you're right - pre_handler_kretprobe() checks > in_nmi() early on, which prevents arch_prepare_kretprobe() (which > replaces regs->ARM_lr with the trampoline address) being run. Hence, > the trampoline should not be run if we were entered in FIQ mode. Right. > However, looking at kprobe_handler(), I'm much less convinced. This is > called as a result of hitting a probe instruction via > kprobe_trap_handler(). > > Now, if we have two kprobes, one in non-FIQ context and one in FIQ > context, and the non-FIQ context one is hit, we set the current kprobe: > > } else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) { > /* Probe hit and conditional execution check ok. */ > set_current_kprobe(p); > kcb->kprobe_status = KPROBE_HIT_ACTIVE; > > and call the pre-handler (which succeeds.) If we then take a FIQ and > hit a kprobe in a function called from FIQ, we will re-enter this > function. > > In this case, "cur" will be the non-FIQ kprobe, and "p" will be the FIQ > kprobe. It looks to me like we will single-step over the kprobe, and > resume. However, it will modify the kprobe_status to KPROBE_REENTER, > which may not be desirable. > > However, there does seem to be a hole. Let's say that we have a similar > scenario, except that the FIQ is well-timed to happen: > > if (!p->pre_handler || !p->pre_handler(p, regs)) { > kcb->kprobe_status = KPROBE_HIT_SS; > /* HERE */ > singlestep(p, regs, kcb); > if (p->post_handler) { > kcb->kprobe_status = > KPROBE_HIT_SSDONE; > > In that case: > > /* Kprobe is pending, so we're recursing. */ > switch (kcb->kprobe_status) { > case KPROBE_HIT_ACTIVE: > case KPROBE_HIT_SSDONE: > ... > default: > /* impossible cases */ > BUG(); > > becomes not such an "impossible case", so the kernel is likely to > explode. OK, this one should be a bug on arm implementation. On x86, we also check status == KPROBE_HIT_SS too, see reenter_kprobe() at arch/x86/kernel/kprobes/core.c. (see commit 6a5022a56) It seems same issue on arm64. I'll fix that. > This doesn't look good to me, and the pre-handler does nothing to > prevent this, so I still think we need some higher level protection in > kprobe_handler() against being entered in FIQ context - not only to > prevent that BUG() but also to prevent the kprobe status being changed > to "re-enter". What would you mean higher level? Thank you, -- Masami Hiramatsu <[email protected]>
