Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from
+3,441 −7

Conversation

@kernel-patches-daemon-bpf
Copy link

Pull request for series with
subject: x86/fgraph,bpf: Fix ORC stack unwind from return probe
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: e758657
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@kernel-patches-review-bot
Copy link 

commit 26b06def1956497a15eecf22787d5430fad2fed6
Author: Jiri Olsa <[email protected]>

Revert "perf/x86: Always store regs->ip in perf_callchain_kernel()"

This commit reverts a 2019 fix that ensured regs->ip is stored when
using non-HW regs (regs populated by perf_arch_fetch_caller_regs). The
revert claims the IP is captured during unwinding, but analysis suggests
otherwise.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 745caa6c1..fa6c47b50 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2789,13 +2789,13 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
>  		return;
>  	}
>
> -	if (perf_callchain_store(entry, regs->ip))
> -		return;
> -
> -	if (perf_hw_regs(regs))
> +	if (perf_hw_regs(regs)) {
> +		if (perf_callchain_store(entry, regs->ip))
> +			return;
>  		unwind_start(&state, current, regs, NULL);
> -	else
> +	} else {
>  		unwind_start(&state, current, NULL, (void *)regs->sp);
> +	}

When perf_hw_regs(regs) returns false (which happens when regs comes from
perf_arch_fetch_caller_regs since it sets regs->flags=0), the else branch
calls unwind_start with regs=NULL. Can this lose the regs->ip value?

Looking at the code flow:

perf_arch_fetch_caller_regs() sets:
  - regs->ip = CALLER_ADDR0 (the caller's instruction pointer)
  - regs->flags = 0 (no X86_EFLAGS_FIXED)

perf_hw_regs() checks:
  - returns (regs->flags & X86_EFLAGS_FIXED), which is false

The else branch executes:
  - unwind_start(&state, current, NULL, (void *)regs->sp)

In __unwind_start() at arch/x86/kernel/unwind_orc.c, when regs=NULL:
  - Takes the "else if (task == current)" path (line 711)
  - Initializes state->ip via inline asm from current location
  - This is __unwind_start's IP, not the original regs->ip
  - Then advances to first_frame at regs->sp (lines 755-758)

The original regs->ip value (captured by perf_arch_fetch_caller_regs) is
never stored in the callchain. This was the exact issue fixed by commit
83f44ae0f8af in 2019, which stated:

  "The stacktrace_map_raw_tp BPF selftest is failing because the RIP
   saved by perf_arch_fetch_caller_regs() isn't getting saved by
   perf_callchain_kernel()."

Does this revert re-introduce the BPF selftest failure that was fixed in
2019? The test tools/testing/selftests/bpf/prog_tests/stacktrace_map_raw_tp.c
still exists in the kernel tree.

>
>  	for (; !unwind_done(&state); unwind_next_frame(&state)) {
>  		addr = unwind_get_return_address(&state);

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

In-Reply-To-Subject: Revert "perf/x86: Always store regs->ip in perf_callchain_kernel()"
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18842579839

@kernel-patches-daemon-bpf
Copy link
Author

Forwarding comment 3451362555 via email
In-Reply-To: [email protected]
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: ff88079
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: f9db3a3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: f9db3a3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 8842732
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 23f852d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 54c134f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 9f317bd
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 54c134f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@olsajiri
Revert "perf/x86: Always store regs->ip in perf_callchain_kernel()"
36c02a4 
This reverts commit 83f44ae.

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.

Acked-by: Song Liu <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
@olsajiri
x86/fgraph,bpf: Fix stack ORC unwind from kprobe_multi return probe
a9af73f 
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

Note I feel like it'd be better to set those directly in return_to_handler,
but Steven suggested setting them later in kprobe_multi code path.

Signed-off-by: Jiri Olsa <[email protected]>
Acked-by: Masami Hiramatsu (Google) <[email protected]>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: e2e668b
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271
version: 1

@olsajiri
selftests/bpf: Add stacktrace ips test for kprobe_multi/kretprobe_multi
ec2dea1 
Adding test that attaches kprobe/kretprobe multi and verifies the
ORC stacktrace matches expected functions.

It skips the test for if kernels built with frame pointer unwinder.

Signed-off-by: Jiri Olsa <[email protected]>
@kernel-patches-daemon-bpf
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=1016271 expired. Closing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bpf-next changes-requested V1-ci-fail V1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@olsajiri