Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734777 > unrolled thread

[PATCH -tip v3 0/7] kprobes/x86: Preempt related enhancements

Back to article view | Back to linux.kernel

Contents

  [PATCH -tip v3 0/7] kprobes/x86: Preempt related enhancements Masami Hiramatsu <mhiramat@kernel.org> - 2017-09-19 12:00 +0200
    [PATCH -tip v3 1/7] kprobes: Improve smoke test to check preemptible Masami Hiramatsu <mhiramat@kernel.org> - 2017-09-19 12:00 +0200
    [PATCH -tip v3 7/7] kprobes: Use synchronize_rcu_tasks() for optprobe with CONFIG_PREEMPT Masami Hiramatsu <mhiramat@kernel.org> - 2017-09-19 12:10 +0200
    [PATCH -tip v3 3/7] kprobes: Warn if optprobe handler tries to change execution path Masami Hiramatsu <mhiramat@kernel.org> - 2017-09-19 12:10 +0200
    [PATCH -tip v3 5/7] kprobes/x86: Disable preempt ftrace-based jprobe Masami Hiramatsu <mhiramat@kernel.org> - 2017-09-19 12:10 +0200
    [PATCH -tip v3 4/7] kprobes/x86: Disable preempt in optprobe Masami Hiramatsu <mhiramat@kernel.org> - 2017-09-19 12:10 +0200
    [PATCH -tip v3 2/7] kprobes/x86: Move get_kprobe_ctlblk in irq-disabled block Masami Hiramatsu <mhiramat@kernel.org> - 2017-09-19 12:10 +0200
    [PATCH -tip v3 6/7] kprobes/x86: Remove disable_irq from ftrace-based/optimized kprobe Masami Hiramatsu <mhiramat@kernel.org> - 2017-09-19 12:10 +0200

#1734777 — [PATCH -tip v3 0/7] kprobes/x86: Preempt related enhancements

Hi,

Here is the 3rd version of the series to improve preempt
related behavior in kprobes/x86. This actually includes
many enhancements/fixes from the 2nd version, which is

https://lkml.org/lkml/2017/9/11/482

With the previous patch, lkp-bot reported that an issue
( https://lkml.org/lkml/2017/9/14/3 ) and I couldn't
reproduce it. However, I found a suspicious bug and fixed
it ([2/7]).

Also, while I was checking the correct condition for 
*probe handlers in Documentation/kprobes.txt, I also
found that current implementations for ftrace-based kprobe
and optprobe were mis-reading the document.
>From the document, handlers must be run with preempt-
disabled, but interrupt disabling is not guaranteed.
So in the middle of this series, patches ([4/7],[5/7],
[6/7]) adding preempt-disabling and removing irq-disabling.

And at last, I placed the original patch (Enable optprobe
with CONFIG_PREEMPT).

The others are just for making sure this fix works well.
- [1/7] is just adding preemptible checker in kprobe
  smake tests so that we can easily find mistake.
- [3/7] is adding an assert if user tries to change
  execution path in optprobe, which is obviously
  prohibited in the document (there also be how to
  avoid it.)

Thank you,

---

Masami Hiramatsu (7):
      kprobes: Improve smoke test to check preemptible
      kprobes/x86: Move get_kprobe_ctlblk in irq-disabled block
      kprobes: Warn if optprobe handler tries to change execution path
      kprobes/x86: Disable preempt in optprobe
      kprobes/x86: Disable preempt ftrace-based jprobe
      kprobes/x86: Remove disable_irq from ftrace-based/optimized kprobe
      kprobes: Use synchronize_rcu_tasks() for optprobe with CONFIG_PREEMPT


 arch/Kconfig                     |    2 +-
 arch/x86/kernel/kprobes/ftrace.c |   32 ++++++++++++++++----------------
 arch/x86/kernel/kprobes/opt.c    |    8 +++-----
 kernel/kprobes.c                 |   23 +++++++++++++++++------
 kernel/test_kprobes.c            |   20 ++++++++++++++++++++
 5 files changed, 57 insertions(+), 28 deletions(-)

--
Masami Hiramatsu

[toc] | [next] | [standalone]

#1734778 — [PATCH -tip v3 1/7] kprobes: Improve smoke test to check preemptible

Add preemptible check to each handler. Handlers are called with
non-preemtible, which is guaranteed by Documentation/kprobes.txt.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/test_kprobes.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/kernel/test_kprobes.c b/kernel/test_kprobes.c
index 0dbab6d1acb4..47106a1e645a 100644
--- a/kernel/test_kprobes.c
+++ b/kernel/test_kprobes.c
@@ -34,6 +34,10 @@ static noinline u32 kprobe_target(u32 value)
 
 static int kp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
+	if (preemptible()) {
+		handler_errors++;
+		pr_err("pre-handler is preemptible\n");
+	}
 	preh_val = (rand1 / div_factor);
 	return 0;
 }
@@ -41,6 +45,10 @@ static int kp_pre_handler(struct kprobe *p, struct pt_regs *regs)
 static void kp_post_handler(struct kprobe *p, struct pt_regs *regs,
 		unsigned long flags)
 {
+	if (preemptible()) {
+		handler_errors++;
+		pr_err("post-handler is preemptible\n");
+	}
 	if (preh_val != (rand1 / div_factor)) {
 		handler_errors++;
 		pr_err("incorrect value in post_handler\n");
@@ -156,6 +164,10 @@ static int test_kprobes(void)
 
 static u32 j_kprobe_target(u32 value)
 {
+	if (preemptible()) {
+		handler_errors++;
+		pr_err("jprobe-handler is preemptible\n");
+	}
 	if (value != rand1) {
 		handler_errors++;
 		pr_err("incorrect value in jprobe handler\n");
@@ -232,6 +244,10 @@ static u32 krph_val;
 
 static int entry_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
+	if (preemptible()) {
+		handler_errors++;
+		pr_err("kretprobe entry handler is preemptible\n");
+	}
 	krph_val = (rand1 / div_factor);
 	return 0;
 }
@@ -240,6 +256,10 @@ static int return_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
 	unsigned long ret = regs_return_value(regs);
 
+	if (preemptible()) {
+		handler_errors++;
+		pr_err("kretprobe return handler is preemptible\n");
+	}
 	if (ret != (rand1 / div_factor)) {
 		handler_errors++;
 		pr_err("incorrect value in kretprobe handler\n");

[toc] | [prev] | [next] | [standalone]

#1734780 — [PATCH -tip v3 7/7] kprobes: Use synchronize_rcu_tasks() for optprobe with CONFIG_PREEMPT

To enable jump optimized probe with CONFIG_PREEMPT, use
synchronize_rcu_tasks() to wait for all tasks preempted
on trampoline code back on track.

Since the jump optimized kprobes can replace multiple
instructions, there can be tasks which are preempted
on the 2nd (or 3rd) instructions. If the kprobe
replaces those instructions by a jump instruction,
when those tasks back to the preempted place, it is
a middle of the jump instruction and causes a kernel
panic.
To avoid such tragedies in advance, kprobe optimizer
prepare a detour route using normal kprobe (e.g.
int3 breakpoint on x86), and wait for the tasks which
is interrrupted on such place by synchronize_sched()
when CONFIG_PREEMPT=n.
If CONFIG_PREEMPT=y, things be more complicated, because
such interrupted thread can be preempted (other thread
can be scheduled in interrupt handler.) So, kprobes
optimizer has to wait for those tasks scheduled normally.
In this case we can use synchronize_rcu_tasks() which
ensures that all preempted tasks back on track and
schedule it.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 arch/Kconfig     |    2 +-
 kernel/kprobes.c |   18 +++++++++++++-----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 1aafb4efbb51..f75c8e8a229b 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -90,7 +90,7 @@ config STATIC_KEYS_SELFTEST
 config OPTPROBES
 	def_bool y
 	depends on KPROBES && HAVE_OPTPROBES
-	depends on !PREEMPT
+	select TASKS_RCU if PREEMPT
 
 config KPROBES_ON_FTRACE
 	def_bool y
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index de73b843c623..21d42ed2aaa5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -577,12 +577,20 @@ static void kprobe_optimizer(struct work_struct *work)
 
 	/*
 	 * Step 2: Wait for quiesence period to ensure all running interrupts
-	 * are done. Because optprobe may modify multiple instructions
-	 * there is a chance that Nth instruction is interrupted. In that
-	 * case, running interrupt can return to 2nd-Nth byte of jump
-	 * instruction. This wait is for avoiding it.
+	 * are done. Because optprobe may modify multiple instructions,
+	 * there is a chance that the Nth instruction is interrupted. In that
+	 * case, running interrupt can return to the Nth byte of jump
+	 * instruction. This can be avoided by waiting for returning of
+	 * such interrupts, since (until here) the first byte of the optimized
+	 * probe is already replaced with normal kprobe (sw breakpoint) and
+	 * all threads which reach to the probed address will hit it and
+	 * bypass the copied instructions (instead of executing the original.)
+	 * With CONFIG_PREEMPT, such interrupts can be preepmted. To wait
+	 * for such thread, we will use synchronize_rcu_tasks() which ensures
+	 * all preeempted tasks are scheduled normally (not preempted).
+	 * So we can ensure there is no threads preempted at probed address.
 	 */
-	synchronize_sched();
+	synchronize_rcu_tasks();
 
 	/* Step 3: Optimize kprobes after quiesence period */
 	do_optimize_kprobes();

[toc] | [prev] | [next] | [standalone]

#1734781 — [PATCH -tip v3 3/7] kprobes: Warn if optprobe handler tries to change execution path

Warn if optprobe handler tries to change execution path.
As described in Documentation/kprobes.txt, with optprobe
user handler can not change instruction pointer. In that
case user must avoid optimizing the kprobes by setting
post_handler or break_handler.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a1606a4224e1..de73b843c623 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -387,7 +387,10 @@ void opt_pre_handler(struct kprobe *p, struct pt_regs *regs)
 	list_for_each_entry_rcu(kp, &p->list, list) {
 		if (kp->pre_handler && likely(!kprobe_disabled(kp))) {
 			set_kprobe_instance(kp);
-			kp->pre_handler(kp, regs);
+			if (kp->pre_handler(kp, regs)) {
+				if (WARN_ON_ONCE(1))
+					pr_err("Optprobe ignores instruction pointer changing.(%pF)\n", p->addr);
+			}
 		}
 		reset_kprobe_instance();
 	}

[toc] | [prev] | [next] | [standalone]

#1734784 — [PATCH -tip v3 5/7] kprobes/x86: Disable preempt ftrace-based jprobe

Disable preempt in ftrace-based jprobe handler as
described in Documentation/kprobes.txt, there is

"Probe handlers are run with preemption disabled."

This will fix jprobes behavior when CONFIG_PREEMPT=y.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/ftrace.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 041f7b6dfa0f..bcfee4f69b0e 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -26,7 +26,7 @@
 #include "common.h"
 
 static nokprobe_inline
-int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
+void __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
 		      struct kprobe_ctlblk *kcb, unsigned long orig_ip)
 {
 	/*
@@ -41,20 +41,21 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
 	__this_cpu_write(current_kprobe, NULL);
 	if (orig_ip)
 		regs->ip = orig_ip;
-	return 1;
 }
 
 int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
 		    struct kprobe_ctlblk *kcb)
 {
-	if (kprobe_ftrace(p))
-		return __skip_singlestep(p, regs, kcb, 0);
-	else
-		return 0;
+	if (kprobe_ftrace(p)) {
+		__skip_singlestep(p, regs, kcb, 0);
+		preempt_enable_no_resched();
+		return 1;
+	}
+	return 0;
 }
 NOKPROBE_SYMBOL(skip_singlestep);
 
-/* Ftrace callback handler for kprobes */
+/* Ftrace callback handler for kprobes -- called under preepmt disabed */
 void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 			   struct ftrace_ops *ops, struct pt_regs *regs)
 {
@@ -77,13 +78,17 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		/* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
 		regs->ip = ip + sizeof(kprobe_opcode_t);
 
+		/* To emulate trap based kprobes, preempt_disable here */
+		preempt_disable();
 		__this_cpu_write(current_kprobe, p);
 		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
-		if (!p->pre_handler || !p->pre_handler(p, regs))
+		if (!p->pre_handler || !p->pre_handler(p, regs)) {
 			__skip_singlestep(p, regs, kcb, orig_ip);
+			preempt_enable_no_resched();
+		}
 		/*
 		 * If pre_handler returns !0, it sets regs->ip and
-		 * resets current kprobe.
+		 * resets current kprobe, and keep preempt count +1.
 		 */
 	}
 end:

[toc] | [prev] | [next] | [standalone]

#1734785 — [PATCH -tip v3 4/7] kprobes/x86: Disable preempt in optprobe

Disable preempt in optprobe handler as described
in Documentation/kprobes.txt, there is

"Probe handlers are run with preemption disabled."

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/opt.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 259b7e828b02..36e4f61c3eec 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -161,6 +161,7 @@ optimized_callback(struct optimized_kprobe *op, struct pt_regs *regs)
 		return;
 
 	local_irq_save(flags);
+	preempt_disable();
 	if (kprobe_running()) {
 		kprobes_inc_nmissed_count(&op->kp);
 	} else {
@@ -180,6 +181,7 @@ optimized_callback(struct optimized_kprobe *op, struct pt_regs *regs)
 		opt_pre_handler(&op->kp, regs);
 		__this_cpu_write(current_kprobe, NULL);
 	}
+	preempt_enable_no_resched();
 	local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(optimized_callback);

[toc] | [prev] | [next] | [standalone]

#1734787 — [PATCH -tip v3 2/7] kprobes/x86: Move get_kprobe_ctlblk in irq-disabled block

Since get_kprobe_ctlblk() accesses per-cpu variable
which calls smp_processor_id(), it must be called under
preempt-disabled or irq-disabled.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/opt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 4f98aad38237..259b7e828b02 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -154,7 +154,6 @@ STACK_FRAME_NON_STANDARD(optprobe_template_func);
 static void
 optimized_callback(struct optimized_kprobe *op, struct pt_regs *regs)
 {
-	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 	unsigned long flags;
 
 	/* This is possible if op is under delayed unoptimizing */
@@ -165,6 +164,7 @@ optimized_callback(struct optimized_kprobe *op, struct pt_regs *regs)
 	if (kprobe_running()) {
 		kprobes_inc_nmissed_count(&op->kp);
 	} else {
+		struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 		/* Save skipped registers */
 #ifdef CONFIG_X86_64
 		regs->cs = __KERNEL_CS;

[toc] | [prev] | [next] | [standalone]

#1734788 — [PATCH -tip v3 6/7] kprobes/x86: Remove disable_irq from ftrace-based/optimized kprobe

Actually kprobes doesn't need to disable irq if it is
called from ftrace/jump trampoline code because
Documentation/kprobes.txt says

-----
Probe handlers are run with preemption disabled.  Depending on the
architecture and optimization state, handlers may also run with
interrupts disabled (e.g., kretprobe handlers and optimized kprobe
handlers run without interrupt disabled on x86/x86-64).
-----

So let's remove irq disabling from those handlers.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/ftrace.c |    9 ++-------
 arch/x86/kernel/kprobes/opt.c    |    4 ----
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index bcfee4f69b0e..8dc0161cec8f 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -61,14 +61,11 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 {
 	struct kprobe *p;
 	struct kprobe_ctlblk *kcb;
-	unsigned long flags;
-
-	/* Disable irq for emulating a breakpoint and avoiding preempt */
-	local_irq_save(flags);
 
+	/* Preempt is disabled by ftrace */
 	p = get_kprobe((kprobe_opcode_t *)ip);
 	if (unlikely(!p) || kprobe_disabled(p))
-		goto end;
+		return;
 
 	kcb = get_kprobe_ctlblk();
 	if (kprobe_running()) {
@@ -91,8 +88,6 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 		 * resets current kprobe, and keep preempt count +1.
 		 */
 	}
-end:
-	local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
 
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 36e4f61c3eec..511aad1990a0 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -154,13 +154,10 @@ STACK_FRAME_NON_STANDARD(optprobe_template_func);
 static void
 optimized_callback(struct optimized_kprobe *op, struct pt_regs *regs)
 {
-	unsigned long flags;
-
 	/* This is possible if op is under delayed unoptimizing */
 	if (kprobe_disabled(&op->kp))
 		return;
 
-	local_irq_save(flags);
 	preempt_disable();
 	if (kprobe_running()) {
 		kprobes_inc_nmissed_count(&op->kp);
@@ -182,7 +179,6 @@ optimized_callback(struct optimized_kprobe *op, struct pt_regs *regs)
 		__this_cpu_write(current_kprobe, NULL);
 	}
 	preempt_enable_no_resched();
-	local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(optimized_callback);
 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web