lkml.org 
[lkml]   [2018]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH -tip] x86: kprobes: Cleanup preempt disabling and enabling
    On Sat, 3 Mar 2018 21:25:45 +0900
    Masami Hiramatsu <mhiramat@kernel.org> wrote:

    > On Sat, 3 Mar 2018 10:58:23 +0100
    > Ingo Molnar <mingo@kernel.org> wrote:
    >
    > >
    > > * Masami Hiramatsu <mhiramat@kernel.org> wrote:
    > >
    > > > +/*
    > > > + * Interrupts are disabled on entry as trap3 is an interrupt gate and they
    > > > + * remain disabled throughout this function.
    > > > + */
    > > > +int kprobe_int3_handler(struct pt_regs *regs)
    > > > +{
    > > > + struct kprobe_ctlblk *kcb;
    > > > + int ret;
    > > > +
    > > > + if (user_mode(regs))
    > > > + return 0;
    > > > +
    > > > + /*
    > > > + * We don't want to be preempted for the entire
    > > > + * duration of kprobe processing.
    > > > + */
    > > > + preempt_disable();
    > > > +
    > > > + kcb = get_kprobe_ctlblk();
    > > > + ret = kprobe_int3_dispatcher(regs, kcb);
    > > > +
    > > > + if (!kprobe_ready_for_singlestep(regs))
    > > > + preempt_enable_no_resched();
    > > > +
    > > > + return ret;
    > >
    > > What's the point of disabling preemption, if IRQs are disabled already?
    > >
    > > There's no preemption when IRQs are off...
    >
    > Ahh, right! Whole the kprobe singlestepping, IRQs are off (kprobes drops
    > IF from regs->flags for single stepping) so we don't need to care about
    > preempt count anymore...

    Hmm, I've found one issue for removing preemption... This is caused by error-
    injection framework and similar routines.

    OK, let me explain it. Histrically, jprobe needs to run its handler with preempt
    disabled. So kprobes skips preempt_enable() (and singlestep too) when its
    pre_handler returns 1 (which is the signal that the handler is jprobe pre_handler).
    Thus, this patch has a bug IF jprobe is survived. Fortunatelly(?) the jprobe has
    gone now, actual code is still there but interface has gone.

    Then, we have no problem?
    No, actually not, because of new error injection framework. The error injection
    framework uses similar techniq of jprobe. But it actually doesn't call back to
    longjmp-restore function, it changes regs->ip, enable preemption and return 1.
    In this path, since it doesn't setup singlestep (because return 1), above patch
    preempt_enable() again in the end of kprobe_int3_handler(). This may cause
    double decrement the preemt count when we use error_injection feature on bpf
    and fault-injection.

    So it must check "does kprobes set up singlestep, or kprobe pre_handler returns 1?"

    To tell the truth, this never happen currently because all error-injectable
    functions are ftrace-based, which will preempt_enable() if it pre_handler
    returns 1. But this means the behaviors of ftrace-based kprobes (and optprobe) and
    normal kprobes are different. That is a bug.

    So please ignore this patch.

    Anyway, I would like to change this behavior among archs after removing jprobe code.
    If we don't have jprobe, there is no reason to skip preempt_enable() if pre_handler
    returns 1. (including ftrace-based kprobe and optprobe)

    My plan is
    1. Remove arch-independent jprobe code
    2. Remove jprobes related code from each architecture.
    3. Change preempt behavior for each arch and update error-injection/bpf code.

    In step3, if I split patches for each arch, there is a chance to make a buggy kernel in
    error injection code. I will make a unified patch (for tree wide) or introduce some
    temporal flag (like ARCH_NO_PREEMPT_FIXUP) so that error-injection/bpf can check it.

    Thank you,


    --
    Masami Hiramatsu <mhiramat@kernel.org>

    \
     
     \ /
      Last update: 2018-03-04 10:51    [W:3.175 / U:0.504 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site