[PATCH] arm64: stacktrace: don't trace arch_stack_walk()

[PATCH] arm64: stacktrace: don't trace arch_stack_walk()

[Mark Rutland]

We recently converted arm64 to use arch_stack_walk() in commit:

  5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")

The core stacktrace code expects that (when tracing the current task)
arch_stack_walk() starts a trace at its caller, and does not include
itself in the trace. However, arm64's arch_stack_walk() includes itself,
and so traces include one more entry than callers expect. The core
stacktrace code which calls arch_stack_walk() tries to skip a number of
entries to prevent itself appearing in a trace, and the additional entry
prevents skipping one of the core stacktrace functions, leaving this in
the trace unexpectedly.

We can fix this by having arm64's arch_stack_walk() begin the trace with
its caller. The first value returned by the trace will be
__builtin_return_address(0), i.e. the caller of arch_stack_walk(). The
first frame record to be unwound will be __builtin_frame_address(1),
i.e. the caller's frame record. To prevent surprises, arch_stack_walk()
is also marked noinline.

While __builtin_frame_address(1) is not safe in portable code, local GCC
developers have confirmed that it is safe on arm64. To find the caller's
frame record, the builtin can safely dereference the current function's
frame record or (in theory) could stash the original FP into another GPR
at function entry time, neither of which are problematic.

Prior to this patch, the tracing code would unexpectedly show up in
traces of the current task, e.g.

| # cat /proc/self/stack
| [<0>] stack_trace_save_tsk+0x98/0x100
| [<0>] proc_pid_stack+0xb4/0x130
| [<0>] proc_single_show+0x60/0x110
| [<0>] seq_read_iter+0x230/0x4d0
| [<0>] seq_read+0xdc/0x130
| [<0>] vfs_read+0xac/0x1e0
| [<0>] ksys_read+0x6c/0xfc
| [<0>] __arm64_sys_read+0x20/0x30
| [<0>] el0_svc_common.constprop.0+0x60/0x120
| [<0>] do_el0_svc+0x24/0x90
| [<0>] el0_svc+0x2c/0x54
| [<0>] el0_sync_handler+0x1a4/0x1b0
| [<0>] el0_sync+0x170/0x180

After this patch, the tracing code will not show up in such traces:

| # cat /proc/self/stack
| [<0>] proc_pid_stack+0xb4/0x130
| [<0>] proc_single_show+0x60/0x110
| [<0>] seq_read_iter+0x230/0x4d0
| [<0>] seq_read+0xdc/0x130
| [<0>] vfs_read+0xac/0x1e0
| [<0>] ksys_read+0x6c/0xfc
| [<0>] __arm64_sys_read+0x20/0x30
| [<0>] el0_svc_common.constprop.0+0x60/0x120
| [<0>] do_el0_svc+0x24/0x90
| [<0>] el0_svc+0x2c/0x54
| [<0>] el0_sync_handler+0x1a4/0x1b0
| [<0>] el0_sync+0x170/0x180

Erring on the side of caution, I've given this a spin with a bunch of
toolchains, verifying the output of /proc/self/stack and checking that
the assembly looked sound. For GCC (where we require version 5.1.0 or
later) I tested with the kernel.org crosstool binares for versions
5.5.0, 6.4.0, 6.5.0, 7.3.0, 7.5.0, 8.1.0, 8.3.0, 8.4.0, 9.2.0, and
10.1.0. For clang (where we require version 10.0.1 or later) I tested
with the llvm.org binary releases of 11.0.0, and 11.0.1.

Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chen Jun <chenjun102@huawei.com>
Cc: Marco Elver <elver@google.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ad20981dfda4..d55bdfb7789c 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -194,8 +194,9 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl)
 
 #ifdef CONFIG_STACKTRACE
 
-void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
-		     struct task_struct *task, struct pt_regs *regs)
+noinline void arch_stack_walk(stack_trace_consume_fn consume_entry,
+			      void *cookie, struct task_struct *task,
+			      struct pt_regs *regs)
 {
 	struct stackframe frame;
 
@@ -203,8 +204,8 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 		start_backtrace(&frame, regs->regs[29], regs->pc);
 	else if (task == current)
 		start_backtrace(&frame,
-				(unsigned long)__builtin_frame_address(0),
-				(unsigned long)arch_stack_walk);
+				(unsigned long)__builtin_frame_address(1),
+				(unsigned long)__builtin_return_address(0));
 	else
 		start_backtrace(&frame, thread_saved_fp(task),
 				thread_saved_pc(task));
-- 
2.11.0

Re: [PATCH] arm64: stacktrace: don't trace arch_stack_walk()

[Catalin Marinas]

