On Sat, Feb 11, 2017 at 07:33:16AM +0900, Masami Hiramatsu wrote:
> On Fri, 10 Feb 2017 11:34:45 +0900
> Masami Hiramatsu <mhira...@kernel.org> 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.
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.
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".
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
