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

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

[Masami Hiramatsu]

Cleanup x86/kprobes preempt counts so that preemt_disable()
and preempt_enable_no_sched() are called from kprobe_int3_handler().
Only if a kprobe runs single-stepping, preemption is kept
disabled and that is enabled when
 - single-stepping is finished
 - a fault occurs on single-steped instruction
 - jprobe handler is finished
 - Or, in user handler which changes regs->ip
   (e.g. function-based error injection)

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c   |   64 +++++++++++++++++++++++---------------
 arch/x86/kernel/kprobes/ftrace.c |    1 -
 arch/x86/kernel/kprobes/opt.c    |    1 -
 3 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index bd36f3c33cd0..7d63a3b8c8b2 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -592,7 +592,6 @@ static void setup_singlestep(struct kprobe *p, struct pt_regs *regs,
 		 * stepping.
 		 */
 		regs->ip = (unsigned long)p->ainsn.insn;
-		preempt_enable_no_resched();
 		return;
 	}
 #endif
@@ -650,29 +649,13 @@ static int reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
 }
 NOKPROBE_SYMBOL(reenter_kprobe);
 
-/*
- * 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)
+static nokprobe_inline int
+kprobe_int3_dispatcher(struct pt_regs *regs, struct kprobe_ctlblk *kcb)
 {
 	kprobe_opcode_t *addr;
 	struct kprobe *p;
-	struct kprobe_ctlblk *kcb;
-
-	if (user_mode(regs))
-		return 0;
 
 	addr = (kprobe_opcode_t *)(regs->ip - sizeof(kprobe_opcode_t));
-	/*
-	 * We don't want to be preempted for the entire
-	 * duration of kprobe processing. We conditionally
-	 * re-enable preemption at the end of this function,
-	 * and also in reenter_kprobe() and setup_singlestep().
-	 */
-	preempt_disable();
-
-	kcb = get_kprobe_ctlblk();
 	p = get_kprobe(addr);
 
 	if (p) {
@@ -706,7 +689,6 @@ int kprobe_int3_handler(struct pt_regs *regs)
 		 * the original instruction.
 		 */
 		regs->ip = (unsigned long)addr;
-		preempt_enable_no_resched();
 		return 1;
 	} else if (kprobe_running()) {
 		p = __this_cpu_read(current_kprobe);
@@ -717,9 +699,41 @@ int kprobe_int3_handler(struct pt_regs *regs)
 		}
 	} /* else: not a kprobe fault; let the kernel handle it */
 
-	preempt_enable_no_resched();
 	return 0;
 }
+
+static nokprobe_inline bool
+kprobe_ready_for_singlestep(struct pt_regs *regs)
+{
+	return kprobe_running() && (regs->flags & X86_EFLAGS_TF);
+}
+
+/*
+ * 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;
+}
 NOKPROBE_SYMBOL(kprobe_int3_handler);
 
 /*
@@ -962,12 +976,10 @@ int kprobe_debug_handler(struct pt_regs *regs)
 	}
 
 	/* Restore back the original saved kprobes variables and continue. */
-	if (kcb->kprobe_status == KPROBE_REENTER) {
+	if (kcb->kprobe_status == KPROBE_REENTER)
 		restore_previous_kprobe(kcb);
-		goto out;
-	}
-	reset_current_kprobe();
-out:
+	else
+		reset_current_kprobe();
 	preempt_enable_no_resched();
 
 	/*
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 8dc0161cec8f..24cb3b676240 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -48,7 +48,6 @@ int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
 {
 	if (kprobe_ftrace(p)) {
 		__skip_singlestep(p, regs, kcb, 0);
-		preempt_enable_no_resched();
 		return 1;
 	}
 	return 0;
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 203d398802a3..eaf02f2e7300 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -491,7 +491,6 @@ int setup_detour_execution(struct kprobe *p, struct pt_regs *regs, int reenter)
 		regs->ip = (unsigned long)op->optinsn.insn + TMPL_END_IDX;
 		if (!reenter)
 			reset_current_kprobe();
-		preempt_enable_no_resched();
 		return 1;
 	}
 	return 0;

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

[Ingo Molnar]

* 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...

Thanks,

	Ingo

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

[Masami Hiramatsu]

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...

Thank you!


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Masami Hiramatsu]

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>