On Fri, Mar 19, 2021 at 06:41:06PM +0000, Mark Rutland wrote:
> We recently converted arm64 to use arch_stack_walk() in commit:
> 
>   5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> 
> The core stacktrace code expects that (when tracing the current task)
> arch_stack_walk() starts a trace at its caller, and does not include
> itself in the trace. However, arm64's arch_stack_walk() includes itself,
> and so traces include one more entry than callers expect. The core
> stacktrace code which calls arch_stack_walk() tries to skip a number of
> entries to prevent itself appearing in a trace, and the additional entry
> prevents skipping one of the core stacktrace functions, leaving this in
> the trace unexpectedly.
> 
> We can fix this by having arm64's arch_stack_walk() begin the trace with
> its caller. The first value returned by the trace will be
> __builtin_return_address(0), i.e. the caller of arch_stack_walk(). The
> first frame record to be unwound will be __builtin_frame_address(1),
> i.e. the caller's frame record. To prevent surprises, arch_stack_walk()
> is also marked noinline.
> 
> While __builtin_frame_address(1) is not safe in portable code, local GCC
> developers have confirmed that it is safe on arm64. To find the caller's
> frame record, the builtin can safely dereference the current function's
> frame record or (in theory) could stash the original FP into another GPR
> at function entry time, neither of which are problematic.
> 
> Prior to this patch, the tracing code would unexpectedly show up in
> traces of the current task, e.g.
> 
> | # cat /proc/self/stack
> | [<0>] stack_trace_save_tsk+0x98/0x100
> | [<0>] proc_pid_stack+0xb4/0x130
> | [<0>] proc_single_show+0x60/0x110
> | [<0>] seq_read_iter+0x230/0x4d0
> | [<0>] seq_read+0xdc/0x130
> | [<0>] vfs_read+0xac/0x1e0
> | [<0>] ksys_read+0x6c/0xfc
> | [<0>] __arm64_sys_read+0x20/0x30
> | [<0>] el0_svc_common.constprop.0+0x60/0x120
> | [<0>] do_el0_svc+0x24/0x90
> | [<0>] el0_svc+0x2c/0x54
> | [<0>] el0_sync_handler+0x1a4/0x1b0
> | [<0>] el0_sync+0x170/0x180
> 
> After this patch, the tracing code will not show up in such traces:
> 
> | # cat /proc/self/stack
> | [<0>] proc_pid_stack+0xb4/0x130
> | [<0>] proc_single_show+0x60/0x110
> | [<0>] seq_read_iter+0x230/0x4d0
> | [<0>] seq_read+0xdc/0x130
> | [<0>] vfs_read+0xac/0x1e0
> | [<0>] ksys_read+0x6c/0xfc
> | [<0>] __arm64_sys_read+0x20/0x30
> | [<0>] el0_svc_common.constprop.0+0x60/0x120
> | [<0>] do_el0_svc+0x24/0x90
> | [<0>] el0_svc+0x2c/0x54
> | [<0>] el0_sync_handler+0x1a4/0x1b0
> | [<0>] el0_sync+0x170/0x180
> 
> Erring on the side of caution, I've given this a spin with a bunch of
> toolchains, verifying the output of /proc/self/stack and checking that
> the assembly looked sound. For GCC (where we require version 5.1.0 or
> later) I tested with the kernel.org crosstool binares for versions
> 5.5.0, 6.4.0, 6.5.0, 7.3.0, 7.5.0, 8.1.0, 8.3.0, 8.4.0, 9.2.0, and
> 10.1.0. For clang (where we require version 10.0.1 or later) I tested
> with the llvm.org binary releases of 11.0.0, and 11.0.1.
> 
> Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Chen Jun <chenjun102@huawei.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>

Thanks Mark. I think we should add a cc stable, just with Fixes doesn't
always seem to end up in a stable kernel:

Cc: <stable@vger.kernel.org> # 5.10.x

With that:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH] arm64: stacktrace: don't trace arch_stack_walk()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Fri, Mar 19, 2021 at 06:41:06PM +0000, Mark Rutland wrote:
> We recently converted arm64 to use arch_stack_walk() in commit:
> 
>   5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> 
> The core stacktrace code expects that (when tracing the current task)
> arch_stack_walk() starts a trace at its caller, and does not include
> itself in the trace. However, arm64's arch_stack_walk() includes itself,

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] arm64: stacktrace: don't trace arch_stack_walk()

[Mark Rutland]

On Fri, Mar 19, 2021 at 07:02:06PM +0000, Catalin Marinas wrote:
> On Fri, Mar 19, 2021 at 06:41:06PM +0000, Mark Rutland wrote:
> > We recently converted arm64 to use arch_stack_walk() in commit:
> > 
> >   5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> > 
> > The core stacktrace code expects that (when tracing the current task)
> > arch_stack_walk() starts a trace at its caller, and does not include
> > itself in the trace. However, arm64's arch_stack_walk() includes itself,
> > and so traces include one more entry than callers expect. The core
> > stacktrace code which calls arch_stack_walk() tries to skip a number of
> > entries to prevent itself appearing in a trace, and the additional entry
> > prevents skipping one of the core stacktrace functions, leaving this in
> > the trace unexpectedly.
> > 
> > We can fix this by having arm64's arch_stack_walk() begin the trace with
> > its caller. The first value returned by the trace will be
> > __builtin_return_address(0), i.e. the caller of arch_stack_walk(). The
> > first frame record to be unwound will be __builtin_frame_address(1),
> > i.e. the caller's frame record. To prevent surprises, arch_stack_walk()
> > is also marked noinline.

