From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Naveen N Rao <naveen@kernel.org>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH 17/17] powerpc/ftrace: Create a dummy stackframe to fix stack unwind
Date: Fri, 23 Jun 2023 05:40:34 +0000 [thread overview]
Message-ID: <1a54912a-7f84-8555-4fd5-6eb970b7e415@csgroup.eu> (raw)
In-Reply-To: <b5ac4a99883624a74fdc2a5d141fddb2e53aa06d.1687166935.git.naveen@kernel.org>
Le 19/06/2023 à 11:47, Naveen N Rao a écrit :
> With ppc64 -mprofile-kernel and ppc32 -pg, profiling instructions to
> call into ftrace are emitted right at function entry. The instruction
> sequence used is minimal to reduce overhead. Crucially, a stackframe is
> not created for the function being traced. This breaks stack unwinding
> since the function being traced does not have a stackframe for itself.
> As such, it never shows up in the backtrace:
>
> /sys/kernel/debug/tracing # echo 1 > /proc/sys/kernel/stack_tracer_enabled
> /sys/kernel/debug/tracing # cat stack_trace
> Depth Size Location (17 entries)
> ----- ---- --------
> 0) 4144 32 ftrace_call+0x4/0x44
> 1) 4112 432 get_page_from_freelist+0x26c/0x1ad0
> 2) 3680 496 __alloc_pages+0x290/0x1280
> 3) 3184 336 __folio_alloc+0x34/0x90
> 4) 2848 176 vma_alloc_folio+0xd8/0x540
> 5) 2672 272 __handle_mm_fault+0x700/0x1cc0
> 6) 2400 208 handle_mm_fault+0xf0/0x3f0
> 7) 2192 80 ___do_page_fault+0x3e4/0xbe0
> 8) 2112 160 do_page_fault+0x30/0xc0
> 9) 1952 256 data_access_common_virt+0x210/0x220
> 10) 1696 400 0xc00000000f16b100
> 11) 1296 384 load_elf_binary+0x804/0x1b80
> 12) 912 208 bprm_execve+0x2d8/0x7e0
> 13) 704 64 do_execveat_common+0x1d0/0x2f0
> 14) 640 160 sys_execve+0x54/0x70
> 15) 480 64 system_call_exception+0x138/0x350
> 16) 416 416 system_call_common+0x160/0x2c4
>
> Fix this by having ftrace create a dummy stackframe for the function
> being traced. Since this is only relevant when ftrace is active, we nop
> out the instruction to store LR in the LR save area in the profiling
> instruction sequence on ppc32 (and in ppc64 with older gcc versions).
> Instead, in ftrace, we store LR in the LR save area of the previous
> stackframe, and create a minimal stackframe to represent the function
> being traced. With this, backtraces now capture the function being
> traced:
>
> /sys/kernel/debug/tracing # cat stack_trace
> Depth Size Location (17 entries)
> ----- ---- --------
> 0) 3888 32 _raw_spin_trylock+0x8/0x70
> 1) 3856 576 get_page_from_freelist+0x26c/0x1ad0
> 2) 3280 64 __alloc_pages+0x290/0x1280
> 3) 3216 336 __folio_alloc+0x34/0x90
> 4) 2880 176 vma_alloc_folio+0xd8/0x540
> 5) 2704 416 __handle_mm_fault+0x700/0x1cc0
> 6) 2288 96 handle_mm_fault+0xf0/0x3f0
> 7) 2192 48 ___do_page_fault+0x3e4/0xbe0
> 8) 2144 192 do_page_fault+0x30/0xc0
> 9) 1952 608 data_access_common_virt+0x210/0x220
> 10) 1344 16 0xc0000000334bbb50
> 11) 1328 416 load_elf_binary+0x804/0x1b80
> 12) 912 64 bprm_execve+0x2d8/0x7e0
> 13) 848 176 do_execveat_common+0x1d0/0x2f0
> 14) 672 192 sys_execve+0x54/0x70
> 15) 480 64 system_call_exception+0x138/0x350
> 16) 416 416 system_call_common+0x160/0x2c4
>
> This results in two additional stores in the ftrace entry code, but
> produces reliable backtraces. Note that this change now aligns with
> other architectures (arm64, s390, x86).
>
> Signed-off-by: Naveen N Rao <naveen@kernel.org>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/trace/ftrace.c | 6 ++++--
> arch/powerpc/kernel/trace/ftrace_entry.S | 11 ++++++++---
> 2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index 82010629cf887c..2956196c98ffdc 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -229,13 +229,15 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> /* Expected sequence: 'mflr r0', 'stw r0,4(r1)', 'bl _mcount' */
> ret = ftrace_validate_inst(ip - 8, ppc_inst(PPC_RAW_MFLR(_R0)));
> if (!ret)
> - ret = ftrace_validate_inst(ip - 4, ppc_inst(PPC_RAW_STW(_R0, _R1, 4)));
> + ret = ftrace_modify_code(ip - 4, ppc_inst(PPC_RAW_STW(_R0, _R1, 4)),
> + ppc_inst(PPC_RAW_NOP()));
> } else if (IS_ENABLED(CONFIG_MPROFILE_KERNEL)) {
> /* Expected sequence: 'mflr r0', ['std r0,16(r1)'], 'bl _mcount' */
> ret = ftrace_read_inst(ip - 4, &old);
> if (!ret && !ppc_inst_equal(old, ppc_inst(PPC_RAW_MFLR(_R0)))) {
> ret = ftrace_validate_inst(ip - 8, ppc_inst(PPC_RAW_MFLR(_R0)));
> - ret |= ftrace_validate_inst(ip - 4, ppc_inst(PPC_RAW_STD(_R0, _R1, 16)));
> + ret |= ftrace_modify_code(ip - 4, ppc_inst(PPC_RAW_STD(_R0, _R1, 16)),
> + ppc_inst(PPC_RAW_NOP()));
> }
> } else {
> return -EINVAL;
> diff --git a/arch/powerpc/kernel/trace/ftrace_entry.S b/arch/powerpc/kernel/trace/ftrace_entry.S
> index bab3ab1368a33f..05e981fb526c2e 100644
> --- a/arch/powerpc/kernel/trace/ftrace_entry.S
> +++ b/arch/powerpc/kernel/trace/ftrace_entry.S
> @@ -33,6 +33,11 @@
> * and then arrange for the ftrace function to be called.
> */
> .macro ftrace_regs_entry allregs
> + /* Create a minimal stack frame for representing B */
> + PPC_STLU r1, -STACK_FRAME_MIN_SIZE(r1)
> + /* Save the original return address in A's stack frame */
> + PPC_STL r0, LRSAVE+STACK_FRAME_MIN_SIZE(r1)
> +
> /* Create our stack frame + pt_regs */
> PPC_STLU r1,-SWITCH_FRAME_SIZE(r1)
>
> @@ -41,8 +46,6 @@
> SAVE_GPRS(3, 10, r1)
>
> #ifdef CONFIG_PPC64
> - /* Save the original return address in A's stack frame */
> - std r0, LRSAVE+SWITCH_FRAME_SIZE(r1)
> /* Ok to continue? */
> lbz r3, PACA_FTRACE_ENABLED(r13)
> cmpdi r3, 0
> @@ -77,6 +80,8 @@
> mflr r7
> /* Save it as pt_regs->nip */
> PPC_STL r7, _NIP(r1)
> + /* Also save it in B's stackframe header for proper unwind */
> + PPC_STL r7, LRSAVE+SWITCH_FRAME_SIZE(r1)
> /* Save the read LR in pt_regs->link */
> PPC_STL r0, _LINK(r1)
>
> @@ -142,7 +147,7 @@
> #endif
>
> /* Pop our stack frame */
> - addi r1, r1, SWITCH_FRAME_SIZE
> + addi r1, r1, SWITCH_FRAME_SIZE+STACK_FRAME_MIN_SIZE
>
> #ifdef CONFIG_LIVEPATCH_64
> /* Based on the cmpd above, if the NIP was altered handle livepatch */
next prev parent reply other threads:[~2023-06-23 5:41 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-19 9:47 [PATCH 00/17] powerpc/ftrace: refactor and add support for -fpatchable-function-entry Naveen N Rao
2023-06-19 9:47 ` [PATCH 01/17] powerpc/ftrace: Fix dropping weak symbols with older toolchains Naveen N Rao
2023-06-23 5:10 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 02/17] powerpc/module: Remove unused .ftrace.tramp section Naveen N Rao
2023-06-23 5:12 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 03/17] powerpc64/ftrace: Move ELFv1 and -pg support code into a separate file Naveen N Rao
2023-06-23 5:13 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 04/17] powerpc/ftrace: Simplify function_graph support in ftrace.c Naveen N Rao
2023-06-23 5:14 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 05/17] powerpc/ftrace: Use FTRACE_REGS_ADDR to identify the correct ftrace trampoline Naveen N Rao
2023-06-23 5:15 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 06/17] powerpc/ftrace: Extend ftrace support for large kernels to ppc32 Naveen N Rao
2023-06-23 5:21 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 07/17] powerpc/ftrace: Consolidate ftrace support into fewer files Naveen N Rao
2023-06-23 5:25 ` Christophe Leroy
2023-06-28 7:32 ` Naveen N Rao
2023-06-19 9:47 ` [PATCH 08/17] powerpc/ftrace: Refactor ftrace_modify_code() Naveen N Rao
2023-06-23 5:27 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 09/17] powerpc/ftrace: Stop re-purposing linker generated long branches for ftrace Naveen N Rao
2023-06-23 5:28 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 10/17] powerpc/ftrace: Add separate ftrace_init_nop() with additional validation Naveen N Rao
2023-06-23 5:29 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 11/17] powerpc/ftrace: Simplify ftrace_make_nop() Naveen N Rao
2023-06-23 5:30 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 12/17] powerpc/ftrace: Simplify ftrace_make_call() Naveen N Rao
2023-06-23 5:30 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 13/17] powerpc/ftrace: Simplify ftrace_modify_call() Naveen N Rao
2023-06-23 5:31 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 14/17] powerpc/ftrace: Replace use of ftrace_call_replace() with ftrace_create_branch_inst() Naveen N Rao
2023-06-23 5:32 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 15/17] powerpc/ftrace: Implement ftrace_replace_code() Naveen N Rao
2023-06-23 5:32 ` Christophe Leroy
2023-06-19 9:47 ` [PATCH 16/17] powerpc/ftrace: Add support for -fpatchable-function-entry Naveen N Rao
2023-06-23 5:37 ` Christophe Leroy
2023-06-28 7:40 ` Naveen N Rao
2023-06-19 9:47 ` [PATCH 17/17] powerpc/ftrace: Create a dummy stackframe to fix stack unwind Naveen N Rao
2023-06-23 5:40 ` Christophe Leroy [this message]
2023-06-28 7:43 ` Naveen N Rao
2023-08-23 11:55 ` [PATCH 00/17] powerpc/ftrace: refactor and add support for -fpatchable-function-entry Michael Ellerman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1a54912a-7f84-8555-4fd5-6eb970b7e415@csgroup.eu \
--to=christophe.leroy@csgroup.eu \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=naveen@kernel.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.