kprobes: Fix stacktrace with kretprobes

Replace arch_deref_entry_point() with dereference_function_descriptor() because those are doing same thing.

Quoted 2 lines · Masami Hiramatsu

Replace arch_deref_entry_point() with dereference_function_descriptor() because those are doing same thing.

It's not quite the same -- you need dereference_symbol_descriptor().

- Naveen

Recover the return address on the stack which changed by the kretprobe. Note that this does not recover the address on the !current stack trace if CONFIG_ARCH_STACKWALK=n because old stack trace interface doesn't lock the stack in the generic stack_trace_save*() functions.

So with this patch, ftrace correctly shows the stacktrace as below;

 # echo r vfs_read > kprobe_events
 # echo stacktrace > events/kprobes/r_vfs_read_0/trigger
 # echo 1 > events/kprobes/r_vfs_read_0/enable
 # echo 1 > options/sym-offset
 # less trace
...

              sh-132     [007] ...1    22.524917: <stack trace>

=> kretprobe_dispatcher+0x7d/0xc0
=> __kretprobe_trampoline_handler+0xdb/0x1b0
=> trampoline_handler+0x48/0x60
=> kretprobe_trampoline+0x2a/0x50
=> ksys_read+0x70/0xf0
=> __x64_sys_read+0x1a/0x20
=> do_syscall_64+0x38/0x50
=> entry_SYSCALL_64_after_hwframe+0x44/0xae
=> 0

The trampoline_handler+0x48 is actual call site address, not modified by kretprobe.

Reported-by Daniel Xu <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes in v2:
- Add is_kretprobe_trampoline() for checking address outside of kretprobe_find_ret_addr()
- Remove unneeded addr from kretprobe_find_ret_addr()
- Rename fixup_kretprobe_tramp_addr() to fixup_kretprobe_trampoline()

---
 include/linux/kprobes.h |   22 ++++++++++++++
 kernel/kprobes.c        |   73 ++++++++++++++++++++++++++++++-----------------
 kernel/stacktrace.c     |   22 ++++++++++++++
 3 files changed, 91 insertions(+), 26 deletions(-)

include/linux/kprobes.h +22 -0

index 65dadd4238a2..674b5adad281 100644
@@ -215,6 +215,14 @@ static nokprobe_inline void *kretprobe_trampoline_addr(void)
 	return dereference_function_descriptor(kretprobe_trampoline);
 }
 
+static nokprobe_inline bool is_kretprobe_trampoline(unsigned long addr)
+{
+	return (void *)addr == kretprobe_trampoline_addr();
+}
+
+unsigned long kretprobe_find_ret_addr(struct task_struct *tsk,
+				      struct llist_node **cur);
+
 /* If the trampoline handler called from a kprobe, use this version */
 unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
 					     void *frame_pointer);

@@ -514,6 +522,20 @@ static inline bool is_kprobe_optinsn_slot(unsigned long addr)
 }
 #endif
 
+#if !defined(CONFIG_KRETPROBES)
+static nokprobe_inline bool is_kretprobe_trampoline(unsigned long addr)
+{
+	return false;
+}
+
+static nokprobe_inline
+unsigned long kretprobe_find_ret_addr(struct task_struct *tsk,
+				      struct llist_node **cur)
+{
+	return 0;
+}
+#endif
+
 /* Returns true if kprobes handled the fault */
 static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs,
 					      unsigned int trap)

kernel/kprobes.c +47 -26