[...]

> > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Chen Jun <chenjun102@huawei.com>
> > Cc: Marco Elver <elver@google.com>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Will Deacon <will@kernel.org>
> 
> Thanks Mark. I think we should add a cc stable, just with Fixes doesn't
> always seem to end up in a stable kernel:
> 
> Cc: <stable@vger.kernel.org> # 5.10.x

Makes sense to me, sure.

> With that:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks!

Will, I assume you're happy to fold in the above when picking this. If
you'd prefer I repost with that folded in, please let me know!

Mark.

Re: [PATCH] arm64: stacktrace: don't trace arch_stack_walk()

[Will Deacon]

On Fri, 19 Mar 2021 18:41:06 +0000, Mark Rutland wrote:
> We recently converted arm64 to use arch_stack_walk() in commit:
> 
>   5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> 
> The core stacktrace code expects that (when tracing the current task)
> arch_stack_walk() starts a trace at its caller, and does not include
> itself in the trace. However, arm64's arch_stack_walk() includes itself,
> and so traces include one more entry than callers expect. The core
> stacktrace code which calls arch_stack_walk() tries to skip a number of
> entries to prevent itself appearing in a trace, and the additional entry
> prevents skipping one of the core stacktrace functions, leaving this in
> the trace unexpectedly.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: stacktrace: don't trace arch_stack_walk()
      https://git.kernel.org/arm64/c/c607ab4f916d

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH] arm64: stacktrace: don't trace arch_stack_walk()

[Ard Biesheuvel]

On Mon, 22 Mar 2021 at 14:26, Will Deacon <will@kernel.org> wrote:
>
> On Fri, 19 Mar 2021 18:41:06 +0000, Mark Rutland wrote:
> > We recently converted arm64 to use arch_stack_walk() in commit:
> >
> >   5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> >
> > The core stacktrace code expects that (when tracing the current task)
> > arch_stack_walk() starts a trace at its caller, and does not include
> > itself in the trace. However, arm64's arch_stack_walk() includes itself,
> > and so traces include one more entry than callers expect. The core
> > stacktrace code which calls arch_stack_walk() tries to skip a number of
> > entries to prevent itself appearing in a trace, and the additional entry
> > prevents skipping one of the core stacktrace functions, leaving this in
> > the trace unexpectedly.
> >
> > [...]
>
> Applied to arm64 (for-next/fixes), thanks!
>
> [1/1] arm64: stacktrace: don't trace arch_stack_walk()
>       https://git.kernel.org/arm64/c/c607ab4f916d
>

Ehm, did anyone check if the following caveat regarding
__builtin_frame_address() applies on arm64? (from the GCC man page
[0])

"""
Calling this function with a nonzero argument can have unpredictable
effects, including crashing the calling program. As a result, calls
that are considered unsafe are diagnosed when the -Wframe-address
option is in effect. Such calls should only be made in debugging
situations.
"""

[0] https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html

Re: [PATCH] arm64: stacktrace: don't trace arch_stack_walk()

[Ard Biesheuvel]

On Mon, 22 Mar 2021 at 16:57, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 22 Mar 2021 at 14:26, Will Deacon <will@kernel.org> wrote:
> >
> > On Fri, 19 Mar 2021 18:41:06 +0000, Mark Rutland wrote:
> > > We recently converted arm64 to use arch_stack_walk() in commit:
> > >
> > >   5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> > >
> > > The core stacktrace code expects that (when tracing the current task)
> > > arch_stack_walk() starts a trace at its caller, and does not include
> > > itself in the trace. However, arm64's arch_stack_walk() includes itself,
> > > and so traces include one more entry than callers expect. The core
> > > stacktrace code which calls arch_stack_walk() tries to skip a number of
> > > entries to prevent itself appearing in a trace, and the additional entry
> > > prevents skipping one of the core stacktrace functions, leaving this in
> > > the trace unexpectedly.
> > >
> > > [...]
> >
> > Applied to arm64 (for-next/fixes), thanks!
> >
> > [1/1] arm64: stacktrace: don't trace arch_stack_walk()
> >       https://git.kernel.org/arm64/c/c607ab4f916d
> >
>
> Ehm, did anyone check if the following caveat regarding
> __builtin_frame_address() applies on arm64? (from the GCC man page
> [0])
>
> """
> Calling this function with a nonzero argument can have unpredictable
> effects, including crashing the calling program. As a result, calls
> that are considered unsafe are diagnosed when the -Wframe-address
> option is in effect. Such calls should only be made in debugging
> situations.
> """
>
> [0] https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html

Never mind, failed to read the entire thread. Apologies ...