x86/fgraph,bpf: Fix ORC stack unwind from return probe

This reverts commit 83f44ae0f8afcc9da659799db8693f74847e66b3.

Currently we store initial stacktrace entry twice for non-HW ot_regs, which means callers that fail perf_hw_regs(regs) condition in perf_callchain_kernel.

It's easy to reproduce this bpftrace:

  # bpftrace -e 'tracepoint:sched:sched_process_exec { print(kstack()); }'
  Attaching 1 probe...

        bprm_execve+1767
        bprm_execve+1767
        do_execveat_common.isra.0+425
        __x64_sys_execve+56
        do_syscall_64+133
        entry_SYSCALL_64_after_hwframe+118

When perf_callchain_kernel calls unwind_start with first_frame, AFAICS we do not skip regs->ip, but it's added as part of the unwind process. Hence reverting the extra perf_callchain_store for non-hw regs leg.

I was not able to bisect this, so I'm not really sure why this was needed in v5.2 and why it's not working anymore, but I could see double entries as far as v5.10.

arch/x86/include/asm/ftrace.h arch_ftrace_get_regs(struct ftrace_regs *fregs) · line 63

Quoted 13 lines

arch/x86/include/asm/ftrace.h +5 -0

index 93156ac4f..b08c95872 100644
@@ -56,6 +56,11 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
 	return &arch_ftrace_regs(fregs)->regs;
 }
 
+#define arch_ftrace_partial_regs(regs) do {	\
+	regs->flags &= ~X86_EFLAGS_FIXED;	\
+	regs->cs = __KERNEL_CS;			\
+} while (0)
+

Can arch_ftrace_partial_regs read uninitialized regs->flags?

In return_to_handler, the ftrace_regs structure is allocated on the stack but only a few fields are initialized (RAX, RDX, RBP, RSP). The EFLAGS/flags field is never written before arch_ftrace_partial_regs reads it:

arch/x86/kernel/ftrace_64.S return_to_handler:
subq $(FRAME_SIZE), %rsp # Allocate ftrace_regs on stack
movq %rax, RAX(%rsp)
movq %rdx, RDX(%rsp)
movq %rbp, RBP(%rsp)
movq %rsp, RSP(%rsp)
# No initialization of EFLAGS field

Then ftrace_partial_regs()->arch_ftrace_partial_regs() executes:

    regs->flags &= ~X86_EFLAGS_FIXED;

This reads the uninitialized flags field. Stack allocations contain garbage, so regs->flags will have whatever data was previously on the stack. The &= operation produces undefined results when operating on uninitialized memory.

For comparison, ftrace_regs_caller explicitly initializes EFLAGS:
movq MCOUNT_REG_SIZE(%rsp), %rcx
movq %rcx, EFLAGS(%rsp)

Should return_to_handler initialize regs->flags to 0 (or another appropriate value) before arch_ftrace_partial_regs modifies it?

Currently we don't get stack trace via ORC unwinder on top of fgraph exit handler. We can see that when generating stacktrace from kretprobe_multi bpf program which is based on fprobe/fgraph.

The reason is that the ORC unwind code won't get pass the return_to_handler callback installed by fgraph return probe machinery.

Solving this by creating stack frame in return_to_handler expected by ftrace_graph_ret_addr function to recover original return address and continue with the unwind.

Also updating the pt_regs data with cs/flags/rsp which are needed for successful stack retrieval from ebpf bpf_get_stackid helper.
- in get_perf_callchain we check user_mode(regs) so CS has to be set
- in perf_callchain_kernel we call perf_hw_regs(regs), so EFLAGS/FIXED has to be unset

Acked-by Masami Hiramatsu (Google) <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
 arch/x86/include/asm/ftrace.h |  5 +++++
 arch/x86/kernel/ftrace_64.S   |  8 +++++++-
 include/linux/ftrace.h        | 10 +++++++++-
 3 files changed, 21 insertions(+), 2 deletions(-)

arch/x86/include/asm/ftrace.h +0 -0

index 93156ac4ffe0..b08c95872eed 100644

arch/x86/include/asm/ftrace.h +0 -0

 #define arch_ftrace_fill_perf_regs(fregs, _regs) do {	\
 		(_regs)->ip = arch_ftrace_regs(fregs)->regs.ip;		\
 		(_regs)->sp = arch_ftrace_regs(fregs)->regs.sp;		\

arch/x86/kernel/ftrace_64.S +7 -1

index 367da3638167..823dbdd0eb41 100644
@@ -354,12 +354,17 @@ SYM_CODE_START(return_to_handler)
 	UNWIND_HINT_UNDEFINED
 	ANNOTATE_NOENDBR
 
+	/* Restore return_to_handler value that got eaten by previous ret instruction. */
+	subq $8, %rsp
+	UNWIND_HINT_FUNC
+
 	/* Save ftrace_regs for function exit context  */
 	subq $(FRAME_SIZE), %rsp
 
 	movq %rax, RAX(%rsp)
 	movq %rdx, RDX(%rsp)
 	movq %rbp, RBP(%rsp)
