On Tue, 8 Jan 2019 13:44:54 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

Mark ftrace mcount handler functions nokprobe since probing on these functions with kretprobe pushes return address incorrectly on kretprobe shadow stack.

Signed-off-by: Masami Hiramatsu mhiramat@kernel.org Reported-by: Francis Deslauriers francis.deslauriers@efficios.com Tested-by: Andrea Righi righi.andrea@gmail.com Cc: stable@vger.kernel.org

Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org

-- Steve

---

• Changes in v2: Fix to include kprobes.h (Thanks Andrea!)

---

kernel/trace/ftrace.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index f0ff24173a0b..b0774388d52b 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -34,6 +34,7 @@ #include <linux/list.h> #include <linux/hash.h> #include <linux/rcupdate.h> +#include <linux/kprobes.h> #include <trace/events/sched.h> @@ -6250,7 +6251,7 @@ void ftrace_reset_array_ops(struct trace_array *tr) tr->ops->func = ftrace_stub; } -static inline void +static nokprobe_inline void __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, struct ftrace_ops *ignored, struct pt_regs *regs) { @@ -6310,11 +6311,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, { __ftrace_ops_list_func(ip, parent_ip, NULL, regs); } +NOKPROBE_SYMBOL(ftrace_ops_list_func); #else static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) { __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); } +NOKPROBE_SYMBOL(ftrace_ops_no_ops); #endif /* @@ -6341,6 +6344,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip, preempt_enable_notrace(); trace_clear_recursion(bit); } +NOKPROBE_SYMBOL(ftrace_ops_assist_func); /**

• ftrace_ops_get_func - get the function a trampoline should call

On Tue, 8 Jan 2019 13:45:22 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:

Fix to avoid kretprobe recursion loop by setting a dummy kprobes to current_kprobe per-cpu variable.

This bug has been introduced with the asm-coded trampoline code, since previously it used another kprobe for hooking the function return placeholder (which only has a nop) and trampoline handler was called from that kprobe.

This revives the old lost kprobe again.

With this fix, we don't see deadlock anymore.

# echo "r:event_1 __fdget" >> kprobe_events # echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events # echo 1 > events/kprobes/enable

And you can see that all inner-called kretprobe are skipped.

# cat kprobe_profile event_1 235 0 event_2 19375 19612

The 1st column is recorded count and the 2nd is missed count. Above shows (event_1 rec) + (event_2 rec) ~= (event_2 missed)

I don't quite understand the above. Is the miss count because we missed event_2 events for both event_1 and event_2?

trace raw_spin_lock() handler calls raw_spin_lock() trace raw_spin_lock() [ skip ]

I'm also guessing that the 2 extra (19612 - (235 + 19375) = 2) are possibly due to the displaying being racy?

Signed-off-by: Masami Hiramatsu mhiramat@kernel.org Reported-by: Andrea Righi righi.andrea@gmail.com Fixes: c9becf58d935 ("[PATCH] kretprobe: kretprobe-booster") Cc: stable@vger.kernel.org

---

arch/x86/kernel/kprobes/core.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index 69b6400d1ce2..f4b954ff5b89 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -749,11 +749,16 @@ asm( NOKPROBE_SYMBOL(kretprobe_trampoline); STACK_FRAME_NON_STANDARD(kretprobe_trampoline); +static struct kprobe kretprobe_kprobe = {

• .addr = (void *)kretprobe_trampoline,

+};

/*

• Called from kretprobe_trampoline

*/ static __used void *trampoline_handler(struct pt_regs *regs) {

• struct kprobe_ctlblk *kcb; struct kretprobe_instance *ri = NULL; struct hlist_head *head, empty_rp; struct hlist_node *tmp;

@@ -763,6 +768,17 @@ static __used void *trampoline_handler(struct pt_regs *regs) void *frame_pointer; bool skipped = false;

• preempt_disable();
• /*

• * Set a dummy kprobe for avoiding kretprobe recursion.
  

• * Since kretprobe never run in kprobe handler, kprobe must not
  

• * be running at this point.
  

• */
  

• kcb = get_kprobe_ctlblk();
• __this_cpu_write(current_kprobe, &kretprobe_kprobe);

If an interrupt comes in here, is this still safe, if the interrupt handler has a kretprobe too?

-- Steve

• kcb->kprobe_status = KPROBE_HIT_ACTIVE;
• INIT_HLIST_HEAD(&empty_rp); kretprobe_hash_lock(current, &head, &flags); /* fixup registers */

@@ -838,10 +854,9 @@ static __used void *trampoline_handler(struct pt_regs *regs) orig_ret_address = (unsigned long)ri->ret_addr; if (ri->rp && ri->rp->handler) { __this_cpu_write(current_kprobe, &ri->rp->kp);

• 	get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
  ri->ret_addr = correct_ret_addr;
  ri->rp->handler(ri, regs);
  

• 	__this_cpu_write(current_kprobe, NULL);
  

• 	__this_cpu_write(current_kprobe, &kretprobe_kprobe);
  

  }

recycle_rp_inst(ri, &empty_rp); @@ -857,6 +872,9 @@ static __used void *trampoline_handler(struct pt_regs *regs) kretprobe_hash_unlock(current, &flags);

• __this_cpu_write(current_kprobe, NULL);
• preempt_enable();
• hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) { hlist_del(&ri->hlist); kfree(ri);