On Fri, Mar 12, 2021 at 03:41:44PM +0900, Masami Hiramatsu wrote: > 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. 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. diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index 06f33bfebc50..70188fdd288e 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -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" diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c index 1d1b9388a1b1..1d3de84d2410 100644 --- a/arch/x86/kernel/unwind_orc.c +++ b/arch/x86/kernel/unwind_orc.c @@ -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);