+	movq %rsp, RSP(%rsp)
 	movq %rsp, %rdi
 
 	call ftrace_return_to_handler

@@ -368,7 +373,8 @@ SYM_CODE_START(return_to_handler)
 	movq RDX(%rsp), %rdx
 	movq RAX(%rsp), %rax
 
-	addq $(FRAME_SIZE), %rsp
+	addq $(FRAME_SIZE) + 8, %rsp
+
 	/*
 	 * Jump back to the old return address. This cannot be JMP_NOSPEC rdi
 	 * since IBT would demand that contain ENDBR, which simply isn't so for

Quoted 33 lines · Jiri Olsa

Currently we don't get stack trace via ORC unwinder on top of fgraph exit handler. We can see that when generating stacktrace from kretprobe_multi bpf program which is based on fprobe/fgraph.

The reason is that the ORC unwind code won't get pass the return_to_handler callback installed by fgraph return probe machinery.

Solving this by creating stack frame in return_to_handler expected by ftrace_graph_ret_addr function to recover original return address and continue with the unwind.

Also updating the pt_regs data with cs/flags/rsp which are needed for successful stack retrieval from ebpf bpf_get_stackid helper.
- in get_perf_callchain we check user_mode(regs) so CS has to be set
- in perf_callchain_kernel we call perf_hw_regs(regs), so EFLAGS/FIXED has to be unset

Acked-by Masami Hiramatsu (Google) <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
 arch/x86/include/asm/ftrace.h |  5 +++++
 arch/x86/kernel/ftrace_64.S   |  8 +++++++-
 include/linux/ftrace.h        | 10 +++++++++-
 3 files changed, 21 insertions(+), 2 deletions(-)

arch/x86/include/asm/ftrace.h +0 -0

index 93156ac4ffe0..b08c95872eed 100644
@@ -56,6 +56,11 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
      return &arch_ftrace_regs(fregs)->regs;
 }
 

arch/x86/include/asm/ftrace.h arch_ftrace_get_regs(struct ftrace_regs *fregs) · line 62

Quoted 4 lines

arch/x86/include/asm/ftrace.h +4 -0

+#define arch_ftrace_partial_regs(regs) do {  \
+     regs->flags &= ~X86_EFLAGS_FIXED;       \
+     regs->cs = __KERNEL_CS;                 \
+} while (0)

I lost the plot. Why here and not in asm below?

Quoted 10 lines

tools/testing/selftests/bpf/prog_tests/stacktrace_ips.c +2 -0

index 6fca459ba..282a068d2 100644
@@ -84,12 +84,58 @@ static void test_stacktrace_ips_kprobe_multi(bool retprobe)
 	stacktrace_ips__destroy(skel);
 }
 
+static void test_stacktrace_ips_raw_tp(void)
+{

[ ... ]

tools/testing/selftests/bpf/prog_tests/stacktrace_ips.c static void test_stacktrace_ips_kprobe_multi(bool retprobe) · line 117

Quoted 7 lines

tools/testing/selftests/bpf/prog_tests/stacktrace_ips.c +7 -0

+	/* get bpf program address */
+	info.jited_ksyms = ptr_to_u64(&bpf_prog_ksym);
+	info.nr_jited_ksyms = 1;
+	err = bpf_prog_get_info_by_fd(bpf_program__fd(skel->progs.rawtp_test),
+				      &info, &info_len);
+	if (ASSERT_OK(err, "bpf_prog_get_info_by_fd"))
+		goto cleanup;

             ^^^^^^^^^^^^^^

Is the error check inverted in test_stacktrace_ips_raw_tp()?

The ASSERT_OK macro returns true when err == 0 (success), so the code jumps to cleanup on success and continues on failure. Comparing with test_stacktrace_ips_kprobe_multi() at line 70, the pattern should be:

if (!ASSERT_OK_PTR(...))

      goto cleanup;

With the current code, bpf_prog_get_info_by_fd() success skips the test (trigger_module_test_read and check_stacktrace_ips), while failure continues with invalid bpf_prog_ksym data.

tools/testing/selftests/bpf/prog_tests/stacktrace_ips.c static void test_stacktrace_ips_kprobe_multi(bool retprobe) · line 130

Quoted 13 lines

tools/testing/selftests/bpf/prog_tests/stacktrace_ips.c +13 -0

+
+	trigger_module_test_read(1);
+
+	load_kallsyms();
+
+	check_stacktrace_ips(bpf_map__fd(skel->maps.stackmap), skel->bss->stack_key, 2,
+			     bpf_prog_ksym,
+			     ksym_get_addr("bpf_trace_run2"));
+
+cleanup:
+	stacktrace_ips__destroy(skel);
+}
+

[ ... ]

---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19051288274