Re: [PATCH -tip] x86: kprobes: Cleanup preempt disabling and enabling

[Masami Hiramatsu]

On Sat, 3 Mar 2018 21:25:45 +0900
Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:

> On Sat, 3 Mar 2018 10:58:23 +0100
> Ingo Molnar <mingo@xxxxxxxxxx> wrote:
> 
> > 
> > * Masami Hiramatsu <mhiramat@xxxxxxxxxx> 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@xxxxxxxxxx>