kprobes: Fix stacktrace with kretprobes on x86

Remove trampoline_address from kretprobe_trampoline_handler(). Instead of passing the address, kretprobe_trampoline_handler() can use new kretprobe_trampoline_addr().

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

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v3:
- Remove wrong kretprobe_trampoline declaration from

     arch/x86/include/asm/kprobes.h.
 Changes in v2:
   - Remove arch_deref_entry_point() from comment.
---
 arch/arc/kernel/kprobes.c          |    2 +-
 arch/arm/probes/kprobes/core.c     |    3 +--
 arch/arm64/kernel/probes/kprobes.c |    3 +--
 arch/csky/kernel/probes/kprobes.c  |    2 +-
 arch/ia64/kernel/kprobes.c         |    5 ++---
 arch/mips/kernel/kprobes.c         |    3 +--
 arch/parisc/kernel/kprobes.c       |    4 ++--
 arch/powerpc/kernel/kprobes.c      |    2 +-
 arch/riscv/kernel/probes/kprobes.c |    2 +-
 arch/s390/kernel/kprobes.c         |    2 +-
 arch/sh/kernel/kprobes.c           |    2 +-
 arch/sparc/kernel/kprobes.c        |    2 +-
 arch/x86/include/asm/kprobes.h     |    1 -
 arch/x86/kernel/kprobes/core.c     |    2 +-
 include/linux/kprobes.h            |   18 +++++++++++++-----
 kernel/kprobes.c                   |    3 +--
 16 files changed, 29 insertions(+), 27 deletions(-)

include/linux/kprobes.h +0 -0

index d65c041b5c22..65dadd4238a2 100644
@@ -205,15 +205,23 @@ extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				   struct pt_regs *regs);

Hi Masami,

Quoted 31 lines · Masami Hiramatsu

Remove trampoline_address from kretprobe_trampoline_handler(). Instead of passing the address, kretprobe_trampoline_handler() can use new kretprobe_trampoline_addr().

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

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v3:
- Remove wrong kretprobe_trampoline declaration from

     arch/x86/include/asm/kprobes.h.
 Changes in v2:
   - Remove arch_deref_entry_point() from comment.
---
 arch/arc/kernel/kprobes.c          |    2 +-
 arch/arm/probes/kprobes/core.c     |    3 +--
 arch/arm64/kernel/probes/kprobes.c |    3 +--
 arch/csky/kernel/probes/kprobes.c  |    2 +-
 arch/ia64/kernel/kprobes.c         |    5 ++---
 arch/mips/kernel/kprobes.c         |    3 +--
 arch/parisc/kernel/kprobes.c       |    4 ++--
 arch/powerpc/kernel/kprobes.c      |    2 +-
 arch/riscv/kernel/probes/kprobes.c |    2 +-
 arch/s390/kernel/kprobes.c         |    2 +-
 arch/sh/kernel/kprobes.c           |    2 +-
 arch/sparc/kernel/kprobes.c        |    2 +-
 arch/x86/include/asm/kprobes.h     |    1 -
 arch/x86/kernel/kprobes/core.c     |    2 +-
 include/linux/kprobes.h            |   18 +++++++++++++-----
 kernel/kprobes.c                   |    3 +--
 16 files changed, 29 insertions(+), 27 deletions(-)

<snip>

Quoted 8 lines

include/linux/kprobes.h +0 -0

index d65c041b5c22..65dadd4238a2 100644
@@ -205,15 +205,23 @@ extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				   struct pt_regs *regs);
 extern int arch_trampoline_kprobe(struct kprobe *p);
 

include/linux/kprobes.h extern void arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 215

Quoted 8 lines

include/linux/kprobes.h +8 -0

