[PATCH] arm64: stacktrace: Clarify documentation of EL0 entry frame skipping

[PATCH] arm64: stacktrace: Clarify documentation of EL0 entry frame skipping

[Mark Brown]

The unwinder suppresses reporting of frames created on entry from EL0 where
both FP and PC are NULL in order to avoid having the final NULL PC
cluttering traces and potentially creating confusion. There is a comment
explaining this which doesn't explicitly call out the case where we have
a valid PC but NULL FP, do so in order to ensure that things are as
clear as possible to readers unfamiliar with the code.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index fa56af1a59c3..0c474d81d4e9 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -107,8 +107,9 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	/*
 	 * Frames created upon entry from EL0 have NULL FP and PC values, so
 	 * don't bother reporting these. Frames created by __noreturn functions
-	 * might have a valid FP even if PC is bogus, so only terminate where
-	 * both are NULL.
+	 * might have a valid FP even if PC is bogus and the last frame in a
+	 * normal stack will have a valid PC but NULL FP, so only terminate
+	 * early where both are NULL.
 	 */
 	if (!frame->fp && !frame->pc)
 		return -EINVAL;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: stacktrace: Clarify documentation of EL0 entry frame skipping

[Mark Rutland]

Hi Mark,

On Wed, Oct 07, 2020 at 06:11:38PM +0100, Mark Brown wrote:
> The unwinder suppresses reporting of frames created on entry from EL0 where
> both FP and PC are NULL in order to avoid having the final NULL PC
> cluttering traces and potentially creating confusion. There is a comment
> explaining this which doesn't explicitly call out the case where we have
> a valid PC but NULL FP, do so in order to ensure that things are as
> clear as possible to readers unfamiliar with the code.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index fa56af1a59c3..0c474d81d4e9 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -107,8 +107,9 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  	/*
>  	 * Frames created upon entry from EL0 have NULL FP and PC values, so
>  	 * don't bother reporting these. Frames created by __noreturn functions
> -	 * might have a valid FP even if PC is bogus, so only terminate where
> -	 * both are NULL.
> +	 * might have a valid FP even if PC is bogus and the last frame in a
> +	 * normal stack will have a valid PC but NULL FP, so only terminate
> +	 * early where both are NULL.
>  	 */

I reckon we should remove the special case entirely rather than trying
to shore up the comment here -- the related comment in entry.S is stale
and misleading, and this will be a mystery to unfamiliar readers anyhow.

Does the below patch look ok to you? I suspect the explicit check for the
terminal case may help with the reliable stacktrace stuff? e.g. we could
make that return -ENOENT to distinguish a terminal case from a broken
frame record.

Thanks,
Mark.

---->8----
From 61cb79bd9d926744190a43a5e1ec9aefda7ba052 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Thu, 8 Oct 2020 10:47:59 +0100
Subject: [PATCH] arm64: remove EL0 exception frame record

When entering an exception from EL0, the entry code creates a synthetic
frame record with a NULL PC. This was used by the code introduced in
commit:

  7326749801396105 ("arm64: unwind: reference pt_regs via embedded stack frame")

... to discover exception entries on the stack and dump the associated
pt_regs. Since the NULL PC was undesirable for the stacktrace, we added
a special case to unwind_frame() to prevent the NULL PC from being
logged.

Since commit:

  a25ffd3a6302a678 ("arm64: traps: Don't print stack or raw PC/LR values in backtraces")

... we no longer try to dump the pt_regs as part of a stacktrace, and
hence no longer need the synthetic exception record.

This patch removes the synthetic exception record and the associated
special case in unwind_frame(). Instead, EL0 exceptions set the FP to
NULL, as is the case for other terminal records (e.g. when a kernel
thread starts). The synthetic record for exceptions from EL1 is
retrained as this has useful unwind information for the interrupted
context.

To make the terminal case a bit clearer, an explicit check is added to
the start of unwind_frame(). This would otherwise be caught implicitly
by the on_accessible_stack() checks.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Mark Brown <broonie@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/entry.S      | 10 +++++-----
 arch/arm64/kernel/stacktrace.c | 13 ++++---------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index f30007dff35f7..8af811b6b2259 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -228,16 +228,16 @@ alternative_else_nop_endif
 	stp	lr, x21, [sp, #S_LR]
 
 	/*
-	 * In order to be able to dump the contents of struct pt_regs at the
-	 * time the exception was taken (in case we attempt to walk the call
-	 * stack later), chain it together with the stack frames.
+	 * For exceptions from EL0, terminate the callchain here.
+	 * For exceptions from EL1, create a synthetic frame record so the
+	 * interrupted code shows up in the backtrace.
 	 */
 	.if \el == 0
-	stp	xzr, xzr, [sp, #S_STACKFRAME]
+	mov	x29, xzr
 	.else
 	stp	x29, x22, [sp, #S_STACKFRAME]
-	.endif
 	add	x29, sp, #S_STACKFRAME
+	.endif
 
 #ifdef CONFIG_ARM64_SW_TTBR0_PAN
 alternative_if_not ARM64_HAS_PAN
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index fa56af1a59c39..0fb42129b4691 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -44,6 +44,10 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	unsigned long fp = frame->fp;
 	struct stack_info info;
 
+	/* Terminal record; nothing to unwind */
+	if (!fp)
+		return -EINVAL;
+
 	if (fp & 0xf)
 		return -EINVAL;
 
@@ -104,15 +108,6 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 
 	frame->pc = ptrauth_strip_insn_pac(frame->pc);
 
-	/*
-	 * Frames created upon entry from EL0 have NULL FP and PC values, so
-	 * don't bother reporting these. Frames created by __noreturn functions
-	 * might have a valid FP even if PC is bogus, so only terminate where
-	 * both are NULL.
-	 */
-	if (!frame->fp && !frame->pc)
-		return -EINVAL;
-
 	return 0;
 }
 NOKPROBE_SYMBOL(unwind_frame);
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: stacktrace: Clarify documentation of EL0 entry frame skipping

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 958 bytes --]

On Thu, Oct 08, 2020 at 11:15:58AM +0100, Mark Rutland wrote:

> I reckon we should remove the special case entirely rather than trying
> to shore up the comment here -- the related comment in entry.S is stale
> and misleading, and this will be a mystery to unfamiliar readers anyhow.

That works too, so long as the check isn't really doing anything it can
just be deleted and the comment with it - it's certainly one less weird
special case.  So long as all the things relying on it are already gone.

> Does the below patch look ok to you? I suspect the explicit check for the
> terminal case may help with the reliable stacktrace stuff? e.g. we could
> make that return -ENOENT to distinguish a terminal case from a broken
> frame record.

That's almost exactly what I've got in my reliable stacktrace queue,
down to the choice of -ENOENT for reporting the end of an intact stack.
I'll pick this up for my changes if it doesn't get picked up separately.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel