[RFC PATCH -tip 0/4] kprobes: x86: Remove jprobes related code

[RFC PATCH -tip 0/4] kprobes: x86: Remove jprobes related code

[Masami Hiramatsu]

Hi,

This series is a core part of removing jprobes related
code from generic part and x86 tree. This also simplifies
the preparation code for function override by balancing
preempt count and current kprobe setting inside
kprobe_int3_handler and kprobe_ftrace_handler.

 [1/4]: Trivial cleanup, remove a remaining "jps" word
        from document.
 [2/4]: Remove arch-independent jprobe code from
        kernel/kprobes.c. This prevents link errors
        after removing arch dependent jprobe ports.
 [3/4]: Remove x86 jprobe port.
 [4/4]: Simplify preempt count and current kprobe setting
        inside kprobe_int3_handler and kprobe_ftrace_handler.

I'm preparing similar patches for other archs. But I think
this series can be (and should be) merged first, since
the function override feature (which is the only in-tree
component affected by this change) is implemented on x86.

Thank you,

---

Masami Hiramatsu (4):
      Documentation/kprobes: Fix to remove remaining jprobe
      kprobes: Remove jprobe generic code
      kprobes: x86: Remove jprobe x86 port
      bpf: error-inject: x86: Fix unbalanced preempt-count for function override


 Documentation/kprobes.txt        |    2 -
 arch/x86/include/asm/kprobes.h   |    3 -
 arch/x86/kernel/kprobes/core.c   |  113 ++++----------------------------------
 arch/x86/kernel/kprobes/ftrace.c |   16 +++--
 kernel/fail_function.c           |    3 -
 kernel/kprobes.c                 |   76 --------------------------
 kernel/trace/trace_kprobe.c      |   11 +---
 7 files changed, 24 insertions(+), 200 deletions(-)

-- 
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[RFC PATCH -tip 1/4] Documentation/kprobes: Fix to remove remaining jprobe

[Masami Hiramatsu]

Remove jps from the document, since jprobe is removed.

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

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 22208bf2386d..5ae80baf3921 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -474,7 +474,7 @@ error occurs during registration, all probes in the array, up to
 the bad probe, are safely unregistered before the register_*probes
 function returns.
 
-- kps/rps/jps: an array of pointers to ``*probe`` data structures
+- kps/rps: an array of pointers to ``*probe`` data structures
 - num: the number of the array entries.
 
 .. note::

[RFC PATCH -tip 2/4] kprobes: Remove jprobe generic code

[Masami Hiramatsu]

Remove jprobe arch independent generic code
from kernel/kprobes.c.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |   76 ------------------------------------------------------
 1 file changed, 76 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ea619021d901..cb1c372eef62 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1812,77 +1812,6 @@ unsigned long __weak arch_deref_entry_point(void *entry)
 	return (unsigned long)entry;
 }
 
-#if 0
-int register_jprobes(struct jprobe **jps, int num)
-{
-	int ret = 0, i;
-
-	if (num <= 0)
-		return -EINVAL;
-
-	for (i = 0; i < num; i++) {
-		ret = register_jprobe(jps[i]);
-
-		if (ret < 0) {
-			if (i > 0)
-				unregister_jprobes(jps, i);
-			break;
-		}
-	}
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(register_jprobes);
-
-int register_jprobe(struct jprobe *jp)
-{
-	unsigned long addr, offset;
-	struct kprobe *kp = &jp->kp;
-
-	/*
-	 * Verify probepoint as well as the jprobe handler are
-	 * valid function entry points.
-	 */
-	addr = arch_deref_entry_point(jp->entry);
-
-	if (kallsyms_lookup_size_offset(addr, NULL, &offset) && offset == 0 &&
-	    kprobe_on_func_entry(kp->addr, kp->symbol_name, kp->offset)) {
-		kp->pre_handler = setjmp_pre_handler;
-		kp->break_handler = longjmp_break_handler;
-		return register_kprobe(kp);
-	}
-
-	return -EINVAL;
-}
-EXPORT_SYMBOL_GPL(register_jprobe);
-
-void unregister_jprobe(struct jprobe *jp)
-{
-	unregister_jprobes(&jp, 1);
-}
-EXPORT_SYMBOL_GPL(unregister_jprobe);
-
-void unregister_jprobes(struct jprobe **jps, int num)
-{
-	int i;
-
-	if (num <= 0)
-		return;
-	mutex_lock(&kprobe_mutex);
-	for (i = 0; i < num; i++)
-		if (__unregister_kprobe_top(&jps[i]->kp) < 0)
-			jps[i]->kp.addr = NULL;
-	mutex_unlock(&kprobe_mutex);
-
-	synchronize_sched();
-	for (i = 0; i < num; i++) {
-		if (jps[i]->kp.addr)
-			__unregister_kprobe_bottom(&jps[i]->kp);
-	}
-}
-EXPORT_SYMBOL_GPL(unregister_jprobes);
-#endif
-
 #ifdef CONFIG_KRETPROBES
 /*
  * This kprobe pre_handler is registered with every kretprobe. When probe
@@ -2329,8 +2258,6 @@ static void report_probe(struct seq_file *pi, struct kprobe *p,
 
 	if (p->pre_handler == pre_handler_kretprobe)
 		kprobe_type = "r";
-	else if (p->pre_handler == setjmp_pre_handler)
-		kprobe_type = "j";
 	else
 		kprobe_type = "k";
 
@@ -2637,6 +2564,3 @@ late_initcall(debugfs_kprobe_init);
 #endif /* CONFIG_DEBUG_FS */
 
 module_init(init_kprobes);
-
-/* defined in arch/.../kernel/kprobes.c */
-EXPORT_SYMBOL_GPL(jprobe_return);

Re: [RFC PATCH -tip 2/4] kprobes: Remove jprobe generic code

[Thomas Gleixner]

On Tue, 8 May 2018, Masami Hiramatsu wrote:

> Remove jprobe arch independent generic code
> from kernel/kprobes.c.

# git grep register_jprobes
include/linux/kprobes.h:static inline int register_jprobes(struct jprobe **jps, int num)
include/linux/kprobes.h:static inline void unregister_jprobes(struct jprobe **jps, int num)
kernel/test_kprobes.c:  ret = register_jprobes(jps, 2);
kernel/test_kprobes.c:          pr_err("register_jprobes returned %d\n", ret);
kernel/test_kprobes.c:  unregister_jprobes(jps, 2);

You forgot to remove a few things ...

Thanks,

	tglx

Re: [RFC PATCH -tip 2/4] kprobes: Remove jprobe generic code

[Masami Hiramatsu]

On Sun, 13 May 2018 21:55:38 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 8 May 2018, Masami Hiramatsu wrote:
> 
> > Remove jprobe arch independent generic code
> > from kernel/kprobes.c.
> 
> # git grep register_jprobes
> include/linux/kprobes.h:static inline int register_jprobes(struct jprobe **jps, int num)
> include/linux/kprobes.h:static inline void unregister_jprobes(struct jprobe **jps, int num)
> kernel/test_kprobes.c:  ret = register_jprobes(jps, 2);
> kernel/test_kprobes.c:          pr_err("register_jprobes returned %d\n", ret);
> kernel/test_kprobes.c:  unregister_jprobes(jps, 2);
> 
> You forgot to remove a few things ...

Ah, thoes are already disabled (with #if 0) but it is better to remove it first.

Thanks!

> 
> Thanks,
> 
> 	tglx


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[RFC PATCH -tip 3/4] kprobes: x86: Remove jprobe x86 port

[Masami Hiramatsu]

Remove jprobe related functions and ignore break_handler
since the break_handler is used only from jprobe.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/include/asm/kprobes.h |    3 -
 arch/x86/kernel/kprobes/core.c |   94 ----------------------------------------
 2 files changed, 97 deletions(-)

diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index 367d99cff426..06782c2efa04 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -111,9 +111,6 @@ struct kprobe_ctlblk {
 	unsigned long kprobe_status;
 	unsigned long kprobe_old_flags;
 	unsigned long kprobe_saved_flags;
-	unsigned long *jprobe_saved_sp;
-	struct pt_regs jprobe_saved_regs;
-	kprobe_opcode_t jprobes_stack[MAX_STACK_SIZE];
 	struct prev_kprobe prev_kprobe;
 };
 
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 0715f827607c..29c11c8d79ab 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -708,13 +708,6 @@ int kprobe_int3_handler(struct pt_regs *regs)
 		regs->ip = (unsigned long)addr;
 		preempt_enable_no_resched();
 		return 1;
-	} else if (kprobe_running()) {
-		p = __this_cpu_read(current_kprobe);
-		if (p->break_handler && p->break_handler(p, regs)) {
-			if (!skip_singlestep(p, regs, kcb))
-				setup_singlestep(p, regs, kcb, 0);
-			return 1;
-		}
 	} /* else: not a kprobe fault; let the kernel handle it */
 
 	preempt_enable_no_resched();
@@ -1079,93 +1072,6 @@ int kprobe_exceptions_notify(struct notifier_block *self, unsigned long val,
 }
 NOKPROBE_SYMBOL(kprobe_exceptions_notify);
 
-int setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs)
-{
-	struct jprobe *jp = container_of(p, struct jprobe, kp);
-	unsigned long addr;
-	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
-
-	kcb->jprobe_saved_regs = *regs;
-	kcb->jprobe_saved_sp = stack_addr(regs);
-	addr = (unsigned long)(kcb->jprobe_saved_sp);
-
-	/*
-	 * As Linus pointed out, gcc assumes that the callee
-	 * owns the argument space and could overwrite it, e.g.
-	 * tailcall optimization. So, to be absolutely safe
-	 * we also save and restore enough stack bytes to cover
-	 * the argument area.
-	 * Use __memcpy() to avoid KASAN stack out-of-bounds reports as we copy
-	 * raw stack chunk with redzones:
-	 */
-	__memcpy(kcb->jprobes_stack, (kprobe_opcode_t *)addr, MIN_STACK_SIZE(addr));
-	regs->ip = (unsigned long)(jp->entry);
-
-	/*
-	 * jprobes use jprobe_return() which skips the normal return
-	 * path of the function, and this messes up the accounting of the
-	 * function graph tracer to get messed up.
-	 *
-	 * Pause function graph tracing while performing the jprobe function.
-	 */
-	pause_graph_tracing();
-	return 1;
-}
-NOKPROBE_SYMBOL(setjmp_pre_handler);
-
-void jprobe_return(void)
-{
-	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
-
-	/* Unpoison stack redzones in the frames we are going to jump over. */
-	kasan_unpoison_stack_above_sp_to(kcb->jprobe_saved_sp);
-
-	asm volatile (
-#ifdef CONFIG_X86_64
-			"       xchg   %%rbx,%%rsp	\n"
-#else
-			"       xchgl   %%ebx,%%esp	\n"
-#endif
-			"       int3			\n"
-			"       .globl jprobe_return_end\n"
-			"       jprobe_return_end:	\n"
-			"       nop			\n"::"b"
-			(kcb->jprobe_saved_sp):"memory");
-}
-NOKPROBE_SYMBOL(jprobe_return);
-NOKPROBE_SYMBOL(jprobe_return_end);
-
-int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
-{
-	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
-	u8 *addr = (u8 *) (regs->ip - 1);
-	struct jprobe *jp = container_of(p, struct jprobe, kp);
-	void *saved_sp = kcb->jprobe_saved_sp;
-
-	if ((addr > (u8 *) jprobe_return) &&
-	    (addr < (u8 *) jprobe_return_end)) {
-		if (stack_addr(regs) != saved_sp) {
-			struct pt_regs *saved_regs = &kcb->jprobe_saved_regs;
-			printk(KERN_ERR
-			       "current sp %p does not match saved sp %p\n",
-			       stack_addr(regs), saved_sp);
-			printk(KERN_ERR "Saved registers for jprobe %p\n", jp);
-			show_regs(saved_regs);
-			printk(KERN_ERR "Current registers\n");
-			show_regs(regs);
-			BUG();
-		}
-		/* It's OK to start function graph tracing again */
-		unpause_graph_tracing();
-		*regs = kcb->jprobe_saved_regs;
-		__memcpy(saved_sp, kcb->jprobes_stack, MIN_STACK_SIZE(saved_sp));
-		preempt_enable_no_resched();
-		return 1;
-	}
-	return 0;
-}
-NOKPROBE_SYMBOL(longjmp_break_handler);
-
 bool arch_within_kprobe_blacklist(unsigned long addr)
 {
 	bool is_in_entry_trampoline_section = false;

[RFC PATCH -tip 4/4] bpf: error-inject: x86: Fix unbalanced preempt-count for function override

[Masami Hiramatsu]

Fix unbalanced preempt-count for function override
using kprobes on x86.

Jprobe requires to keep preemption disabled and keep
current kprobes until it recovers to original function
entry. For this reason kprobe_int3_handler() makes
preempt-count unbalanced when user handler returns !0.

After removing the jprobe, Kprobes does not need to
keep preempt disabled even if user handler returns !0.

But since the function override handler is also returns
!0 if it overrides a function, to balancing the preempt
count, it enables preemption and reset current kprobe
by itself.

That is a bad design that is very buggy. This fixes
the unbalanced preempt-count in both kprobes and
function override on x86.
Of course we have to fix same code of kprobes on other
archs to keep the kprobes behavior consistent. But this
is bisect-safe because the function override is
implemented only on x86 at this moment.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c   |   19 +++++++++++--------
 arch/x86/kernel/kprobes/ftrace.c |   16 +++++++++-------
 kernel/fail_function.c           |    3 ---
 kernel/trace/trace_kprobe.c      |   11 +++--------
 4 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 29c11c8d79ab..4bb03570b7bf 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -683,16 +683,19 @@ int kprobe_int3_handler(struct pt_regs *regs)
 			set_current_kprobe(p, regs, kcb);
 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
 
-			/*
-			 * If we have no pre-handler or it returned 0, we
-			 * continue with normal processing.  If we have a
-			 * pre-handler and it returned non-zero, it prepped
-			 * for calling the break_handler below on re-entry
-			 * for jprobe processing, so get out doing nothing
-			 * more here.
-			 */
 			if (!p->pre_handler || !p->pre_handler(p, regs))
 				setup_singlestep(p, regs, kcb, 0);
+			else {
+				/*
+				 * If pre_handler returns !0, this handler
+				 * modifies regs->ip and goes back to there
+				 * directly without single stepping.
+				 * So let's clear current kprobe and decrement
+				 * preempt count, which we set in this function.
+				 */
+				reset_current_kprobe();
+				preempt_enable_no_resched();
+			}
 			return 1;
 		}
 	} else if (*addr != BREAKPOINT_INSTRUCTION) {
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 8dc0161cec8f..e6f0075bfefc 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -75,18 +75,20 @@ 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)) {
 			__skip_singlestep(p, regs, kcb, orig_ip);
-			preempt_enable_no_resched();
+		} else {
+			/*
+			 * If pre_handler returns !0, this handler
+			 * modifies regs->ip and goes back to there
+			 * directly without single stepping.
+			 * So let's just clear current kprobe. Preempt count
+			 * will be handled by ftrace correctly.
+			 */
+			__this_cpu_write(current_kprobe, NULL);
 		}
-		/*
-		 * If pre_handler returns !0, it sets regs->ip and
-		 * resets current kprobe, and keep preempt count +1.
-		 */
 	}
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
diff --git a/kernel/fail_function.c b/kernel/fail_function.c
index 1d5632d8bbcc..b090688df94f 100644
--- a/kernel/fail_function.c
+++ b/kernel/fail_function.c
@@ -184,9 +184,6 @@ static int fei_kprobe_handler(struct kprobe *kp, struct pt_regs *regs)
 	if (should_fail(&fei_fault_attr, 1)) {
 		regs_set_return_value(regs, attr->retval);
 		override_function_with_return(regs);
-		/* Kprobe specific fixup */
-		reset_current_kprobe();
-		preempt_enable_no_resched();
 		return 1;
 	}
 
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 02aed76e0978..b65cd6834450 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1217,16 +1217,11 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)
 
 		/*
 		 * We need to check and see if we modified the pc of the
-		 * pt_regs, and if so clear the kprobe and return 1 so that we
-		 * don't do the single stepping.
-		 * The ftrace kprobe handler leaves it up to us to re-enable
-		 * preemption here before returning if we've modified the ip.
+		 * pt_regs, and if so return 1 so that we don't do the
+		 * single stepping.
 		 */
-		if (orig_ip != instruction_pointer(regs)) {
-			reset_current_kprobe();
-			preempt_enable_no_resched();
+		if (orig_ip != instruction_pointer(regs))
 			return 1;
-		}
 		if (!ret)
 			return 0;
 	}