+void kretprobe_trampoline(void);
+/*
+ * Since some architecture uses structured function pointer,
+ * use dereference_function_descriptor() to get real function address.
+ */
+static nokprobe_inline void *kretprobe_trampoline_addr(void)
+{
+	return dereference_function_descriptor(kretprobe_trampoline);

I'm afraid this won't work correctly. For kernel functions, please use dereference_kernel_function_descriptor() which checks if the function has a descriptor before dereferencing it.

Thanks,
Naveen

To simplify the stacktrace with pt_regs from kretprobe handler, set the correct return address to the instruction pointer in the pt_regs before calling kretprobe handlers.

Suggested-by Josh Poimboeuf <[email protected]>

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

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v3:
- Cast the correct_ret_addr to unsigned long.

---
 kernel/kprobes.c |    3 +++
 1 file changed, 3 insertions(+)

kernel/kprobes.c +0 -0

index 54e5b89aad67..1598aca375c9 100644

Quoted 20 lines · Masami Hiramatsu

To simplify the stacktrace with pt_regs from kretprobe handler, set the correct return address to the instruction pointer in the pt_regs before calling kretprobe handlers.

Suggested-by Josh Poimboeuf <[email protected]>

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

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v3:
- Cast the correct_ret_addr to unsigned long.

---
 kernel/kprobes.c |    3 +++
 1 file changed, 3 insertions(+)

kernel/kprobes.c +0 -0

index 54e5b89aad67..1598aca375c9 100644

kernel/kprobes.c unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, · line 1922

Quoted 9 lines

kernel/kprobes.c +3 -0

@@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
 		BUG_ON(1);
 	}
 
+	/* Set the instruction pointer to the correct address */
+	instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
+
 	/* Run them. */
 	first = current->kretprobe_instances.first;
 	while (first) {

Hi Masami,

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 38 lines · Josh Poimboeuf

Quoted 20 lines · Masami Hiramatsu

To simplify the stacktrace with pt_regs from kretprobe handler, set the correct return address to the instruction pointer in the pt_regs before calling kretprobe handlers.

Suggested-by Josh Poimboeuf <[email protected]>

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

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v3:
- Cast the correct_ret_addr to unsigned long.

---
 kernel/kprobes.c |    3 +++
 1 file changed, 3 insertions(+)

kernel/kprobes.c +0 -0

index 54e5b89aad67..1598aca375c9 100644

kernel/kprobes.c unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, · line 1922

Quoted 9 lines

kernel/kprobes.c +3 -0

@@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
 		BUG_ON(1);
 	}
 
+	/* Set the instruction pointer to the correct address */
+	instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
+
 	/* Run them. */
 	first = current->kretprobe_instances.first;
 	while (first) {

Hi Masami,

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

Quoted 2 lines

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

... and I guess this means patches 6-8 are no longer necessary.

Signature · 2 lines

-- 
Josh

Quoted 39 lines · On Wed, 16 Jun 2021 23:40:32 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 38 lines · Josh Poimboeuf

Quoted 20 lines · Masami Hiramatsu

To simplify the stacktrace with pt_regs from kretprobe handler, set the correct return address to the instruction pointer in the pt_regs before calling kretprobe handlers.

Suggested-by Josh Poimboeuf <[email protected]>

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

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v3:
- Cast the correct_ret_addr to unsigned long.

---
 kernel/kprobes.c |    3 +++
 1 file changed, 3 insertions(+)

kernel/kprobes.c +0 -0

index 54e5b89aad67..1598aca375c9 100644

kernel/kprobes.c unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, · line 1922

Quoted 9 lines

kernel/kprobes.c +3 -0

@@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
 		BUG_ON(1);
 	}
 