index 75c0a58c19c2..2550521ff64d 100644
@@ -1858,45 +1858,51 @@ static struct notifier_block kprobe_exceptions_nb = {
 
 #ifdef CONFIG_KRETPROBES
 
-unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
-					     void *frame_pointer)
+/* This assumes the tsk is current or the task which is not running. */
+unsigned long kretprobe_find_ret_addr(struct task_struct *tsk,
+				      struct llist_node **cur)
 {
-	kprobe_opcode_t *correct_ret_addr = NULL;
 	struct kretprobe_instance *ri = NULL;
-	struct llist_node *first, *node;
-	struct kretprobe *rp;
+	struct llist_node *node = *cur;
+
+	if (!node)
+		node = tsk->kretprobe_instances.first;
+	else
+		node = node->next;
 
-	/* Find all nodes for this frame. */
-	first = node = current->kretprobe_instances.first;
 	while (node) {
 		ri = container_of(node, struct kretprobe_instance, llist);
-
-		BUG_ON(ri->fp != frame_pointer);
-
 		if (ri->ret_addr != kretprobe_trampoline_addr()) {
-			correct_ret_addr = ri->ret_addr;
-			/*
-			 * This is the real return address. Any other
-			 * instances associated with this task are for
-			 * other calls deeper on the call stack
-			 */
-			goto found;
+			*cur = node;
+			return (unsigned long)ri->ret_addr;
 		}
-
 		node = node->next;
 	}
-	pr_err("Oops! Kretprobe fails to find correct return address.\n");
-	BUG_ON(1);
+	return 0;
+}
+NOKPROBE_SYMBOL(kretprobe_find_ret_addr);
 
