Messages in this thread | | | Date | Sun, 4 Mar 2018 18:50:39 +0900 | From | Masami Hiramatsu <> | Subject | Re: [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>
| |