+	/* Set the instruction pointer to the correct address */
+	instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
+
 	/* Run them. */
 	first = current->kretprobe_instances.first;
 	while (first) {

Hi Masami,

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

Quoted 4 lines

Quoted 2 lines

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Thank you,

Quoted 4 lines

Signature · 2 lines

-- 
Josh

Signature · 1 line

Masami Hiramatsu <[email protected]>

Quoted 45 lines · On Thu, 17 Jun 2021 23:40:01 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 42 lines · On Wed, 16 Jun 2021 23:40:32 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 40 lines · Josh Poimboeuf

Quoted 20 lines · Masami Hiramatsu

To simplify the stacktrace with pt_regs from kretprobe handler, set the correct return address to the instruction pointer in the pt_regs before calling kretprobe handlers.

Suggested-by Josh Poimboeuf <[email protected]>

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

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v3:
- Cast the correct_ret_addr to unsigned long.

---
 kernel/kprobes.c |    3 +++
 1 file changed, 3 insertions(+)

kernel/kprobes.c +0 -0

index 54e5b89aad67..1598aca375c9 100644

kernel/kprobes.c unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, · line 1922

Quoted 9 lines

kernel/kprobes.c +3 -0

@@ -1914,6 +1914,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
                BUG_ON(1);
        }
 
+       /* Set the instruction pointer to the correct address */
+       instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
+
        /* Run them. */
        first = current->kretprobe_instances.first;
        while (first) {

Hi Masami,

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

This changes the kretprobe to pass the return address via regs->ip to handler. Dynamic-event doesn't use it, but I'm not sure bcc is using it or not.

Thank you,

Quoted 11 lines

Thank you,

Quoted 4 lines

--
Josh

--
Masami Hiramatsu <[email protected]>

--
Masami Hiramatsu <[email protected]>

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

Hi Masami,

Dropping this patch and leaving all the other in place breaks stack traces from kretprobes for BPF. I double checked with and without this patch. Without this patch we are back to having broken stack traces. I see either

kretprobe_trampoline+0x0

or

ftrace_trampoline+0xc8
kretprobe_trampoline+0x0

Is there any problem if you leave this patch as is?

Quoted 31 lines · Andrii Nakryiko

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

Hi Masami,

Dropping this patch and leaving all the other in place breaks stack traces from kretprobes for BPF. I double checked with and without this patch. Without this patch we are back to having broken stack traces. I see either

kretprobe_trampoline+0x0

or

ftrace_trampoline+0xc8
kretprobe_trampoline+0x0

Quoted 1 line

Is there any problem if you leave this patch as is?

Hm, I must be missing something then. The patch is probably fine to keep, we just may need to improve the commit log so that it makes sense to me.

Which unwinder are you using (CONFIG_UNWINDER_*)?

--
Josh

Quoted 26 lines · Josh Poimboeuf

Quoted 24 lines · Andrii Nakryiko

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

Hi Masami,

Dropping this patch and leaving all the other in place breaks stack traces from kretprobes for BPF. I double checked with and without this patch. Without this patch we are back to having broken stack traces. I see either

Quoted 6 lines

Quoted 6 lines

kretprobe_trampoline+0x0

or

ftrace_trampoline+0xc8
kretprobe_trampoline+0x0

Quoted 27 lines · Andrii Nakryiko

Quoted 26 lines · Josh Poimboeuf

Quoted 24 lines · Andrii Nakryiko

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

Hi Masami,

Dropping this patch and leaving all the other in place breaks stack traces from kretprobes for BPF. I double checked with and without this patch. Without this patch we are back to having broken stack traces. I see either

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

kretprobe_trampoline+0x0

or

ftrace_trampoline+0xc8
kretprobe_trampoline+0x0

Do the stack traces end there? Or do they continue normally after that?

--
Josh

Quoted 31 lines · Josh Poimboeuf

Quoted 29 lines · Andrii Nakryiko

Quoted 28 lines · Josh Poimboeuf

Quoted 26 lines · Andrii Nakryiko

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

Hi Masami,

Dropping this patch and leaving all the other in place breaks stack traces from kretprobes for BPF. I double checked with and without this patch. Without this patch we are back to having broken stack traces. I see either

kretprobe_trampoline+0x0

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

or

ftrace_trampoline+0xc8
kretprobe_trampoline+0x0

Do the stack traces end there? Or do they continue normally after that?

That's the entire stack trace.

Quoted 32 lines · On Thu, 17 Jun 2021 12:46:19 -0700 Andrii Nakryiko <[email protected]>…

Quoted 31 lines · Josh Poimboeuf

Quoted 29 lines · Andrii Nakryiko

Quoted 28 lines · Josh Poimboeuf

Quoted 26 lines · Andrii Nakryiko

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

Hi Masami,

Dropping this patch and leaving all the other in place breaks stack traces from kretprobes for BPF. I double checked with and without this patch. Without this patch we are back to having broken stack traces. I see either

kretprobe_trampoline+0x0

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

or

ftrace_trampoline+0xc8
kretprobe_trampoline+0x0

Do the stack traces end there? Or do they continue normally after that?

That's the entire stack trace.

So, there are 2 cases of the stacktrace from inside the kretprobe handler.

1) Call stack_trace_save() in the handler. This will unwind stack from the handler's context. This is the case of the ftrace dynamic events.

2) Call stack_trace_save_regs(regs) in the handler with the pt_regs passed by the kretprobe. This is the case of ebpf.

For the case 1, these patches can be dropped because ORC can unwind the stack with UNWIND_HINT_FUNC. For the case 2, regs->ip must be set to the correct (return) address so that ORC can find the correct entry from that ip.

Quoted 24 lines · Andrii Nakryiko

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

Hi Masami,

Dropping this patch and leaving all the other in place breaks stack traces from kretprobes for BPF. I double checked with and without this patch. Without this patch we are back to having broken stack traces. I see either

Quoted 6 lines

kretprobe_trampoline+0x0

or

ftrace_trampoline+0xc8
kretprobe_trampoline+0x0

Quoted 2 lines

Is there any problem if you leave this patch as is?

Hm, I must be missing something then. The patch is probably fine to keep, we just may need to improve the commit log so that it makes sense to me.

Which unwinder are you using (CONFIG_UNWINDER_*)?

Signature · 1 line

Josh

Quoted 25 lines · On Thu, 17 Jun 2021 13:21:59 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 24 lines · Andrii Nakryiko

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

I know I suggested this patch, but I believe it would only be useful in combination with the use of UNWIND_HINT_REGS in SAVE_REGS_STRING. But I think that would be tricky to pull off correctly. Instead, we have UNWIND_HINT_FUNC, which is working fine.

So I'd suggest dropping this patch, as the unwinder isn't actually reading regs->ip after all.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

... and I guess this means patches 6-8 are no longer necessary.

OK, I also confirmed that dropping those patche does not make any change on the stacktrace.
Let me update the series without those.

Oops, Andrii, can you also test the kernel without this patch? (you don't need to drop patch 6-8)

Hi Masami,

Dropping this patch and leaving all the other in place breaks stack traces from kretprobes for BPF. I double checked with and without this patch. Without this patch we are back to having broken stack traces. I see either

Quoted 6 lines

Quoted 6 lines

kretprobe_trampoline+0x0

or

ftrace_trampoline+0xc8
kretprobe_trampoline+0x0

Thanks for confirmation.

Quoted 1 line

Quoted 1 line

Quoted 5 lines

Quoted 1 line

Is there any problem if you leave this patch as is?

Hm, I must be missing something then. The patch is probably fine to keep, we just may need to improve the commit log so that it makes sense to me.

Yeah, I need to update the commit message so that this will help the stacktrace from kretprobe's pt_regs, which will be used in bpf.

This changes x86/kretprobe stack frame on kretprobe_trampoline a bit, which now push the kretprobe_trampoline as a fake return address at the bottom of the stack frame. With this fix, the ORC unwinder will see the kretprobe_trampoline as a return address.

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

Suggested-by Josh Poimboeuf <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

Since the kretprobe replaces the function return address with the kretprobe_trampoline on the stack, x86 unwinders can not continue the stack unwinding at that point, or record kretprobe_trampoline instead of correct return address.

To fix this issue, find the correct return address from task's kretprobe_instances as like as function-graph tracer does.

With this fix, the unwinder can correctly unwind the stack from kretprobe event on x86, as below.

           <...>-135     [003] ...1     6.722338: r_full_proxy_read_0: (vfs_read+0xab/0x1a0 <- full_proxy_read)
           <...>-135     [003] ...1     6.722377: <stack trace>

=> kretprobe_trace_func+0x209/0x2f0
=> kretprobe_dispatcher+0x4a/0x70
=> __kretprobe_trampoline_handler+0xca/0x150
=> trampoline_handler+0x44/0x70
=> kretprobe_trampoline+0x2a/0x50
=> vfs_read+0xab/0x1a0
=> ksys_read+0x5f/0xe0
=> do_syscall_64+0x33/0x40
=> entry_SYSCALL_64_after_hwframe+0x44/0xae

Reported-by Daniel Xu <[email protected]>

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

Suggested-by Josh Poimboeuf <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

Hello,

Here is the 7th version of the series to fix the stacktrace with kretprobe on x86.

The previous version is;

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

This version is adding Tested-by from Andrii and do minor cleanups to solve some warnings from kernel test bots.

Changes from v6:
For x86 and generic patch:
- Add Andrii's Tested-by. (Andrii, I think you have tested only x86, is it OK?)

[11/13]:
- Remove superfluous #include <linux/kprobes.h>.
[13/13]:
- Add a prototype for arch_kretprobe_fixup_return().

With this series, unwinder can unwind stack correctly from ftrace as below;

  # cd /sys/kernel/debug/tracing
  # echo > trace
  # echo 1 > options/sym-offset
  # echo r vfs_read >> kprobe_events
  # echo r full_proxy_read >> kprobe_events
  # echo traceoff:1 > events/kprobes/r_vfs_read_0/trigger
  # echo stacktrace:1 > events/kprobes/r_full_proxy_read_0/trigger
  # echo 1 > events/kprobes/enable
  # cat /sys/kernel/debug/kprobes/list
ffffffff8133b740  r  full_proxy_read+0x0    [FTRACE]
ffffffff812560b0  r  vfs_read+0x0    [FTRACE]
  # echo 0 > events/kprobes/enable
  # cat trace
# tracer: nop
#
# entries-in-buffer/entries-written: 3/3   #P:8
#
#                                _-----=> irqs-off
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| /     delay
#           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
#              | |         |   ||||      |         |
           <...>-134     [007] ...1    16.185877: r_full_proxy_read_0: (vfs_read+0x98/0x180 <- full_proxy_read)
           <...>-134     [007] ...1    16.185901: <stack trace>
 => kretprobe_trace_func+0x209/0x300
 => kretprobe_dispatcher+0x4a/0x70
 => __kretprobe_trampoline_handler+0xd4/0x170
 => trampoline_handler+0x43/0x60
 => kretprobe_trampoline+0x2a/0x50
 => vfs_read+0x98/0x180
 => ksys_read+0x5f/0xe0
 => do_syscall_64+0x37/0x90
 => entry_SYSCALL_64_after_hwframe+0x44/0xae
           <...>-134     [007] ...1    16.185902: r_vfs_read_0: (ksys_read+0x5f/0xe0 <- vfs_read)

This shows the double return probes (vfs_read and full_proxy_read) on the stack correctly unwinded. (vfs_read will return to ksys_read+0x5f and full_proxy_read will return to vfs_read+0x98)

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.

You can also get this series from
git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git kprobes/kretprobe-stackfix-v7

Thank you,

---

Josh Poimboeuf (1):
x86/kprobes: Add UNWIND_HINT_FUNC on kretprobe_trampoline code

Masami Hiramatsu (12):
ia64: kprobes: Fix to pass correct trampoline address to the handler
kprobes: treewide: Replace arch_deref_entry_point() with dereference_symbol_descriptor()
kprobes: treewide: Remove trampoline_address from kretprobe_trampoline_handler()
kprobes: Add kretprobe_find_ret_addr() for searching return address
ARC: Add instruction_pointer_set() API
ia64: Add instruction_pointer_set() API
arm: kprobes: Make a space for regs->ARM_pc at kretprobe_trampoline
kprobes: Setup instruction pointer in __kretprobe_trampoline_handler x86/kprobes: Push a fake return address at kretprobe_trampoline x86/unwind: Recover kretprobe trampoline entry
tracing: Show kretprobe unknown indicator only for kretprobe_trampoline x86/kprobes: Fixup return address in generic trampoline handler

 arch/arc/include/asm/ptrace.h       |    5 ++
 arch/arc/kernel/kprobes.c           |    2 -
 arch/arm/probes/kprobes/core.c      |    5 +-
 arch/arm64/kernel/probes/kprobes.c  |    3 -
 arch/csky/kernel/probes/kprobes.c   |    2 -
 arch/ia64/include/asm/ptrace.h      |    5 ++
 arch/ia64/kernel/kprobes.c          |   15 ++---
 arch/mips/kernel/kprobes.c          |    3 -
 arch/parisc/kernel/kprobes.c        |    4 +
 arch/powerpc/kernel/kprobes.c       |   13 ----
 arch/riscv/kernel/probes/kprobes.c  |    2 -
 arch/s390/kernel/kprobes.c          |    2 -
 arch/sh/kernel/kprobes.c            |    2 -
 arch/sparc/kernel/kprobes.c         |    2 -
 arch/x86/include/asm/kprobes.h      |    1
 arch/x86/include/asm/unwind.h       |   23 +++++++
 arch/x86/include/asm/unwind_hints.h |    5 ++
 arch/x86/kernel/kprobes/core.c      |   53 +++++++++++++++--
 arch/x86/kernel/unwind_frame.c      |    3 -
 arch/x86/kernel/unwind_guess.c      |    3 -
 arch/x86/kernel/unwind_orc.c        |   18 +++++-
 include/linux/kprobes.h             |   44 ++++++++++++--
 kernel/kprobes.c                    |  108 +++++++++++++++++++++++++----------
 kernel/trace/trace_output.c         |   17 +-----
 lib/error-inject.c                  |    3 +
 25 files changed, 238 insertions(+), 105 deletions(-)

--
Masami Hiramatsu (Linaro) <[email protected]>

Hello,

Here is the 7th version of the series to fix the stacktrace with kretprobe on x86.

The previous version is;

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

This version is adding Tested-by from Andrii and do minor cleanups to solve some warnings from kernel test bots.

Changes from v6:
For x86 and generic patch:
- Add Andrii's Tested-by. (Andrii, I think you have tested only x86, is it OK?)
[11/13]:
- Remove superfluous #include <linux/kprobes.h>.
[13/13]:
- Add a prototype for arch_kretprobe_fixup_return().

With this series, unwinder can unwind stack correctly from ftrace as below;

  # cd /sys/kernel/debug/tracing
  # echo > trace
  # echo 1 > options/sym-offset
  # echo r vfs_read >> kprobe_events
  # echo r full_proxy_read >> kprobe_events
  # echo traceoff:1 > events/kprobes/r_vfs_read_0/trigger
  # echo stacktrace:1 > events/kprobes/r_full_proxy_read_0/trigger
  # echo 1 > events/kprobes/enable
  # cat /sys/kernel/debug/kprobes/list
ffffffff8133b740  r  full_proxy_read+0x0    [FTRACE]
ffffffff812560b0  r  vfs_read+0x0    [FTRACE]
  # echo 0 > events/kprobes/enable
  # cat trace
# tracer: nop
#
# entries-in-buffer/entries-written: 3/3   #P:8
#
#                                _-----=> irqs-off
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| /     delay
#           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
#              | |         |   ||||      |         |
           <...>-134     [007] ...1    16.185877: r_full_proxy_read_0: (vfs_read+0x98/0x180 <- full_proxy_read)
           <...>-134     [007] ...1    16.185901: <stack trace>
 => kretprobe_trace_func+0x209/0x300
 => kretprobe_dispatcher+0x4a/0x70
 => __kretprobe_trampoline_handler+0xd4/0x170
 => trampoline_handler+0x43/0x60
 => kretprobe_trampoline+0x2a/0x50
 => vfs_read+0x98/0x180
 => ksys_read+0x5f/0xe0
 => do_syscall_64+0x37/0x90
 => entry_SYSCALL_64_after_hwframe+0x44/0xae
           <...>-134     [007] ...1    16.185902: r_vfs_read_0: (ksys_read+0x5f/0xe0 <- vfs_read)

This shows the double return probes (vfs_read and full_proxy_read) on the stack correctly unwinded. (vfs_read will return to ksys_read+0x5f and full_proxy_read will return to vfs_read+0x98)

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.

You can also get this series from
git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git kprobes/kretprobe-stackfix-v7

Thank you,

---

Josh Poimboeuf (1):
x86/kprobes: Add UNWIND_HINT_FUNC on kretprobe_trampoline code

Masami Hiramatsu (12):
ia64: kprobes: Fix to pass correct trampoline address to the handler
kprobes: treewide: Replace arch_deref_entry_point() with dereference_symbol_descriptor()
kprobes: treewide: Remove trampoline_address from kretprobe_trampoline_handler()
kprobes: Add kretprobe_find_ret_addr() for searching return address
ARC: Add instruction_pointer_set() API
ia64: Add instruction_pointer_set() API
arm: kprobes: Make a space for regs->ARM_pc at kretprobe_trampoline
kprobes: Setup instruction pointer in __kretprobe_trampoline_handler x86/kprobes: Push a fake return address at kretprobe_trampoline x86/unwind: Recover kretprobe trampoline entry
tracing: Show kretprobe unknown indicator only for kretprobe_trampoline x86/kprobes: Fixup return address in generic trampoline handler

 arch/arc/include/asm/ptrace.h       |    5 ++
 arch/arc/kernel/kprobes.c           |    2 -
 arch/arm/probes/kprobes/core.c      |    5 +-
 arch/arm64/kernel/probes/kprobes.c  |    3 -
 arch/csky/kernel/probes/kprobes.c   |    2 -
 arch/ia64/include/asm/ptrace.h      |    5 ++
 arch/ia64/kernel/kprobes.c          |   15 ++---
 arch/mips/kernel/kprobes.c          |    3 -
 arch/parisc/kernel/kprobes.c        |    4 +
 arch/powerpc/kernel/kprobes.c       |   13 ----
 arch/riscv/kernel/probes/kprobes.c  |    2 -
 arch/s390/kernel/kprobes.c          |    2 -
 arch/sh/kernel/kprobes.c            |    2 -
 arch/sparc/kernel/kprobes.c         |    2 -
 arch/x86/include/asm/kprobes.h      |    1 
 arch/x86/include/asm/unwind.h       |   23 +++++++
 arch/x86/include/asm/unwind_hints.h |    5 ++
 arch/x86/kernel/kprobes/core.c      |   53 +++++++++++++++--
 arch/x86/kernel/unwind_frame.c      |    3 -
 arch/x86/kernel/unwind_guess.c      |    3 -
 arch/x86/kernel/unwind_orc.c        |   18 +++++-
 include/linux/kprobes.h             |   44 ++++++++++++--
 kernel/kprobes.c                    |  108 +++++++++++++++++++++++++----------
 kernel/trace/trace_output.c         |   17 +-----
 lib/error-inject.c                  |    3 +
 25 files changed, 238 insertions(+), 105 deletions(-)

--
Masami Hiramatsu (Linaro) <[email protected]>