-found:
-	/* Unlink all nodes for this frame. */
-	current->kretprobe_instances.first = node->next;
-	node->next = NULL;
+unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
+					     void *frame_pointer)
+{
+	kprobe_opcode_t *correct_ret_addr = NULL;
+	struct kretprobe_instance *ri = NULL;
+	struct llist_node *first, *node = NULL;
+	struct kretprobe *rp;
+
+	/* Find correct address and all nodes for this frame. */
+	correct_ret_addr = (void *)kretprobe_find_ret_addr(current, &node);
+	if (!correct_ret_addr) {
+		pr_err("Oops! Kretprobe fails to find correct return address.\n");
+		BUG_ON(1);
+	}
 
-	/* Run them..  */
+	/* Run them. */
+	first = current->kretprobe_instances.first;
 	while (first) {
 		ri = container_of(first, struct kretprobe_instance, llist);
-		first = first->next;
+
+		BUG_ON(ri->fp != frame_pointer);
 
 		rp = get_kretprobe(ri);
 		if (rp && rp->handler) {

@@ -1907,6 +1913,21 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
 			rp->handler(ri, regs);
 			__this_cpu_write(current_kprobe, prev);
 		}
+		if (first == node)
+			break;
+
+		first = first->next;
+	}
+
+	/* Unlink all nodes for this frame. */
+	first = current->kretprobe_instances.first;
+	current->kretprobe_instances.first = node->next;
+	node->next = NULL;
+
+	/* Recycle them.  */
+	while (first) {
+		ri = container_of(first, struct kretprobe_instance, llist);
+		first = first->next;
 
 		recycle_rp_inst(ri);
 	}

kernel/stacktrace.c +22 -0

index 9f8117c7cfdd..511287069473 100644
@@ -13,6 +13,7 @@
 #include <linux/export.h>
 #include <linux/kallsyms.h>
 #include <linux/stacktrace.h>
+#include <linux/kprobes.h>
 
 /**
  * stack_trace_print - Print the entries in the stack trace

@@ -69,6 +70,18 @@ int stack_trace_snprint(char *buf, size_t size, const unsigned long *entries,
 }
 EXPORT_SYMBOL_GPL(stack_trace_snprint);
 
+static void fixup_kretprobe_trampoline(unsigned long *store, unsigned int len,
+				       struct task_struct *tsk)
+{
+	struct llist_node *cur = NULL;
+
+	while (len--) {
+		if (is_kretprobe_trampoline(*store))
+			*store = kretprobe_find_ret_addr(tsk, &cur);
+		store++;
+	}
+}
+
 #ifdef CONFIG_ARCH_STACKWALK
 
 struct stacktrace_cookie {

@@ -119,6 +132,7 @@ unsigned int stack_trace_save(unsigned long *store, unsigned int size,
 	};
 
 	arch_stack_walk(consume_entry, &c, current, NULL);
+	fixup_kretprobe_trampoline(store, c.len, current);
 	return c.len;
 }
 EXPORT_SYMBOL_GPL(stack_trace_save);

@@ -147,6 +161,7 @@ unsigned int stack_trace_save_tsk(struct task_struct *tsk, unsigned long *store,
 		return 0;
 
 	arch_stack_walk(consume_entry, &c, tsk, NULL);
+	fixup_kretprobe_trampoline(store, c.len, tsk);
 	put_task_stack(tsk);
 	return c.len;
 }

@@ -171,6 +186,7 @@ unsigned int stack_trace_save_regs(struct pt_regs *regs, unsigned long *store,
 	};
 
 	arch_stack_walk(consume_entry, &c, current, regs);
+	fixup_kretprobe_trampoline(store, c.len, current);
 	return c.len;
 }
 

@@ -205,6 +221,8 @@ int stack_trace_save_tsk_reliable(struct task_struct *tsk, unsigned long *store,
 		return 0;
 
 	ret = arch_stack_walk_reliable(consume_entry, &c, tsk);
+	if (!ret)
+		fixup_kretprobe_trampoline(store, c.len, tsk);
 	put_task_stack(tsk);
 	return ret ? ret : c.len;
 }

@@ -276,6 +294,8 @@ unsigned int stack_trace_save(unsigned long *store, unsigned int size,
 	};
 
 	save_stack_trace(&trace);
+	fixup_kretprobe_trampoline(store, trace.nr_entries, current);
+
 	return trace.nr_entries;
 }
 EXPORT_SYMBOL_GPL(stack_trace_save);

@@ -323,6 +343,8 @@ unsigned int stack_trace_save_regs(struct pt_regs *regs, unsigned long *store,
 	};
 
 	save_stack_trace_regs(regs, &trace);
+	fixup_kretprobe_trampoline(store, trace.nr_entries, current);
+
 	return trace.nr_entries;
 }
 

Hello,

Here is the 2nd version of the series to fix the stacktrace with kretprobe.

The 1st series is here;

https://lore.kernel.org/bpf/161495873696.346821.10161501768906432924.stgit@devnote2/

In this version I merged the ORC unwinder fix for kretprobe which discussed in the previous thread. [3/10] is updated according to the Miroslav's comment. [4/10] is updated for simplify the code. [5/10]-[9/10] are discussed in the previsous tread and are introduced to the series.

Daniel, can you also test this again? I and Josh discussed a bit different method and I've implemented it on this version.

This actually changes the kretprobe behavisor a bit, now the instraction pointer in the pt_regs passed to kretprobe user handler is correctly set the real return address. So user handlers can get it via instruction_pointer() API.

Hello,

Here is the 2nd version of the series to fix the stacktrace with kretprobe.

The 1st series is here;

https://lore.kernel.org/bpf/161495873696.346821.10161501768906432924.stgit@devnote2/

In this version I merged the ORC unwinder fix for kretprobe which discussed in the previous thread. [3/10] is updated according to the Miroslav's comment. [4/10] is updated for simplify the code. [5/10]-[9/10] are discussed in the previsous tread and are introduced to the series.

Daniel, can you also test this again? I and Josh discussed a bit different method and I've implemented it on this version.

This actually changes the kretprobe behavisor a bit, now the instraction pointer in the pt_regs passed to kretprobe user handler is correctly set the real return address. So user handlers can get it via instruction_pointer() API.

Quoted 13 lines · Masami Hiramatsu

Hello,

Here is the 2nd version of the series to fix the stacktrace with kretprobe.

The 1st series is here;

https://lore.kernel.org/bpf/161495873696.346821.10161501768906432924.stgit@devnote2/

In this version I merged the ORC unwinder fix for kretprobe which discussed in the previous thread. [3/10] is updated according to the Miroslav's comment. [4/10] is updated for simplify the code. [5/10]-[9/10] are discussed in the previsous tread and are introduced to the series.

Quoted 6 lines

Daniel, can you also test this again? I and Josh discussed a bit different method and I've implemented it on this version.

This actually changes the kretprobe behavisor a bit, now the instraction pointer in the pt_regs passed to kretprobe user handler is correctly set the real return address. So user handlers can get it via instruction_pointer() API.

When I add WARN_ON(1) to a test kretprobe, it doesn't unwind properly.

show_trace_log_lvl() reads the entire stack in lockstep with calls to the unwinder so that it can decide which addresses get prefixed with '?'. So it expects to find an actual return address on the stack. Instead it finds %rsp. So it never syncs up with unwind_next_frame() and shows all remaining addresses as unreliable.

  Call Trace:
   __kretprobe_trampoline_handler+0xca/0x1a0
   trampoline_handler+0x3d/0x50
   kretprobe_trampoline+0x25/0x50
   ? init_test_probes.cold+0x323/0x387
   ? init_kprobes+0x144/0x14c
   ? init_optprobes+0x15/0x15
   ? do_one_initcall+0x5b/0x300
   ? lock_is_held_type+0xe8/0x140
   ? kernel_init_freeable+0x174/0x2cd
   ? rest_init+0x233/0x233
   ? kernel_init+0xa/0x11d
   ? ret_from_fork+0x22/0x30

How about pushing 'kretprobe_trampoline' instead of %rsp for the return address placeholder. That fixes the above test, and removes the need for the weird 'state->ip == sp' check:

  Call Trace:
   __kretprobe_trampoline_handler+0xca/0x150
   trampoline_handler+0x3d/0x50
   kretprobe_trampoline+0x29/0x50
   ? init_test_probes.cold+0x323/0x387
   elfcorehdr_read+0x10/0x10
   init_kprobes+0x144/0x14c
   ? init_optprobes+0x15/0x15
   do_one_initcall+0x72/0x280
   kernel_init_freeable+0x174/0x2cd
   ? rest_init+0x122/0x122
   kernel_init+0xa/0x10e
   ret_from_fork+0x22/0x30

Though, init_test_probes.cold() (the real return address) is still listed as unreliable. Maybe we need a call to kretprobe_find_ret_addr() in show_trace_log_lvl(), similar to the ftrace_graph_ret_addr() call there.

arch/x86/kernel/kprobes/core.c +3 -3

index 06f33bfebc50..70188fdd288e 100644
@@ -766,19 +766,19 @@ asm(
 	"kretprobe_trampoline:\n"
 	/* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
-	"	pushq %rsp\n"
+	/* Push fake return address to tell the unwinder it's a kretprobe */
+	"	pushq $kretprobe_trampoline\n"
 	UNWIND_HINT_FUNC
 	"	pushfq\n"
 	SAVE_REGS_STRING
 	"	movq %rsp, %rdi\n"
 	"	call trampoline_handler\n"
-	/* Replace saved sp with true return address. */
+	/* Replace the fake return address with the real one. */
 	"	movq %rax, 19*8(%rsp)\n"
 	RESTORE_REGS_STRING
 	"	popfq\n"
 #else
 	"	pushl %esp\n"
-	UNWIND_HINT_FUNC
 	"	pushfl\n"
 	SAVE_REGS_STRING
 	"	movl %esp, %eax\n"

arch/x86/kernel/unwind_orc.c +1 -2

index 1d1b9388a1b1..1d3de84d2410 100644
@@ -548,8 +548,7 @@ bool unwind_next_frame(struct unwind_state *state)
 		 * In those cases, find the correct return address from
 		 * task->kretprobe_instances list.
 		 */
-		if (state->ip == sp ||
-		    is_kretprobe_trampoline(state->ip))
+		if (is_kretprobe_trampoline(state->ip))
 			state->ip = kretprobe_find_ret_addr(state->task,
 							    &state->kr_iter);
 

Quoted 61 lines · On Mon, 15 Mar 2021 21:30:03 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 13 lines · Masami Hiramatsu

Hello,

Here is the 2nd version of the series to fix the stacktrace with kretprobe.

The 1st series is here;

https://lore.kernel.org/bpf/161495873696.346821.10161501768906432924.stgit@devnote2/

In this version I merged the ORC unwinder fix for kretprobe which discussed in the previous thread. [3/10] is updated according to the Miroslav's comment. [4/10] is updated for simplify the code. [5/10]-[9/10] are discussed in the previsous tread and are introduced to the series.

Quoted 6 lines

Daniel, can you also test this again? I and Josh discussed a bit different method and I've implemented it on this version.

This actually changes the kretprobe behavisor a bit, now the instraction pointer in the pt_regs passed to kretprobe user handler is correctly set the real return address. So user handlers can get it via instruction_pointer() API.

When I add WARN_ON(1) to a test kretprobe, it doesn't unwind properly.

show_trace_log_lvl() reads the entire stack in lockstep with calls to the unwinder so that it can decide which addresses get prefixed with '?'. So it expects to find an actual return address on the stack. Instead it finds %rsp. So it never syncs up with unwind_next_frame() and shows all remaining addresses as unreliable.

  Call Trace:
   __kretprobe_trampoline_handler+0xca/0x1a0
   trampoline_handler+0x3d/0x50
   kretprobe_trampoline+0x25/0x50
   ? init_test_probes.cold+0x323/0x387
   ? init_kprobes+0x144/0x14c
   ? init_optprobes+0x15/0x15
   ? do_one_initcall+0x5b/0x300
   ? lock_is_held_type+0xe8/0x140
   ? kernel_init_freeable+0x174/0x2cd
   ? rest_init+0x233/0x233
   ? kernel_init+0xa/0x11d
   ? ret_from_fork+0x22/0x30

How about pushing 'kretprobe_trampoline' instead of %rsp for the return address placeholder. That fixes the above test, and removes the need for the weird 'state->ip == sp' check:

  Call Trace:
   __kretprobe_trampoline_handler+0xca/0x150
   trampoline_handler+0x3d/0x50
   kretprobe_trampoline+0x29/0x50
   ? init_test_probes.cold+0x323/0x387
   elfcorehdr_read+0x10/0x10
   init_kprobes+0x144/0x14c
   ? init_optprobes+0x15/0x15
   do_one_initcall+0x72/0x280
   kernel_init_freeable+0x174/0x2cd
   ? rest_init+0x122/0x122
   kernel_init+0xa/0x10e
   ret_from_fork+0x22/0x30

Quoted 4 lines

Though, init_test_probes.cold() (the real return address) is still listed as unreliable. Maybe we need a call to kretprobe_find_ret_addr() in show_trace_log_lvl(), similar to the ftrace_graph_ret_addr() call there.

Thanks for the test!
OK, that could be acceptable. However, push %sp still needed for accessing stack address from kretprobe handler via pt_regs. (regs->sp must point the stack address)
Anyway, with int3, it pushes one more entry for emulating call, so I think it is OK.
Let me update the series!

Thank you!

Quoted 37 lines

arch/x86/kernel/kprobes/core.c +3 -3

index 06f33bfebc50..70188fdd288e 100644
@@ -766,19 +766,19 @@ asm(
      "kretprobe_trampoline:\n"
      /* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
-     "       pushq %rsp\n"
+     /* Push fake return address to tell the unwinder it's a kretprobe */
+     "       pushq $kretprobe_trampoline\n"
      UNWIND_HINT_FUNC
      "       pushfq\n"
      SAVE_REGS_STRING
      "       movq %rsp, %rdi\n"
      "       call trampoline_handler\n"
-     /* Replace saved sp with true return address. */
+     /* Replace the fake return address with the real one. */
      "       movq %rax, 19*8(%rsp)\n"
      RESTORE_REGS_STRING
      "       popfq\n"
 #else
      "       pushl %esp\n"
-     UNWIND_HINT_FUNC
      "       pushfl\n"
      SAVE_REGS_STRING
      "       movl %esp, %eax\n"

arch/x86/kernel/unwind_orc.c +0 -0

index 1d1b9388a1b1..1d3de84d2410 100644

arch/x86/kernel/unwind_orc.c bool unwind_next_frame(struct unwind_state *state) · line 554

Quoted 9 lines

arch/x86/kernel/unwind_orc.c +1 -2

@@ -548,8 +548,7 @@ bool unwind_next_frame(struct unwind_state *state)
               * In those cases, find the correct return address from
               * task->kretprobe_instances list.
               */
-             if (state->ip == sp ||
-                 is_kretprobe_trampoline(state->ip))
+             if (is_kretprobe_trampoline(state->ip))
                      state->ip = kretprobe_find_ret_addr(state->task,
                                                          &state->kr_iter);
 

--
Masami Hiramatsu <[email protected]>

Quoted 70 lines · Masami Hiramatsu

Quoted 61 lines · On Mon, 15 Mar 2021 21:30:03 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 13 lines · Masami Hiramatsu

Hello,

Here is the 2nd version of the series to fix the stacktrace with kretprobe.

The 1st series is here;

https://lore.kernel.org/bpf/161495873696.346821.10161501768906432924.stgit@devnote2/

In this version I merged the ORC unwinder fix for kretprobe which discussed in the previous thread. [3/10] is updated according to the Miroslav's comment. [4/10] is updated for simplify the code. [5/10]-[9/10] are discussed in the previsous tread and are introduced to the series.

Quoted 6 lines

Daniel, can you also test this again? I and Josh discussed a bit different method and I've implemented it on this version.

This actually changes the kretprobe behavisor a bit, now the instraction pointer in the pt_regs passed to kretprobe user handler is correctly set the real return address. So user handlers can get it via instruction_pointer() API.

When I add WARN_ON(1) to a test kretprobe, it doesn't unwind properly.

show_trace_log_lvl() reads the entire stack in lockstep with calls to the unwinder so that it can decide which addresses get prefixed with '?'. So it expects to find an actual return address on the stack. Instead it finds %rsp. So it never syncs up with unwind_next_frame() and shows all remaining addresses as unreliable.

  Call Trace:
   __kretprobe_trampoline_handler+0xca/0x1a0
   trampoline_handler+0x3d/0x50
   kretprobe_trampoline+0x25/0x50
   ? init_test_probes.cold+0x323/0x387
   ? init_kprobes+0x144/0x14c
   ? init_optprobes+0x15/0x15
   ? do_one_initcall+0x5b/0x300
   ? lock_is_held_type+0xe8/0x140
   ? kernel_init_freeable+0x174/0x2cd
   ? rest_init+0x233/0x233
   ? kernel_init+0xa/0x11d
   ? ret_from_fork+0x22/0x30

How about pushing 'kretprobe_trampoline' instead of %rsp for the return address placeholder. That fixes the above test, and removes the need for the weird 'state->ip == sp' check:

  Call Trace:
   __kretprobe_trampoline_handler+0xca/0x150
   trampoline_handler+0x3d/0x50
   kretprobe_trampoline+0x29/0x50
   ? init_test_probes.cold+0x323/0x387
   elfcorehdr_read+0x10/0x10
   init_kprobes+0x144/0x14c
   ? init_optprobes+0x15/0x15
   do_one_initcall+0x72/0x280
   kernel_init_freeable+0x174/0x2cd
   ? rest_init+0x122/0x122
   kernel_init+0xa/0x10e
   ret_from_fork+0x22/0x30

Quoted 4 lines

Though, init_test_probes.cold() (the real return address) is still listed as unreliable. Maybe we need a call to kretprobe_find_ret_addr() in show_trace_log_lvl(), similar to the ftrace_graph_ret_addr() call there.

Quoted 8 lines

Thanks for the test!
OK, that could be acceptable. However, push %sp still needed for accessing stack address from kretprobe handler via pt_regs. (regs->sp must point the stack address)
Anyway, with int3, it pushes one more entry for emulating call, so I think it is OK.
Let me update the series!

Hi Misami,

Did you get a chance to follow up on this? I checked v5.13-rc1 and it still has this issue. Inability to capture a stack trace from BPF kretprobes is a pretty big problem for some applications, it would be great to get this fixed. Thanks!

Quoted 54 lines

Thank you!

Quoted 37 lines

arch/x86/kernel/kprobes/core.c +3 -3

index 06f33bfebc50..70188fdd288e 100644
@@ -766,19 +766,19 @@ asm(
      "kretprobe_trampoline:\n"
      /* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
-     "       pushq %rsp\n"
+     /* Push fake return address to tell the unwinder it's a kretprobe */
+     "       pushq $kretprobe_trampoline\n"
      UNWIND_HINT_FUNC
      "       pushfq\n"
      SAVE_REGS_STRING
      "       movq %rsp, %rdi\n"
      "       call trampoline_handler\n"
-     /* Replace saved sp with true return address. */
+     /* Replace the fake return address with the real one. */
      "       movq %rax, 19*8(%rsp)\n"
      RESTORE_REGS_STRING
      "       popfq\n"
 #else
      "       pushl %esp\n"
-     UNWIND_HINT_FUNC
      "       pushfl\n"
      SAVE_REGS_STRING
      "       movl %esp, %eax\n"

arch/x86/kernel/unwind_orc.c +0 -0

index 1d1b9388a1b1..1d3de84d2410 100644

arch/x86/kernel/unwind_orc.c bool unwind_next_frame(struct unwind_state *state) · line 554

Quoted 9 lines

arch/x86/kernel/unwind_orc.c +1 -2

@@ -548,8 +548,7 @@ bool unwind_next_frame(struct unwind_state *state)
               * In those cases, find the correct return address from
               * task->kretprobe_instances list.
               */
-             if (state->ip == sp ||
-                 is_kretprobe_trampoline(state->ip))
+             if (is_kretprobe_trampoline(state->ip))
                      state->ip = kretprobe_find_ret_addr(state->task,
                                                          &state->kr_iter);
 

--
Masami Hiramatsu <[email protected]>

Quoted 80 lines · On Mon, 17 May 2021 14:06:24 -0700 Andrii Nakryiko <[email protected]>…

Quoted 78 lines · Masami Hiramatsu

Quoted 61 lines · On Mon, 15 Mar 2021 21:30:03 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 13 lines · Masami Hiramatsu

Hello,

Here is the 2nd version of the series to fix the stacktrace with kretprobe.

The 1st series is here;

https://lore.kernel.org/bpf/161495873696.346821.10161501768906432924.stgit@devnote2/

In this version I merged the ORC unwinder fix for kretprobe which discussed in the previous thread. [3/10] is updated according to the Miroslav's comment. [4/10] is updated for simplify the code. [5/10]-[9/10] are discussed in the previsous tread and are introduced to the series.

Quoted 6 lines

Daniel, can you also test this again? I and Josh discussed a bit different method and I've implemented it on this version.

This actually changes the kretprobe behavisor a bit, now the instraction pointer in the pt_regs passed to kretprobe user handler is correctly set the real return address. So user handlers can get it via instruction_pointer() API.

When I add WARN_ON(1) to a test kretprobe, it doesn't unwind properly.

show_trace_log_lvl() reads the entire stack in lockstep with calls to the unwinder so that it can decide which addresses get prefixed with '?'. So it expects to find an actual return address on the stack. Instead it finds %rsp. So it never syncs up with unwind_next_frame() and shows all remaining addresses as unreliable.

  Call Trace:
   __kretprobe_trampoline_handler+0xca/0x1a0
   trampoline_handler+0x3d/0x50
   kretprobe_trampoline+0x25/0x50
   ? init_test_probes.cold+0x323/0x387
   ? init_kprobes+0x144/0x14c
   ? init_optprobes+0x15/0x15
   ? do_one_initcall+0x5b/0x300
   ? lock_is_held_type+0xe8/0x140
   ? kernel_init_freeable+0x174/0x2cd
   ? rest_init+0x233/0x233
   ? kernel_init+0xa/0x11d
   ? ret_from_fork+0x22/0x30

How about pushing 'kretprobe_trampoline' instead of %rsp for the return address placeholder. That fixes the above test, and removes the need for the weird 'state->ip == sp' check:

  Call Trace:
   __kretprobe_trampoline_handler+0xca/0x150
   trampoline_handler+0x3d/0x50
   kretprobe_trampoline+0x29/0x50
   ? init_test_probes.cold+0x323/0x387
   elfcorehdr_read+0x10/0x10
   init_kprobes+0x144/0x14c
   ? init_optprobes+0x15/0x15
   do_one_initcall+0x72/0x280
   kernel_init_freeable+0x174/0x2cd
   ? rest_init+0x122/0x122
   kernel_init+0xa/0x10e
   ret_from_fork+0x22/0x30

Quoted 4 lines

Though, init_test_probes.cold() (the real return address) is still listed as unreliable. Maybe we need a call to kretprobe_find_ret_addr() in show_trace_log_lvl(), similar to the ftrace_graph_ret_addr() call there.

Thanks for the test!
OK, that could be acceptable. However, push %sp still needed for accessing stack address from kretprobe handler via pt_regs. (regs->sp must point the stack address)
Anyway, with int3, it pushes one more entry for emulating call, so I think it is OK.
Let me update the series!

Quoted 6 lines

Hi Misami,

Did you get a chance to follow up on this? I checked v5.13-rc1 and it still has this issue. Inability to capture a stack trace from BPF kretprobes is a pretty big problem for some applications, it would be great to get this fixed. Thanks!

OK, let me rework this series.

Thank you,

Quoted 46 lines

Quoted 46 lines

Thank you!

Quoted 42 lines

arch/x86/kernel/kprobes/core.c +3 -3

index 06f33bfebc50..70188fdd288e 100644
@@ -766,19 +766,19 @@ asm(
      "kretprobe_trampoline:\n"
      /* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
-     "       pushq %rsp\n"
+     /* Push fake return address to tell the unwinder it's a kretprobe */
+     "       pushq $kretprobe_trampoline\n"
      UNWIND_HINT_FUNC
      "       pushfq\n"
      SAVE_REGS_STRING
      "       movq %rsp, %rdi\n"
      "       call trampoline_handler\n"
-     /* Replace saved sp with true return address. */
+     /* Replace the fake return address with the real one. */
      "       movq %rax, 19*8(%rsp)\n"
      RESTORE_REGS_STRING
      "       popfq\n"
 #else
      "       pushl %esp\n"
-     UNWIND_HINT_FUNC
      "       pushfl\n"
      SAVE_REGS_STRING
      "       movl %esp, %eax\n"

arch/x86/kernel/unwind_orc.c +1 -2

index 1d1b9388a1b1..1d3de84d2410 100644
@@ -548,8 +548,7 @@ bool unwind_next_frame(struct unwind_state *state)
               * In those cases, find the correct return address from
               * task->kretprobe_instances list.
               */
-             if (state->ip == sp ||
-                 is_kretprobe_trampoline(state->ip))
+             if (is_kretprobe_trampoline(state->ip))
                      state->ip = kretprobe_find_ret_addr(state->task,

Quoted 8 lines

Quoted 8 lines

arch/x86/kernel/unwind_orc.c bool unwind_next_frame(struct unwind_state *state) · line 554

Quoted 4 lines

arch/x86/kernel/unwind_orc.c +0 -0

                                                          &state->kr_iter);
 

--
Masami Hiramatsu <[email protected]>

--
Masami Hiramatsu <[email protected]>