linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] arm64: stacktrace: cleanups and improvements
@ 2022-08-01 12:12 Mark Rutland
  2022-08-01 12:12 ` [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common() Mark Rutland
                   ` (7 more replies)
  0 siblings, 8 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel, Marc Zyngier
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, tabba, will

Hi Marc,

This series is a set of improvements for the recently merged stacktrace
rework queued in the kvmarm next branch, along the lines of my prior
suggestions. I'm hoping that it would be possible to queue this for
-rc1.

Largely, the series decouples portions of the unwind code, making some
structural changes that remove the need for coupling (e.g. removing the
need for the stack type enumeration, and factoring out callbacks). This
should make it easier to make some further changes I have spoken about
previously (e.g. adding some metadata to the stack dump output), and I
have some patches building atop this which I intend to send out once
this is merged.

The only (intended) functional change from this series is improved
bounary detection, where overlapping frame records will now be caught
and rejected.

I've tested this natively (with the ftrace tests, LKDTM, and
/proc/self/stack), which seems happy.

I've also tested the NVHE and PKVM unwinders by hacking a BUG() into the
hyp code shortly after the KVM hyp code is initialized, and in both
cases the output is unchanged.

Thanks,
Mark.

Mark Rutland (8):
  arm64: stacktrace: simplify unwind_next_common()
  arm64: stacktrace: rename unwind_next_common() ->
    unwind_next_frame_record()
  arm64: stacktrace: move SDEI stack helpers to stacktrace code
  arm64: stacktrace: add stackinfo_on_stack() helper
  arm64: stacktrace: rework stack boundary discovery
  arm64: stacktrace: remove stack type from fp translator
  arm64: stacktrace: track all stack boundaries explicitly
  arm64: stacktrace: track hyp stacks in unwinder's address space

 arch/arm64/include/asm/processor.h         |   2 +-
 arch/arm64/include/asm/sdei.h              |  17 --
 arch/arm64/include/asm/stacktrace.h        |  71 +++++--
 arch/arm64/include/asm/stacktrace/common.h | 211 +++++++++------------
 arch/arm64/kernel/ptrace.c                 |   2 +-
 arch/arm64/kernel/sdei.c                   |  35 ----
 arch/arm64/kernel/stacktrace.c             |  66 ++++---
 arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  40 ++--
 arch/arm64/kvm/stacktrace.c                | 135 +++++++------
 9 files changed, 288 insertions(+), 291 deletions(-)

-- 
2.30.2


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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common()
  2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
@ 2022-08-01 12:12 ` Mark Rutland
  2022-08-01 12:46   ` Mark Brown
  2022-08-02  4:34   ` Kalesh Singh
  2022-08-01 12:12 ` [PATCH 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record() Mark Rutland
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, maz, tabba, will

Currently unwind_next_common() takes a pointer to a stack_info which is
only ever used within unwind_next_common().

Make it a local variable and simplify callers.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/stacktrace/common.h | 12 ++++++------
 arch/arm64/kernel/stacktrace.c             |  3 +--
 arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  4 +---
 arch/arm64/kvm/stacktrace.c                |  4 +---
 4 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index f58eb944c46fb..01dc9f44a24a7 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -140,27 +140,27 @@ typedef bool (*on_accessible_stack_fn)(const struct task_struct *tsk,
 				       struct stack_info *info);
 
 static inline int unwind_next_common(struct unwind_state *state,
-				     struct stack_info *info,
 				     on_accessible_stack_fn accessible,
 				     stack_trace_translate_fp_fn translate_fp)
 {
+	struct stack_info info;
 	unsigned long fp = state->fp, kern_fp = fp;
 	struct task_struct *tsk = state->task;
 
 	if (fp & 0x7)
 		return -EINVAL;
 
-	if (!accessible(tsk, fp, 16, info))
+	if (!accessible(tsk, fp, 16, &info))
 		return -EINVAL;
 
-	if (test_bit(info->type, state->stacks_done))
+	if (test_bit(info.type, state->stacks_done))
 		return -EINVAL;
 
 	/*
 	 * If fp is not from the current address space perform the necessary
 	 * translation before dereferencing it to get the next fp.
 	 */
-	if (translate_fp && !translate_fp(&kern_fp, info->type))
+	if (translate_fp && !translate_fp(&kern_fp, info.type))
 		return -EINVAL;
 
 	/*
@@ -177,7 +177,7 @@ static inline int unwind_next_common(struct unwind_state *state,
 	 * stack to another, it's never valid to unwind back to that first
 	 * stack.
 	 */
-	if (info->type == state->prev_type) {
+	if (info.type == state->prev_type) {
 		if (fp <= state->prev_fp)
 			return -EINVAL;
 	} else {
@@ -191,7 +191,7 @@ static inline int unwind_next_common(struct unwind_state *state,
 	state->fp = READ_ONCE(*(unsigned long *)(kern_fp));
 	state->pc = READ_ONCE(*(unsigned long *)(kern_fp + 8));
 	state->prev_fp = fp;
-	state->prev_type = info->type;
+	state->prev_type = info.type;
 
 	return 0;
 }
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ce190ee18a201..056fb045d0e0c 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -103,14 +103,13 @@ static int notrace unwind_next(struct unwind_state *state)
 {
 	struct task_struct *tsk = state->task;
 	unsigned long fp = state->fp;
-	struct stack_info info;
 	int err;
 
 	/* Final frame; nothing to unwind */
 	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
 		return -ENOENT;
 
-	err = unwind_next_common(state, &info, on_accessible_stack, NULL);
+	err = unwind_next_common(state, on_accessible_stack, NULL);
 	if (err)
 		return err;
 
diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
index 58f645ad66bcb..50a1fa6b5c044 100644
--- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
+++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
@@ -71,9 +71,7 @@ static bool on_accessible_stack(const struct task_struct *tsk,
 
 static int unwind_next(struct unwind_state *state)
 {
-	struct stack_info info;
-
-	return unwind_next_common(state, &info, on_accessible_stack, NULL);
+	return unwind_next_common(state, on_accessible_stack, NULL);
 }
 
 static void notrace unwind(struct unwind_state *state,
diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
index 949d19d603fba..b9cf551d9d31d 100644
--- a/arch/arm64/kvm/stacktrace.c
+++ b/arch/arm64/kvm/stacktrace.c
@@ -97,9 +97,7 @@ static bool on_accessible_stack(const struct task_struct *tsk,
 
 static int unwind_next(struct unwind_state *state)
 {
-	struct stack_info info;
-
-	return unwind_next_common(state, &info, on_accessible_stack,
+	return unwind_next_common(state, on_accessible_stack,
 				  kvm_nvhe_stack_kern_va);
 }
 
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record()
  2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
  2022-08-01 12:12 ` [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common() Mark Rutland
@ 2022-08-01 12:12 ` Mark Rutland
  2022-08-01 12:52   ` Mark Brown
  2022-08-02  4:38   ` Kalesh Singh
  2022-08-01 12:12 ` [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code Mark Rutland
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, maz, tabba, will

The unwind_next_common() function unwinds a single frame record. There
are other unwind steps (e.g. unwinding through trampolines) which are
handled in the regular kernel unwinder, and in future there may be other
common unwind helpers.

Clarify the purpose of unwind_next_common() by renaming it to
unwind_next_frame_record(). At the same time, add commentary, and delete
the redundant comment at the top of asm/stacktrace/common.h.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/stacktrace/common.h | 22 ++++++++++++----------
 arch/arm64/kernel/stacktrace.c             |  2 +-
 arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  2 +-
 arch/arm64/kvm/stacktrace.c                |  4 ++--
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index 01dc9f44a24a7..676002d7d333c 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -2,13 +2,6 @@
 /*
  * Common arm64 stack unwinder code.
  *
- * To implement a new arm64 stack unwinder:
- *     1) Include this header
- *
- *     2) Call into unwind_next_common() from your top level unwind
- *        function, passing it the validation and translation callbacks
- *        (though the later can be NULL if no translation is required).
- *
  * See: arch/arm64/kernel/stacktrace.c for the reference implementation.
  *
  * Copyright (C) 2012 ARM Ltd.
@@ -139,9 +132,18 @@ typedef bool (*on_accessible_stack_fn)(const struct task_struct *tsk,
 				       unsigned long sp, unsigned long size,
 				       struct stack_info *info);
 
-static inline int unwind_next_common(struct unwind_state *state,
-				     on_accessible_stack_fn accessible,
-				     stack_trace_translate_fp_fn translate_fp)
+/*
+ * unwind_next_frame_record() - Unwind to the next frame record indicated by
+ * @state->fp.
+ *
+ * @state:        the current unwind state.
+ * @accessible:   determines whether the frame record is accessible
+ * @translate_fp: translates the fp prior to access (may be NULL)
+ */
+static inline int
+unwind_next_frame_record(struct unwind_state *state,
+			 on_accessible_stack_fn accessible,
+			 stack_trace_translate_fp_fn translate_fp)
 {
 	struct stack_info info;
 	unsigned long fp = state->fp, kern_fp = fp;
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 056fb045d0e0c..4c8865e495fea 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -109,7 +109,7 @@ static int notrace unwind_next(struct unwind_state *state)
 	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
 		return -ENOENT;
 
-	err = unwind_next_common(state, on_accessible_stack, NULL);
+	err = unwind_next_frame_record(state, on_accessible_stack, NULL);
 	if (err)
 		return err;
 
diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
index 50a1fa6b5c044..579b46aa9a553 100644
--- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
+++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
@@ -71,7 +71,7 @@ static bool on_accessible_stack(const struct task_struct *tsk,
 
 static int unwind_next(struct unwind_state *state)
 {
-	return unwind_next_common(state, on_accessible_stack, NULL);
+	return unwind_next_frame_record(state, on_accessible_stack, NULL);
 }
 
 static void notrace unwind(struct unwind_state *state,
diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
index b9cf551d9d31d..b69c18a26567d 100644
--- a/arch/arm64/kvm/stacktrace.c
+++ b/arch/arm64/kvm/stacktrace.c
@@ -97,8 +97,8 @@ static bool on_accessible_stack(const struct task_struct *tsk,
 
 static int unwind_next(struct unwind_state *state)
 {
-	return unwind_next_common(state, on_accessible_stack,
-				  kvm_nvhe_stack_kern_va);
+	return unwind_next_frame_record(state, on_accessible_stack,
+					kvm_nvhe_stack_kern_va);
 }
 
 static void unwind(struct unwind_state *state,
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code
  2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
  2022-08-01 12:12 ` [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common() Mark Rutland
  2022-08-01 12:12 ` [PATCH 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record() Mark Rutland
@ 2022-08-01 12:12 ` Mark Rutland
  2022-08-01 12:48   ` Mark Brown
                     ` (2 more replies)
  2022-08-01 12:12 ` [PATCH 4/8] arm64: stacktrace: add stackinfo_on_stack() helper Mark Rutland
                   ` (4 subsequent siblings)
  7 siblings, 3 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, maz, tabba, will

For clarity and ease of maintenance, it would be helpful for all the
stack helpers to be in the same place.

Move the SDEI stack helpers into the stacktrace code where all the other
stack helpers live.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/sdei.h       | 17 ------------
 arch/arm64/include/asm/stacktrace.h | 40 ++++++++++++++++++++++++++++-
 arch/arm64/kernel/sdei.c            | 35 -------------------------
 arch/arm64/kernel/stacktrace.c      | 13 ++++++++--
 4 files changed, 50 insertions(+), 55 deletions(-)

diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
index 7bea1d705dd64..4292d9bafb9d2 100644
--- a/arch/arm64/include/asm/sdei.h
+++ b/arch/arm64/include/asm/sdei.h
@@ -43,22 +43,5 @@ unsigned long do_sdei_event(struct pt_regs *regs,
 unsigned long sdei_arch_get_entry_point(int conduit);
 #define sdei_arch_get_entry_point(x)	sdei_arch_get_entry_point(x)
 
-struct stack_info;
-
-bool _on_sdei_stack(unsigned long sp, unsigned long size,
-		    struct stack_info *info);
-static inline bool on_sdei_stack(unsigned long sp, unsigned long size,
-				struct stack_info *info)
-{
-	if (!IS_ENABLED(CONFIG_VMAP_STACK))
-		return false;
-	if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
-		return false;
-	if (in_nmi())
-		return _on_sdei_stack(sp, size, info);
-
-	return false;
-}
-
 #endif /* __ASSEMBLY__ */
 #endif	/* __ASM_SDEI_H */
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 6ebdcdff77f56..fa2df1ea22ebc 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -54,7 +54,45 @@ static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
 }
 #else
 static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
-			struct stack_info *info) { return false; }
+				     struct stack_info *info)
+{
+	return false;
+}
+#endif
+
+#if defined(CONFIG_ARM_SDE_INTERFACE) && defined(CONFIG_VMAP_STACK)
+DECLARE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
+DECLARE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
+
+static inline bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
+					struct stack_info *info)
+{
+	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
+	unsigned long high = low + SDEI_STACK_SIZE;
+
+	return on_stack(sp, size, low, high, STACK_TYPE_SDEI_NORMAL, info);
+}
+
+static inline bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
+					  struct stack_info *info)
+{
+	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
+	unsigned long high = low + SDEI_STACK_SIZE;
+
+	return on_stack(sp, size, low, high, STACK_TYPE_SDEI_CRITICAL, info);
+}
+#else
+static inline bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
+					struct stack_info *info)
+{
+	return false;
+}
+
+static inline bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
+					  struct stack_info *info)
+{
+	return false;
+}
 #endif
 
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
index d20620a1c51a4..881eece3422ca 100644
--- a/arch/arm64/kernel/sdei.c
+++ b/arch/arm64/kernel/sdei.c
@@ -31,9 +31,6 @@ unsigned long sdei_exit_mode;
  * sdei stack.
  * For now, we allocate stacks when the driver is probed.
  */
-DECLARE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
-DECLARE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
-
 #ifdef CONFIG_VMAP_STACK
 DEFINE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
 DEFINE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
@@ -162,38 +159,6 @@ static int init_sdei_scs(void)
 	return err;
 }
 
-static bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
-				 struct stack_info *info)
-{
-	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
-	unsigned long high = low + SDEI_STACK_SIZE;
-
-	return on_stack(sp, size, low, high, STACK_TYPE_SDEI_NORMAL, info);
-}
-
-static bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
-				   struct stack_info *info)
-{
-	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
-	unsigned long high = low + SDEI_STACK_SIZE;
-
-	return on_stack(sp, size, low, high, STACK_TYPE_SDEI_CRITICAL, info);
-}
-
-bool _on_sdei_stack(unsigned long sp, unsigned long size, struct stack_info *info)
-{
-	if (!IS_ENABLED(CONFIG_VMAP_STACK))
-		return false;
-
-	if (on_sdei_critical_stack(sp, size, info))
-		return true;
-
-	if (on_sdei_normal_stack(sp, size, info))
-		return true;
-
-	return false;
-}
-
 unsigned long sdei_arch_get_entry_point(int conduit)
 {
 	/*
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 4c8865e495fea..04a9b56b114c1 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -86,8 +86,17 @@ static bool on_accessible_stack(const struct task_struct *tsk,
 		return true;
 	if (on_overflow_stack(sp, size, info))
 		return true;
-	if (on_sdei_stack(sp, size, info))
-		return true;
+
+	if (IS_ENABLED(CONFIG_VMAP_STACK) &&
+	    IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) &&
+	    in_nmi())
+	{
+		if (on_sdei_critical_stack(sp, size, info))
+			return true;
+
+		if (on_sdei_normal_stack(sp, size, info))
+			return true;
+	}
 
 	return false;
 }
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 4/8] arm64: stacktrace: add stackinfo_on_stack() helper
  2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
                   ` (2 preceding siblings ...)
  2022-08-01 12:12 ` [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code Mark Rutland
@ 2022-08-01 12:12 ` Mark Rutland
  2022-08-01 15:56   ` Mark Brown
  2022-08-02  5:00   ` Kalesh Singh
  2022-08-01 12:12 ` [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery Mark Rutland
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, maz, tabba, will

Factor the core predicate out of on_stack() into a helper which can be
used on a pre-populated stack_info.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/stacktrace/common.h | 29 ++++++++++++++++------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index 676002d7d333c..9ed7feb493a36 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -66,21 +66,34 @@ struct unwind_state {
 	struct task_struct *task;
 };
 
+static inline bool stackinfo_on_stack(const struct stack_info *info,
+				      unsigned long sp, unsigned long size)
+{
+	if (!info->low)
+		return false;
+
+	if (sp < info->low || sp + size < sp || sp + size > info->high)
+		return false;
+
+	return true;
+}
+
 static inline bool on_stack(unsigned long sp, unsigned long size,
 			    unsigned long low, unsigned long high,
 			    enum stack_type type, struct stack_info *info)
 {
-	if (!low)
-		return false;
+	struct stack_info tmp = {
+		.low = low,
+		.high = high,
+		.type = type,
+	};
 
-	if (sp < low || sp + size < sp || sp + size > high)
+	if (!stackinfo_on_stack(&tmp, sp, size))
 		return false;
 
-	if (info) {
-		info->low = low;
-		info->high = high;
-		info->type = type;
-	}
+	if (info)
+		*info = tmp;
+
 	return true;
 }
 
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery
  2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
                   ` (3 preceding siblings ...)
  2022-08-01 12:12 ` [PATCH 4/8] arm64: stacktrace: add stackinfo_on_stack() helper Mark Rutland
@ 2022-08-01 12:12 ` Mark Rutland
  2022-08-01 17:14   ` Mark Brown
  2022-08-02  5:22   ` Kalesh Singh
  2022-08-01 12:12 ` [PATCH 6/8] arm64: stacktrace: remove stack type from fp translator Mark Rutland
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, maz, tabba, will

In subsequent patches we'll want to acquire the stack boundaries
ahead-of-time, and we'll need to be able to acquire the relevant
stack_info regardless of whether we have an object the happens to be on
the stack.

This patch replaces the on_XXX_stack() helpers with stackinfo_get_XXX()
helpers, with the caller being responsible for the checking whether an
object is on a relevant stack. For the moment this is moved into the
on_accessible_stack() functions, making these slightly larger;
subsequent patches will remove the on_accessible_stack() functions and
simplify the logic.

The on_irq_stack() and on_task_stack() helpers are kept as these are
used by IRQ entry sequences and stackleak respectively. As they're only
used as predicates, the stack_info pointer parameter is removed in both
cases.

As the on_accessible_stack() functions are always passed a non-NULL info
pointer, these now update info unconditionally. When updating the type
to STACK_TYPE_UNKNOWN, the low/high bounds are also modified, but as
these will not be consumed this should have no adverse affect.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/processor.h         |  2 +-
 arch/arm64/include/asm/stacktrace.h        | 78 +++++++++++++---------
 arch/arm64/include/asm/stacktrace/common.h | 28 +++-----
 arch/arm64/kernel/ptrace.c                 |  2 +-
 arch/arm64/kernel/stacktrace.c             | 67 ++++++++++++-------
 arch/arm64/kvm/hyp/nvhe/stacktrace.c       | 37 +++++++---
 arch/arm64/kvm/stacktrace.c                | 39 ++++++++---
 7 files changed, 154 insertions(+), 99 deletions(-)

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 9e58749db21df..5035e0394a8a0 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -409,7 +409,7 @@ long get_tagged_addr_ctrl(struct task_struct *task);
  * The top of the current task's task stack
  */
 #define current_top_of_stack()	((unsigned long)current->stack + THREAD_SIZE)
-#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, 1, NULL))
+#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, 1))
 
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index fa2df1ea22ebc..aad0c6258721d 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -22,77 +22,91 @@ extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 
 DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
 
-static inline bool on_irq_stack(unsigned long sp, unsigned long size,
-				struct stack_info *info)
+static inline struct stack_info stackinfo_get_irq(void)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
 	unsigned long high = low + IRQ_STACK_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_IRQ, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_IRQ,
+	};
 }
 
-static inline bool on_task_stack(const struct task_struct *tsk,
-				 unsigned long sp, unsigned long size,
-				 struct stack_info *info)
+static inline bool on_irq_stack(unsigned long sp, unsigned long size)
+{
+	struct stack_info info = stackinfo_get_irq();
+	return stackinfo_on_stack(&info, sp, size);
+}
+
+static inline struct stack_info stackinfo_get_task(const struct task_struct *tsk)
 {
 	unsigned long low = (unsigned long)task_stack_page(tsk);
 	unsigned long high = low + THREAD_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_TASK, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_TASK,
+	};
+}
+
+static inline bool on_task_stack(const struct task_struct *tsk,
+				 unsigned long sp, unsigned long size)
+{
+	struct stack_info info = stackinfo_get_task(tsk);
+	return stackinfo_on_stack(&info, sp, size);
 }
 
 #ifdef CONFIG_VMAP_STACK
 DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
 
-static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
-				struct stack_info *info)
+static inline struct stack_info stackinfo_get_overflow(void)
 {
 	unsigned long low = (unsigned long)raw_cpu_ptr(overflow_stack);
 	unsigned long high = low + OVERFLOW_STACK_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_OVERFLOW, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_OVERFLOW,
+	};
 }
 #else
-static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
-				     struct stack_info *info)
-{
-	return false;
-}
+#define stackinfo_get_overflow()	stackinfo_get_unknown()
 #endif
 
 #if defined(CONFIG_ARM_SDE_INTERFACE) && defined(CONFIG_VMAP_STACK)
 DECLARE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
 DECLARE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
 
-static inline bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
-					struct stack_info *info)
+static inline struct stack_info stackinfo_get_sdei_normal(void)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
 	unsigned long high = low + SDEI_STACK_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_SDEI_NORMAL, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_SDEI_NORMAL,
+	};
 }
 
-static inline bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
-					  struct stack_info *info)
+static inline struct stack_info stackinfo_get_sdei_critical(void)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
 	unsigned long high = low + SDEI_STACK_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_SDEI_CRITICAL, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_SDEI_CRITICAL,
+	};
 }
 #else
-static inline bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
-					struct stack_info *info)
-{
-	return false;
-}
-
-static inline bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
-					  struct stack_info *info)
-{
-	return false;
-}
+#define stackinfo_get_sdei_normal()	stackinfo_get_unknown()
+#define stackinfo_get_sdei_critical()	stackinfo_get_unknown()
 #endif
 
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index 9ed7feb493a36..0071f2459c703 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -66,6 +66,15 @@ struct unwind_state {
 	struct task_struct *task;
 };
 
+static inline struct stack_info stackinfo_get_unknown(void)
+{
+	return (struct stack_info) {
+		.low = 0,
+		.high = 0,
+		.type = STACK_TYPE_UNKNOWN,
+	};
+}
+
 static inline bool stackinfo_on_stack(const struct stack_info *info,
 				      unsigned long sp, unsigned long size)
 {
@@ -78,25 +87,6 @@ static inline bool stackinfo_on_stack(const struct stack_info *info,
 	return true;
 }
 
-static inline bool on_stack(unsigned long sp, unsigned long size,
-			    unsigned long low, unsigned long high,
-			    enum stack_type type, struct stack_info *info)
-{
-	struct stack_info tmp = {
-		.low = low,
-		.high = high,
-		.type = type,
-	};
-
-	if (!stackinfo_on_stack(&tmp, sp, size))
-		return false;
-
-	if (info)
-		*info = tmp;
-
-	return true;
-}
-
 static inline void unwind_init_common(struct unwind_state *state,
 				      struct task_struct *task)
 {
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 21da83187a602..2e1b721497794 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -121,7 +121,7 @@ static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
 {
 	return ((addr & ~(THREAD_SIZE - 1))  ==
 		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1))) ||
-		on_irq_stack(addr, sizeof(unsigned long), NULL);
+		on_irq_stack(addr, sizeof(unsigned long));
 }
 
 /**
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 04a9b56b114c1..ca56fd732c2a9 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -67,38 +67,55 @@ static inline void unwind_init_from_task(struct unwind_state *state,
 	state->pc = thread_saved_pc(task);
 }
 
-/*
- * We can only safely access per-cpu stacks from current in a non-preemptible
- * context.
- */
 static bool on_accessible_stack(const struct task_struct *tsk,
 				unsigned long sp, unsigned long size,
 				struct stack_info *info)
 {
-	if (info)
-		info->type = STACK_TYPE_UNKNOWN;
+	struct stack_info tmp;
 
-	if (on_task_stack(tsk, sp, size, info))
-		return true;
-	if (tsk != current || preemptible())
-		return false;
-	if (on_irq_stack(sp, size, info))
-		return true;
-	if (on_overflow_stack(sp, size, info))
-		return true;
-
-	if (IS_ENABLED(CONFIG_VMAP_STACK) &&
-	    IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) &&
-	    in_nmi())
-	{
-		if (on_sdei_critical_stack(sp, size, info))
-			return true;
-
-		if (on_sdei_normal_stack(sp, size, info))
-			return true;
-	}
+	tmp = stackinfo_get_task(tsk);
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
 
+	/*
+	 * We can only safely access per-cpu stacks when unwinding the current
+	 * task in a non-preemptible context.
+	 */
+	if (tsk != current || preemptible())
+		goto not_found;
+
+	tmp = stackinfo_get_irq();
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
+
+	tmp = stackinfo_get_overflow();
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
+
+	/*
+	 * We can only safely access SDEI stacks which unwinding the current
+	 * task in an NMI context.
+	 */
+	if (!IS_ENABLED(CONFIG_VMAP_STACK) ||
+	    !IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) ||
+	    !in_nmi())
+		goto not_found;
+
+	tmp = stackinfo_get_sdei_normal();
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
+
+	tmp = stackinfo_get_sdei_critical();
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
+
+not_found:
+	*info = stackinfo_get_unknown();
 	return false;
+
+found:
+	*info = tmp;
+	return true;
 }
 
 /*
diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
index 579b46aa9a553..5da0d44f61b73 100644
--- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
+++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
@@ -39,34 +39,51 @@ static void hyp_prepare_backtrace(unsigned long fp, unsigned long pc)
 
 DEFINE_PER_CPU(unsigned long [NVHE_STACKTRACE_SIZE/sizeof(long)], pkvm_stacktrace);
 
-static bool on_overflow_stack(unsigned long sp, unsigned long size,
-			      struct stack_info *info)
+static struct stack_info stackinfo_get_overflow(void)
 {
 	unsigned long low = (unsigned long)this_cpu_ptr(overflow_stack);
 	unsigned long high = low + OVERFLOW_STACK_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_OVERFLOW, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_OVERFLOW,
+	};
 }
 
-static bool on_hyp_stack(unsigned long sp, unsigned long size,
-			      struct stack_info *info)
+static struct stack_info stackinfo_get_hyp(void)
 {
 	struct kvm_nvhe_init_params *params = this_cpu_ptr(&kvm_init_params);
 	unsigned long high = params->stack_hyp_va;
 	unsigned long low = high - PAGE_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_HYP, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_HYP,
+	};
 }
 
 static bool on_accessible_stack(const struct task_struct *tsk,
 				unsigned long sp, unsigned long size,
 				struct stack_info *info)
 {
-	if (info)
-		info->type = STACK_TYPE_UNKNOWN;
+	struct stack_info tmp;
 
-	return (on_overflow_stack(sp, size, info) ||
-		on_hyp_stack(sp, size, info));
+	tmp = stackinfo_get_overflow();
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
+
+	tmp = stackinfo_get_hyp();
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
+
+	*info = stackinfo_get_unknown();
+	return false;
+
+found:
+	*info = tmp;
+	return true;
 }
 
 static int unwind_next(struct unwind_state *state)
diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
index b69c18a26567d..3b005530ac02a 100644
--- a/arch/arm64/kvm/stacktrace.c
+++ b/arch/arm64/kvm/stacktrace.c
@@ -62,37 +62,54 @@ static bool kvm_nvhe_stack_kern_va(unsigned long *addr,
 	return true;
 }
 
-static bool on_overflow_stack(unsigned long sp, unsigned long size,
-			      struct stack_info *info)
+static struct stack_info stackinfo_get_overflow(void)
 {
 	struct kvm_nvhe_stacktrace_info *stacktrace_info
 				= this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
 	unsigned long low = (unsigned long)stacktrace_info->overflow_stack_base;
 	unsigned long high = low + OVERFLOW_STACK_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_OVERFLOW, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_OVERFLOW,
+	};
 }
 
-static bool on_hyp_stack(unsigned long sp, unsigned long size,
-			 struct stack_info *info)
+static struct stack_info stackinfo_get_hyp(void)
 {
 	struct kvm_nvhe_stacktrace_info *stacktrace_info
 				= this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
 	unsigned long low = (unsigned long)stacktrace_info->stack_base;
 	unsigned long high = low + PAGE_SIZE;
 
-	return on_stack(sp, size, low, high, STACK_TYPE_HYP, info);
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_HYP,
+	};
 }
 
 static bool on_accessible_stack(const struct task_struct *tsk,
 				unsigned long sp, unsigned long size,
 				struct stack_info *info)
 {
-	if (info)
-		info->type = STACK_TYPE_UNKNOWN;
-
-	return (on_overflow_stack(sp, size, info) ||
-		on_hyp_stack(sp, size, info));
+	struct stack_info tmp;
+
+	tmp = stackinfo_get_overflow();
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
+	
+	tmp = stackinfo_get_hyp();
+	if (stackinfo_on_stack(&tmp, sp, size))
+		goto found;
+	
+	*info = stackinfo_get_unknown();
+	return false;
+
+found:
+	*info = tmp;
+	return true;
 }
 
 static int unwind_next(struct unwind_state *state)
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 6/8] arm64: stacktrace: remove stack type from fp translator
  2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
                   ` (4 preceding siblings ...)
  2022-08-01 12:12 ` [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery Mark Rutland
@ 2022-08-01 12:12 ` Mark Rutland
  2022-08-02  5:32   ` Kalesh Singh
  2022-08-03 12:36   ` Mark Brown
  2022-08-01 12:12 ` [PATCH 7/8] arm64: stacktrace: track all stack boundaries explicitly Mark Rutland
  2022-08-01 12:12 ` [PATCH 8/8] arm64: stacktrace: track hyp stacks in unwinder's address space Mark Rutland
  7 siblings, 2 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, maz, tabba, will

In subsequent patches we'll remove the stack_type enum, and move the FP
translation logic out of the raw FP unwind code.

In preparation for doing so, this patch removes the type parameter from
the FP translation callback, and modifies kvm_nvhe_stack_kern_va() to
determine the relevant stack directly.

So that kvm_nvhe_stack_kern_va() can use the stackinfo_*() helpers,
these are moved earlier in the file, but are not modified in any way.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/stacktrace/common.h |  6 +-
 arch/arm64/kvm/stacktrace.c                | 78 ++++++++++++----------
 2 files changed, 43 insertions(+), 41 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index 0071f2459c703..0bd9d7ad295e0 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -114,13 +114,11 @@ static inline void unwind_init_common(struct unwind_state *state,
  * a kernel address.
  *
  * @fp:   the frame pointer to be updated to its kernel address.
- * @type: the stack type associated with frame pointer @fp
  *
  * Returns true and success and @fp is updated to the corresponding
  * kernel virtual address; otherwise returns false.
  */
-typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp,
-					    enum stack_type type);
+typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp);
 
 /*
  * on_accessible_stack_fn() - Check whether a stack range is on any
@@ -165,7 +163,7 @@ unwind_next_frame_record(struct unwind_state *state,
 	 * If fp is not from the current address space perform the necessary
 	 * translation before dereferencing it to get the next fp.
 	 */
-	if (translate_fp && !translate_fp(&kern_fp, info.type))
+	if (translate_fp && !translate_fp(&kern_fp))
 		return -EINVAL;
 
 	/*
diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
index 3b005530ac02a..62ffa2b40da10 100644
--- a/arch/arm64/kvm/stacktrace.c
+++ b/arch/arm64/kvm/stacktrace.c
@@ -21,6 +21,34 @@
 
 #include <asm/stacktrace/nvhe.h>
 
+static struct stack_info stackinfo_get_overflow(void)
+{
+	struct kvm_nvhe_stacktrace_info *stacktrace_info
+				= this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
+	unsigned long low = (unsigned long)stacktrace_info->overflow_stack_base;
+	unsigned long high = low + OVERFLOW_STACK_SIZE;
+
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_OVERFLOW,
+	};
+}
+
+static struct stack_info stackinfo_get_hyp(void)
+{
+	struct kvm_nvhe_stacktrace_info *stacktrace_info
+				= this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
+	unsigned long low = (unsigned long)stacktrace_info->stack_base;
+	unsigned long high = low + PAGE_SIZE;
+
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+		.type = STACK_TYPE_HYP,
+	};
+}
+
 /*
  * kvm_nvhe_stack_kern_va - Convert KVM nVHE HYP stack addresses to a kernel VAs
  *
@@ -34,27 +62,31 @@
  * Returns true on success and updates @addr to its corresponding kernel VA;
  * otherwise returns false.
  */
-static bool kvm_nvhe_stack_kern_va(unsigned long *addr,
-				   enum stack_type type)
+static bool kvm_nvhe_stack_kern_va(unsigned long *addr)
 {
 	struct kvm_nvhe_stacktrace_info *stacktrace_info;
 	unsigned long hyp_base, kern_base, hyp_offset;
+	struct stack_info stack;
 
 	stacktrace_info = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
 
-	switch (type) {
-	case STACK_TYPE_HYP:
+	stack = stackinfo_get_hyp();
+	if (stackinfo_on_stack(&stack, *addr, 1)) {
 		kern_base = (unsigned long)*this_cpu_ptr(&kvm_arm_hyp_stack_page);
 		hyp_base = (unsigned long)stacktrace_info->stack_base;
-		break;
-	case STACK_TYPE_OVERFLOW:
+		goto found;
+	}
+
+	stack = stackinfo_get_overflow();
+	if (stackinfo_on_stack(&stack, *addr, 1)) {
 		kern_base = (unsigned long)this_cpu_ptr_nvhe_sym(overflow_stack);
 		hyp_base = (unsigned long)stacktrace_info->overflow_stack_base;
-		break;
-	default:
-		return false;
+		goto found;
 	}
 
+	return false;
+
+found:
 	hyp_offset = *addr - hyp_base;
 
 	*addr = kern_base + hyp_offset;
@@ -62,34 +94,6 @@ static bool kvm_nvhe_stack_kern_va(unsigned long *addr,
 	return true;
 }
 
-static struct stack_info stackinfo_get_overflow(void)
-{
-	struct kvm_nvhe_stacktrace_info *stacktrace_info
-				= this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
-	unsigned long low = (unsigned long)stacktrace_info->overflow_stack_base;
-	unsigned long high = low + OVERFLOW_STACK_SIZE;
-
-	return (struct stack_info) {
-		.low = low,
-		.high = high,
-		.type = STACK_TYPE_OVERFLOW,
-	};
-}
-
-static struct stack_info stackinfo_get_hyp(void)
-{
-	struct kvm_nvhe_stacktrace_info *stacktrace_info
-				= this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
-	unsigned long low = (unsigned long)stacktrace_info->stack_base;
-	unsigned long high = low + PAGE_SIZE;
-
-	return (struct stack_info) {
-		.low = low,
-		.high = high,
-		.type = STACK_TYPE_HYP,
-	};
-}
-
 static bool on_accessible_stack(const struct task_struct *tsk,
 				unsigned long sp, unsigned long size,
 				struct stack_info *info)
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 7/8] arm64: stacktrace: track all stack boundaries explicitly
  2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
                   ` (5 preceding siblings ...)
  2022-08-01 12:12 ` [PATCH 6/8] arm64: stacktrace: remove stack type from fp translator Mark Rutland
@ 2022-08-01 12:12 ` Mark Rutland
  2022-08-02 16:05   ` Kalesh Singh
  2022-08-01 12:12 ` [PATCH 8/8] arm64: stacktrace: track hyp stacks in unwinder's address space Mark Rutland
  7 siblings, 1 reply; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, maz, tabba, will

Currently we call an on_accessible_stack() callback for each step of the
unwinder, requiring redundant work to be performed in the core of the
unwind loop (e.g. disabling preemption around accesses to per-cpu
variables containing stack boundaries). To prevent unwind loops which go
through a stack multiple times, we have to track the set of unwound
stacks, requiring a stack_type enum which needs to cater for all the
stacks of all possible callees. To prevent loops within a stack, we must
track the prior FP values.

This patch reworks the unwinder to minimize the work in the core of the
unwinder, and to remove the need for the stack_type enum. The set of
accessible stacks (and their boundaries) are determined at the start of
the unwind, and the current stack is tracked during the unwind, with
completed stacks removed from the set of accessible stacks. This makes
the boundary checks more accurate (e.g. detecting overlapped frame
records), and removes the need for separate tracking of the prior FP and
visited stacks.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/stacktrace.h        |   5 -
 arch/arm64/include/asm/stacktrace/common.h | 158 ++++++++++-----------
 arch/arm64/kernel/stacktrace.c             |  91 +++++-------
 arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  35 ++---
 arch/arm64/kvm/stacktrace.c                |  36 ++---
 5 files changed, 131 insertions(+), 194 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index aad0c6258721d..5a0edb064ea47 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -30,7 +30,6 @@ static inline struct stack_info stackinfo_get_irq(void)
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_IRQ,
 	};
 }
 
@@ -48,7 +47,6 @@ static inline struct stack_info stackinfo_get_task(const struct task_struct *tsk
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_TASK,
 	};
 }
 
@@ -70,7 +68,6 @@ static inline struct stack_info stackinfo_get_overflow(void)
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_OVERFLOW,
 	};
 }
 #else
@@ -89,7 +86,6 @@ static inline struct stack_info stackinfo_get_sdei_normal(void)
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_SDEI_NORMAL,
 	};
 }
 
@@ -101,7 +97,6 @@ static inline struct stack_info stackinfo_get_sdei_critical(void)
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_SDEI_CRITICAL,
 	};
 }
 #else
diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index 0bd9d7ad295e0..c594f332bb946 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -9,26 +9,12 @@
 #ifndef __ASM_STACKTRACE_COMMON_H
 #define __ASM_STACKTRACE_COMMON_H
 
-#include <linux/bitmap.h>
-#include <linux/bitops.h>
 #include <linux/kprobes.h>
 #include <linux/types.h>
 
-enum stack_type {
-	STACK_TYPE_UNKNOWN,
-	STACK_TYPE_TASK,
-	STACK_TYPE_IRQ,
-	STACK_TYPE_OVERFLOW,
-	STACK_TYPE_SDEI_NORMAL,
-	STACK_TYPE_SDEI_CRITICAL,
-	STACK_TYPE_HYP,
-	__NR_STACK_TYPES
-};
-
 struct stack_info {
 	unsigned long low;
 	unsigned long high;
-	enum stack_type type;
 };
 
 /*
@@ -38,32 +24,27 @@ struct stack_info {
  * @fp:          The fp value in the frame record (or the real fp)
  * @pc:          The lr value in the frame record (or the real lr)
  *
- * @stacks_done: Stacks which have been entirely unwound, for which it is no
- *               longer valid to unwind to.
- *
- * @prev_fp:     The fp that pointed to this frame record, or a synthetic value
- *               of 0. This is used to ensure that within a stack, each
- *               subsequent frame record is at an increasing address.
- * @prev_type:   The type of stack this frame record was on, or a synthetic
- *               value of STACK_TYPE_UNKNOWN. This is used to detect a
- *               transition from one stack to another.
- *
  * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
  *               associated with the most recently encountered replacement lr
  *               value.
  *
  * @task:        The task being unwound.
+ *
+ * @stack:       The stack currently being unwound.
+ * @stacks:      An array of stacks which can be unwound.
+ * @nr_stacks:   The number of stacks in @stacks.
  */
 struct unwind_state {
 	unsigned long fp;
 	unsigned long pc;
-	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
-	unsigned long prev_fp;
-	enum stack_type prev_type;
 #ifdef CONFIG_KRETPROBES
 	struct llist_node *kr_cur;
 #endif
 	struct task_struct *task;
+
+	struct stack_info stack;
+	struct stack_info *stacks;
+	int nr_stacks;
 };
 
 static inline struct stack_info stackinfo_get_unknown(void)
@@ -71,7 +52,6 @@ static inline struct stack_info stackinfo_get_unknown(void)
 	return (struct stack_info) {
 		.low = 0,
 		.high = 0,
-		.type = STACK_TYPE_UNKNOWN,
 	};
 }
 
@@ -95,18 +75,7 @@ static inline void unwind_init_common(struct unwind_state *state,
 	state->kr_cur = NULL;
 #endif
 
-	/*
-	 * Prime the first unwind.
-	 *
-	 * In unwind_next() we'll check that the FP points to a valid stack,
-	 * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
-	 * treated as a transition to whichever stack that happens to be. The
-	 * prev_fp value won't be used, but we set it to 0 such that it is
-	 * definitely not an accessible stack address.
-	 */
-	bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
-	state->prev_fp = 0;
-	state->prev_type = STACK_TYPE_UNKNOWN;
+	state->stack = stackinfo_get_unknown();
 }
 
 /*
@@ -120,44 +89,91 @@ static inline void unwind_init_common(struct unwind_state *state,
  */
 typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp);
 
+static struct stack_info *unwind_find_next_stack(const struct unwind_state *state,
+						 unsigned long sp,
+						 unsigned long size)
+{
+	for (int i = 0; i < state->nr_stacks; i++) {
+		struct stack_info *info = &state->stacks[i];
+
+		if (stackinfo_on_stack(info, sp, size))
+			return info;
+	}
+
+	return NULL;
+}
+
 /*
- * on_accessible_stack_fn() - Check whether a stack range is on any
- * of the possible stacks.
+ * unwind_consume_stack - Check if an object is on an accessible stack,
+ * updating stack boundaries so that future unwind steps cannot consume this
+ * object again.
  *
- * @tsk:  task whose stack is being unwound
- * @sp:   stack address being checked
- * @size: size of the stack range being checked
- * @info: stack unwinding context
+ * @state: the current unwind state.
+ * @sp:    the base address of the object.
+ * @size:  the size of the object.
  */
-typedef bool (*on_accessible_stack_fn)(const struct task_struct *tsk,
-				       unsigned long sp, unsigned long size,
-				       struct stack_info *info);
+static inline int unwind_consume_stack(struct unwind_state *state,
+				       unsigned long sp,
+				       unsigned long size)
+{
+	struct stack_info *next;
+
+	if (stackinfo_on_stack(&state->stack, sp, size))
+		goto found;
+
+	next = unwind_find_next_stack(state, sp, size);
+	if (!next)
+		return -EINVAL;
+
+	/*
+	 * Stack transitions are strictly one-way, and once we've
+	 * transitioned from one stack to another, it's never valid to
+	 * unwind back to the old stack.
+	 *
+	 * Remove the current stack from the list of stacks so that it cannot
+	 * be found on a subsequent transition.
+	 *
+	 * Note that stacks can nest in several valid orders, e.g.
+	 *
+	 *   TASK -> IRQ -> OVERFLOW -> SDEI_NORMAL
+	 *   TASK -> SDEI_NORMAL -> SDEI_CRITICAL -> OVERFLOW
+	 *   HYP -> OVERFLOW
+	 *
+	 * ... so we do not check the specific order of stack
+	 * transitions.
+	 */
+	state->stack = *next;
+	*next = stackinfo_get_unknown();
+
+found:
+	/*
+	 * Future unwind steps can only consume stack above this frame record.
+	 * Update the current stack to start immediately above it.
+	 */
+	state->stack.low = sp + size;
+	return 0;
+}
 
 /*
  * unwind_next_frame_record() - Unwind to the next frame record indicated by
  * @state->fp.
  *
  * @state:        the current unwind state.
- * @accessible:   determines whether the frame record is accessible
  * @translate_fp: translates the fp prior to access (may be NULL)
  */
 static inline int
 unwind_next_frame_record(struct unwind_state *state,
-			 on_accessible_stack_fn accessible,
 			 stack_trace_translate_fp_fn translate_fp)
 {
-	struct stack_info info;
 	unsigned long fp = state->fp, kern_fp = fp;
-	struct task_struct *tsk = state->task;
+	int err;
 
 	if (fp & 0x7)
 		return -EINVAL;
 
-	if (!accessible(tsk, fp, 16, &info))
-		return -EINVAL;
-
-	if (test_bit(info.type, state->stacks_done))
-		return -EINVAL;
+	err = unwind_consume_stack(state, fp, 16);
+	if (err)
+		return err;
 
 	/*
 	 * If fp is not from the current address space perform the necessary
@@ -167,34 +183,10 @@ unwind_next_frame_record(struct unwind_state *state,
 		return -EINVAL;
 
 	/*
-	 * As stacks grow downward, any valid record on the same stack must be
-	 * at a strictly higher address than the prior record.
-	 *
-	 * Stacks can nest in several valid orders, e.g.
-	 *
-	 * TASK -> IRQ -> OVERFLOW -> SDEI_NORMAL
-	 * TASK -> SDEI_NORMAL -> SDEI_CRITICAL -> OVERFLOW
-	 * HYP -> OVERFLOW
-	 *
-	 * ... but the nesting itself is strict. Once we transition from one
-	 * stack to another, it's never valid to unwind back to that first
-	 * stack.
-	 */
-	if (info.type == state->prev_type) {
-		if (fp <= state->prev_fp)
-			return -EINVAL;
-	} else {
-		__set_bit(state->prev_type, state->stacks_done);
-	}
-
-	/*
-	 * Record this frame record's values and location. The prev_fp and
-	 * prev_type are only meaningful to the next unwind_next() invocation.
+	 * Record this frame record's values.
 	 */
 	state->fp = READ_ONCE(*(unsigned long *)(kern_fp));
 	state->pc = READ_ONCE(*(unsigned long *)(kern_fp + 8));
-	state->prev_fp = fp;
-	state->prev_type = info.type;
 
 	return 0;
 }
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ca56fd732c2a9..1133be3ff774d 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -67,57 +67,6 @@ static inline void unwind_init_from_task(struct unwind_state *state,
 	state->pc = thread_saved_pc(task);
 }
 
-static bool on_accessible_stack(const struct task_struct *tsk,
-				unsigned long sp, unsigned long size,
-				struct stack_info *info)
-{
-	struct stack_info tmp;
-
-	tmp = stackinfo_get_task(tsk);
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-
-	/*
-	 * We can only safely access per-cpu stacks when unwinding the current
-	 * task in a non-preemptible context.
-	 */
-	if (tsk != current || preemptible())
-		goto not_found;
-
-	tmp = stackinfo_get_irq();
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-
-	tmp = stackinfo_get_overflow();
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-
-	/*
-	 * We can only safely access SDEI stacks which unwinding the current
-	 * task in an NMI context.
-	 */
-	if (!IS_ENABLED(CONFIG_VMAP_STACK) ||
-	    !IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) ||
-	    !in_nmi())
-		goto not_found;
-
-	tmp = stackinfo_get_sdei_normal();
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-
-	tmp = stackinfo_get_sdei_critical();
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-
-not_found:
-	*info = stackinfo_get_unknown();
-	return false;
-
-found:
-	*info = tmp;
-	return true;
-}
-
 /*
  * Unwind from one frame record (A) to the next frame record (B).
  *
@@ -135,7 +84,7 @@ static int notrace unwind_next(struct unwind_state *state)
 	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
 		return -ENOENT;
 
-	err = unwind_next_frame_record(state, on_accessible_stack, NULL);
+	err = unwind_next_frame_record(state, NULL);
 	if (err)
 		return err;
 
@@ -215,11 +164,47 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl)
 	barrier();
 }
 
+/*
+ * Per-cpu stacks are only accessible when unwinding the current task in a
+ * non-preemptible context.
+ */
+#define STACKINFO_CPU(name)					\
+	({							\
+		((task == current) && !preemptible())		\
+			? stackinfo_get_##name()		\
+			: stackinfo_get_unknown();		\
+	})
+
+/*
+ * SDEI stacks are only accessible when unwinding the current task in an NMI
+ * context.
+ */
+#define STACKINFO_SDEI(name)					\
+	({							\
+		((task == current) && in_nmi())			\
+			? stackinfo_get_sdei_##name()		\
+			: stackinfo_get_unknown();		\
+	})
+
 noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 			      void *cookie, struct task_struct *task,
 			      struct pt_regs *regs)
 {
-	struct unwind_state state;
+	struct stack_info stacks[] = {
+		stackinfo_get_task(task),
+		STACKINFO_CPU(irq),
+#if defined (CONFIG_VMAP_STACK)
+		STACKINFO_CPU(overflow),
+#endif
+#if defined(CONFIG_VMAP_STACK) && defined(CONFIG_ARM_SDE_INTERFACE)
+		STACKINFO_SDEI(normal),
+		STACKINFO_SDEI(critical),
+#endif
+	};
+	struct unwind_state state = {
+		.stacks = stacks,
+		.nr_stacks = ARRAY_SIZE(stacks),
+	};
 
 	if (regs) {
 		if (task != current)
diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
index 5da0d44f61b73..08e1325ead73f 100644
--- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
+++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
@@ -47,7 +47,6 @@ static struct stack_info stackinfo_get_overflow(void)
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_OVERFLOW,
 	};
 }
 
@@ -60,35 +59,12 @@ static struct stack_info stackinfo_get_hyp(void)
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_HYP,
 	};
 }
 
-static bool on_accessible_stack(const struct task_struct *tsk,
-				unsigned long sp, unsigned long size,
-				struct stack_info *info)
-{
-	struct stack_info tmp;
-
-	tmp = stackinfo_get_overflow();
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-
-	tmp = stackinfo_get_hyp();
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-
-	*info = stackinfo_get_unknown();
-	return false;
-
-found:
-	*info = tmp;
-	return true;
-}
-
 static int unwind_next(struct unwind_state *state)
 {
-	return unwind_next_frame_record(state, on_accessible_stack, NULL);
+	return unwind_next_frame_record(state, NULL);
 }
 
 static void notrace unwind(struct unwind_state *state,
@@ -144,7 +120,14 @@ static bool pkvm_save_backtrace_entry(void *arg, unsigned long where)
  */
 static void pkvm_save_backtrace(unsigned long fp, unsigned long pc)
 {
-	struct unwind_state state;
+	struct stack_info stacks[] = {
+		stackinfo_get_overflow(),
+		stackinfo_get_hyp(),
+	};
+	struct unwind_state state = {
+		.stacks = stacks,
+		.nr_stacks = ARRAY_SIZE(stacks),
+	};
 	int idx = 0;
 
 	kvm_nvhe_unwind_init(&state, fp, pc);
diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
index 62ffa2b40da10..8295e132da2f0 100644
--- a/arch/arm64/kvm/stacktrace.c
+++ b/arch/arm64/kvm/stacktrace.c
@@ -31,7 +31,6 @@ static struct stack_info stackinfo_get_overflow(void)
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_OVERFLOW,
 	};
 }
 
@@ -45,7 +44,6 @@ static struct stack_info stackinfo_get_hyp(void)
 	return (struct stack_info) {
 		.low = low,
 		.high = high,
-		.type = STACK_TYPE_HYP,
 	};
 }
 
@@ -94,32 +92,9 @@ static bool kvm_nvhe_stack_kern_va(unsigned long *addr)
 	return true;
 }
 
-static bool on_accessible_stack(const struct task_struct *tsk,
-				unsigned long sp, unsigned long size,
-				struct stack_info *info)
-{
-	struct stack_info tmp;
-
-	tmp = stackinfo_get_overflow();
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-	
-	tmp = stackinfo_get_hyp();
-	if (stackinfo_on_stack(&tmp, sp, size))
-		goto found;
-	
-	*info = stackinfo_get_unknown();
-	return false;
-
-found:
-	*info = tmp;
-	return true;
-}
-
 static int unwind_next(struct unwind_state *state)
 {
-	return unwind_next_frame_record(state, on_accessible_stack,
-					kvm_nvhe_stack_kern_va);
+	return unwind_next_frame_record(state, kvm_nvhe_stack_kern_va);
 }
 
 static void unwind(struct unwind_state *state,
@@ -177,7 +152,14 @@ static void kvm_nvhe_dump_backtrace_end(void)
 static void hyp_dump_backtrace(unsigned long hyp_offset)
 {
 	struct kvm_nvhe_stacktrace_info *stacktrace_info;
-	struct unwind_state state;
+	struct stack_info stacks[] = {
+		stackinfo_get_overflow(),
+		stackinfo_get_hyp(),
+	};
+	struct unwind_state state = {
+		.stacks = stacks,
+		.nr_stacks = ARRAY_SIZE(stacks),
+	};
 
 	stacktrace_info = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
 
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* [PATCH 8/8] arm64: stacktrace: track hyp stacks in unwinder's address space
  2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
                   ` (6 preceding siblings ...)
  2022-08-01 12:12 ` [PATCH 7/8] arm64: stacktrace: track all stack boundaries explicitly Mark Rutland
@ 2022-08-01 12:12 ` Mark Rutland
  2022-08-02 16:27   ` Kalesh Singh
  7 siblings, 1 reply; 28+ messages in thread
From: Mark Rutland @ 2022-08-01 12:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	mark.rutland, maz, tabba, will

Currently unwind_next_frame_record() has an optional callback to convert
the address space of the FP. This is necessary for the NVHE unwinder,
which tracks the stacks in the hyp VA space, but accesses the frame
records in the kernel VA space.

This is a bit unfortunate since it clutters unwind_next_frame_record(),
which will get in the way of future rework.

Instead, this patch changes the NVHE unwinder to track the stacks in the
kernel's VA space and translate to FP prior to calling
unwind_next_frame_record(). This removes the need for the translate_fp()
callback, as all unwinders consistently track stacks in the native
address space of the unwinder.

At the same time, this patch consolidates the generation of the stack
addreses behind the stackinfo_get_*() helpers.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Fuad Tabba <tabba@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/stacktrace/common.h | 28 ++--------
 arch/arm64/kernel/stacktrace.c             |  2 +-
 arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  2 +-
 arch/arm64/kvm/stacktrace.c                | 62 ++++++++++++++--------
 4 files changed, 46 insertions(+), 48 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index c594f332bb946..0f634bb14ceb3 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -78,17 +78,6 @@ static inline void unwind_init_common(struct unwind_state *state,
 	state->stack = stackinfo_get_unknown();
 }
 
-/*
- * stack_trace_translate_fp_fn() - Translates a non-kernel frame pointer to
- * a kernel address.
- *
- * @fp:   the frame pointer to be updated to its kernel address.
- *
- * Returns true and success and @fp is updated to the corresponding
- * kernel virtual address; otherwise returns false.
- */
-typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp);
-
 static struct stack_info *unwind_find_next_stack(const struct unwind_state *state,
 						 unsigned long sp,
 						 unsigned long size)
@@ -159,13 +148,11 @@ static inline int unwind_consume_stack(struct unwind_state *state,
  * @state->fp.
  *
  * @state:        the current unwind state.
- * @translate_fp: translates the fp prior to access (may be NULL)
  */
 static inline int
-unwind_next_frame_record(struct unwind_state *state,
-			 stack_trace_translate_fp_fn translate_fp)
+unwind_next_frame_record(struct unwind_state *state)
 {
-	unsigned long fp = state->fp, kern_fp = fp;
+	unsigned long fp = state->fp;
 	int err;
 
 	if (fp & 0x7)
@@ -175,18 +162,11 @@ unwind_next_frame_record(struct unwind_state *state,
 	if (err)
 		return err;
 
-	/*
-	 * If fp is not from the current address space perform the necessary
-	 * translation before dereferencing it to get the next fp.
-	 */
-	if (translate_fp && !translate_fp(&kern_fp))
-		return -EINVAL;
-
 	/*
 	 * Record this frame record's values.
 	 */
-	state->fp = READ_ONCE(*(unsigned long *)(kern_fp));
-	state->pc = READ_ONCE(*(unsigned long *)(kern_fp + 8));
+	state->fp = READ_ONCE(*(unsigned long *)(fp));
+	state->pc = READ_ONCE(*(unsigned long *)(fp + 8));
 
 	return 0;
 }
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 1133be3ff774d..6b447319ce5b9 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -84,7 +84,7 @@ static int notrace unwind_next(struct unwind_state *state)
 	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
 		return -ENOENT;
 
-	err = unwind_next_frame_record(state, NULL);
+	err = unwind_next_frame_record(state);
 	if (err)
 		return err;
 
diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
index 08e1325ead73f..ed6b58b19cfa5 100644
--- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
+++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
@@ -64,7 +64,7 @@ static struct stack_info stackinfo_get_hyp(void)
 
 static int unwind_next(struct unwind_state *state)
 {
-	return unwind_next_frame_record(state, NULL);
+	return unwind_next_frame_record(state);
 }
 
 static void notrace unwind(struct unwind_state *state,
diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
index 8295e132da2f0..fde1ec757d03a 100644
--- a/arch/arm64/kvm/stacktrace.c
+++ b/arch/arm64/kvm/stacktrace.c
@@ -34,6 +34,17 @@ static struct stack_info stackinfo_get_overflow(void)
 	};
 }
 
+static struct stack_info stackinfo_get_overflow_kern_va(void)
+{
+	unsigned long low = (unsigned long)this_cpu_ptr_nvhe_sym(overflow_stack);
+	unsigned long high = low + OVERFLOW_STACK_SIZE;
+
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+	};
+}
+
 static struct stack_info stackinfo_get_hyp(void)
 {
 	struct kvm_nvhe_stacktrace_info *stacktrace_info
@@ -47,6 +58,17 @@ static struct stack_info stackinfo_get_hyp(void)
 	};
 }
 
+static struct stack_info stackinfo_get_hyp_kern_va(void)
+{
+	unsigned long low = (unsigned long)*this_cpu_ptr(&kvm_arm_hyp_stack_page);
+	unsigned long high = low + PAGE_SIZE;
+
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+	};
+}
+
 /*
  * kvm_nvhe_stack_kern_va - Convert KVM nVHE HYP stack addresses to a kernel VAs
  *
@@ -62,39 +84,35 @@ static struct stack_info stackinfo_get_hyp(void)
  */
 static bool kvm_nvhe_stack_kern_va(unsigned long *addr)
 {
-	struct kvm_nvhe_stacktrace_info *stacktrace_info;
-	unsigned long hyp_base, kern_base, hyp_offset;
-	struct stack_info stack;
+	struct stack_info stack_hyp, stack_kern;
 
-	stacktrace_info = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
-
-	stack = stackinfo_get_hyp();
-	if (stackinfo_on_stack(&stack, *addr, 1)) {
-		kern_base = (unsigned long)*this_cpu_ptr(&kvm_arm_hyp_stack_page);
-		hyp_base = (unsigned long)stacktrace_info->stack_base;
+	stack_hyp = stackinfo_get_hyp();
+	stack_kern = stackinfo_get_hyp_kern_va();
+	if (stackinfo_on_stack(&stack_hyp, *addr, 1))
 		goto found;
-	}
 
-	stack = stackinfo_get_overflow();
-	if (stackinfo_on_stack(&stack, *addr, 1)) {
-		kern_base = (unsigned long)this_cpu_ptr_nvhe_sym(overflow_stack);
-		hyp_base = (unsigned long)stacktrace_info->overflow_stack_base;
+	stack_hyp = stackinfo_get_overflow();
+	stack_kern = stackinfo_get_overflow_kern_va();
+	if (stackinfo_on_stack(&stack_hyp, *addr, 1))
 		goto found;
-	}
 
 	return false;
 
 found:
-	hyp_offset = *addr - hyp_base;
-
-	*addr = kern_base + hyp_offset;
-
+	*addr = *addr - stack_hyp.low + stack_kern.low;
 	return true;
 }
 
 static int unwind_next(struct unwind_state *state)
 {
-	return unwind_next_frame_record(state, kvm_nvhe_stack_kern_va);
+	/*
+	 * The FP is in the hypervisor VA space. Convert it to the kernel VA
+	 * space so it can be unwound be the regular unwind functions.
+	 */
+	if (!kvm_nvhe_stack_kern_va(&state->fp))
+		return -EINVAL;
+
+	return unwind_next_frame_record(state);
 }
 
 static void unwind(struct unwind_state *state,
@@ -153,8 +171,8 @@ static void hyp_dump_backtrace(unsigned long hyp_offset)
 {
 	struct kvm_nvhe_stacktrace_info *stacktrace_info;
 	struct stack_info stacks[] = {
-		stackinfo_get_overflow(),
-		stackinfo_get_hyp(),
+		stackinfo_get_overflow_kern_va(),
+		stackinfo_get_hyp_kern_va(),
 	};
 	struct unwind_state state = {
 		.stacks = stacks,
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread
* Re: [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common()
  2022-08-01 12:12 ` [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common() Mark Rutland
@ 2022-08-01 12:46   ` Mark Brown
  2022-08-02  4:34   ` Kalesh Singh
  1 sibling, 0 replies; 28+ messages in thread
From: Mark Brown @ 2022-08-01 12:46 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, catalin.marinas, james.morse, kaleshsingh,
	madvenka, maz, tabba, will


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

On Mon, Aug 01, 2022 at 01:12:02PM +0100, Mark Rutland wrote:
> Currently unwind_next_common() takes a pointer to a stack_info which is
> only ever used within unwind_next_common().
> 
> Make it a local variable and simplify callers.
> 
> There should be no functional change as a result of this patch.

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

[-- 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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code
  2022-08-01 12:12 ` [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code Mark Rutland
@ 2022-08-01 12:48   ` Mark Brown
  2022-08-02  4:53   ` Kalesh Singh
  2022-08-04 14:38   ` Mark Rutland
  2 siblings, 0 replies; 28+ messages in thread
From: Mark Brown @ 2022-08-01 12:48 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, catalin.marinas, james.morse, kaleshsingh,
	madvenka, maz, tabba, will


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

On Mon, Aug 01, 2022 at 01:12:04PM +0100, Mark Rutland wrote:
> For clarity and ease of maintenance, it would be helpful for all the
> stack helpers to be in the same place.
> 
> Move the SDEI stack helpers into the stacktrace code where all the other
> stack helpers live.

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

[-- 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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record()
  2022-08-01 12:12 ` [PATCH 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record() Mark Rutland
@ 2022-08-01 12:52   ` Mark Brown
  2022-08-02  4:38   ` Kalesh Singh
  1 sibling, 0 replies; 28+ messages in thread
From: Mark Brown @ 2022-08-01 12:52 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, catalin.marinas, james.morse, kaleshsingh,
	madvenka, maz, tabba, will


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

On Mon, Aug 01, 2022 at 01:12:03PM +0100, Mark Rutland wrote:
> The unwind_next_common() function unwinds a single frame record. There
> are other unwind steps (e.g. unwinding through trampolines) which are
> handled in the regular kernel unwinder, and in future there may be other
> common unwind helpers.

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

[-- 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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 4/8] arm64: stacktrace: add stackinfo_on_stack() helper
  2022-08-01 12:12 ` [PATCH 4/8] arm64: stacktrace: add stackinfo_on_stack() helper Mark Rutland
@ 2022-08-01 15:56   ` Mark Brown
  2022-08-02  5:00   ` Kalesh Singh
  1 sibling, 0 replies; 28+ messages in thread
From: Mark Brown @ 2022-08-01 15:56 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, catalin.marinas, james.morse, kaleshsingh,
	madvenka, maz, tabba, will


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

On Mon, Aug 01, 2022 at 01:12:05PM +0100, Mark Rutland wrote:
> Factor the core predicate out of on_stack() into a helper which can be
> used on a pre-populated stack_info.
> 
> There should be no functional change as a result of this patch.

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

[-- 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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery
  2022-08-01 12:12 ` [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery Mark Rutland
@ 2022-08-01 17:14   ` Mark Brown
  2022-08-02  5:22   ` Kalesh Singh
  1 sibling, 0 replies; 28+ messages in thread
From: Mark Brown @ 2022-08-01 17:14 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, catalin.marinas, james.morse, kaleshsingh,
	madvenka, maz, tabba, will


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

On Mon, Aug 01, 2022 at 01:12:06PM +0100, Mark Rutland wrote:

> In subsequent patches we'll want to acquire the stack boundaries
> ahead-of-time, and we'll need to be able to acquire the relevant
> stack_info regardless of whether we have an object the happens to be on
> the stack.

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

[-- 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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common()
  2022-08-01 12:12 ` [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common() Mark Rutland
  2022-08-01 12:46   ` Mark Brown
@ 2022-08-02  4:34   ` Kalesh Singh
  1 sibling, 0 replies; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02  4:34 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Currently unwind_next_common() takes a pointer to a stack_info which is
> only ever used within unwind_next_common().
>
> Make it a local variable and simplify callers.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>

Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

> ---
>  arch/arm64/include/asm/stacktrace/common.h | 12 ++++++------
>  arch/arm64/kernel/stacktrace.c             |  3 +--
>  arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  4 +---
>  arch/arm64/kvm/stacktrace.c                |  4 +---
>  4 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index f58eb944c46fb..01dc9f44a24a7 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -140,27 +140,27 @@ typedef bool (*on_accessible_stack_fn)(const struct task_struct *tsk,
>                                        struct stack_info *info);
>
>  static inline int unwind_next_common(struct unwind_state *state,
> -                                    struct stack_info *info,
>                                      on_accessible_stack_fn accessible,
>                                      stack_trace_translate_fp_fn translate_fp)
>  {
> +       struct stack_info info;
>         unsigned long fp = state->fp, kern_fp = fp;
>         struct task_struct *tsk = state->task;
>
>         if (fp & 0x7)
>                 return -EINVAL;
>
> -       if (!accessible(tsk, fp, 16, info))
> +       if (!accessible(tsk, fp, 16, &info))
>                 return -EINVAL;
>
> -       if (test_bit(info->type, state->stacks_done))
> +       if (test_bit(info.type, state->stacks_done))
>                 return -EINVAL;
>
>         /*
>          * If fp is not from the current address space perform the necessary
>          * translation before dereferencing it to get the next fp.
>          */
> -       if (translate_fp && !translate_fp(&kern_fp, info->type))
> +       if (translate_fp && !translate_fp(&kern_fp, info.type))
>                 return -EINVAL;
>
>         /*
> @@ -177,7 +177,7 @@ static inline int unwind_next_common(struct unwind_state *state,
>          * stack to another, it's never valid to unwind back to that first
>          * stack.
>          */
> -       if (info->type == state->prev_type) {
> +       if (info.type == state->prev_type) {
>                 if (fp <= state->prev_fp)
>                         return -EINVAL;
>         } else {
> @@ -191,7 +191,7 @@ static inline int unwind_next_common(struct unwind_state *state,
>         state->fp = READ_ONCE(*(unsigned long *)(kern_fp));
>         state->pc = READ_ONCE(*(unsigned long *)(kern_fp + 8));
>         state->prev_fp = fp;
> -       state->prev_type = info->type;
> +       state->prev_type = info.type;
>
>         return 0;
>  }
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index ce190ee18a201..056fb045d0e0c 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -103,14 +103,13 @@ static int notrace unwind_next(struct unwind_state *state)
>  {
>         struct task_struct *tsk = state->task;
>         unsigned long fp = state->fp;
> -       struct stack_info info;
>         int err;
>
>         /* Final frame; nothing to unwind */
>         if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
>                 return -ENOENT;
>
> -       err = unwind_next_common(state, &info, on_accessible_stack, NULL);
> +       err = unwind_next_common(state, on_accessible_stack, NULL);
>         if (err)
>                 return err;
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> index 58f645ad66bcb..50a1fa6b5c044 100644
> --- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> +++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> @@ -71,9 +71,7 @@ static bool on_accessible_stack(const struct task_struct *tsk,
>
>  static int unwind_next(struct unwind_state *state)
>  {
> -       struct stack_info info;
> -
> -       return unwind_next_common(state, &info, on_accessible_stack, NULL);
> +       return unwind_next_common(state, on_accessible_stack, NULL);
>  }
>
>  static void notrace unwind(struct unwind_state *state,
> diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
> index 949d19d603fba..b9cf551d9d31d 100644
> --- a/arch/arm64/kvm/stacktrace.c
> +++ b/arch/arm64/kvm/stacktrace.c
> @@ -97,9 +97,7 @@ static bool on_accessible_stack(const struct task_struct *tsk,
>
>  static int unwind_next(struct unwind_state *state)
>  {
> -       struct stack_info info;
> -
> -       return unwind_next_common(state, &info, on_accessible_stack,
> +       return unwind_next_common(state, on_accessible_stack,
>                                   kvm_nvhe_stack_kern_va);
>  }
>
> --
> 2.30.2
>

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record()
  2022-08-01 12:12 ` [PATCH 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record() Mark Rutland
  2022-08-01 12:52   ` Mark Brown
@ 2022-08-02  4:38   ` Kalesh Singh
  1 sibling, 0 replies; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02  4:38 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> The unwind_next_common() function unwinds a single frame record. There
> are other unwind steps (e.g. unwinding through trampolines) which are
> handled in the regular kernel unwinder, and in future there may be other
> common unwind helpers.
>
> Clarify the purpose of unwind_next_common() by renaming it to
> unwind_next_frame_record(). At the same time, add commentary, and delete
> the redundant comment at the top of asm/stacktrace/common.h.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>

Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

> ---
>  arch/arm64/include/asm/stacktrace/common.h | 22 ++++++++++++----------
>  arch/arm64/kernel/stacktrace.c             |  2 +-
>  arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  2 +-
>  arch/arm64/kvm/stacktrace.c                |  4 ++--
>  4 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 01dc9f44a24a7..676002d7d333c 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -2,13 +2,6 @@
>  /*
>   * Common arm64 stack unwinder code.
>   *
> - * To implement a new arm64 stack unwinder:
> - *     1) Include this header
> - *
> - *     2) Call into unwind_next_common() from your top level unwind
> - *        function, passing it the validation and translation callbacks
> - *        (though the later can be NULL if no translation is required).
> - *
>   * See: arch/arm64/kernel/stacktrace.c for the reference implementation.
>   *
>   * Copyright (C) 2012 ARM Ltd.
> @@ -139,9 +132,18 @@ typedef bool (*on_accessible_stack_fn)(const struct task_struct *tsk,
>                                        unsigned long sp, unsigned long size,
>                                        struct stack_info *info);
>
> -static inline int unwind_next_common(struct unwind_state *state,
> -                                    on_accessible_stack_fn accessible,
> -                                    stack_trace_translate_fp_fn translate_fp)
> +/*
> + * unwind_next_frame_record() - Unwind to the next frame record indicated by
> + * @state->fp.
> + *
> + * @state:        the current unwind state.
> + * @accessible:   determines whether the frame record is accessible
> + * @translate_fp: translates the fp prior to access (may be NULL)
> + */
> +static inline int
> +unwind_next_frame_record(struct unwind_state *state,
> +                        on_accessible_stack_fn accessible,
> +                        stack_trace_translate_fp_fn translate_fp)
>  {
>         struct stack_info info;
>         unsigned long fp = state->fp, kern_fp = fp;
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 056fb045d0e0c..4c8865e495fea 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -109,7 +109,7 @@ static int notrace unwind_next(struct unwind_state *state)
>         if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
>                 return -ENOENT;
>
> -       err = unwind_next_common(state, on_accessible_stack, NULL);
> +       err = unwind_next_frame_record(state, on_accessible_stack, NULL);
>         if (err)
>                 return err;
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> index 50a1fa6b5c044..579b46aa9a553 100644
> --- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> +++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> @@ -71,7 +71,7 @@ static bool on_accessible_stack(const struct task_struct *tsk,
>
>  static int unwind_next(struct unwind_state *state)
>  {
> -       return unwind_next_common(state, on_accessible_stack, NULL);
> +       return unwind_next_frame_record(state, on_accessible_stack, NULL);
>  }
>
>  static void notrace unwind(struct unwind_state *state,
> diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
> index b9cf551d9d31d..b69c18a26567d 100644
> --- a/arch/arm64/kvm/stacktrace.c
> +++ b/arch/arm64/kvm/stacktrace.c
> @@ -97,8 +97,8 @@ static bool on_accessible_stack(const struct task_struct *tsk,
>
>  static int unwind_next(struct unwind_state *state)
>  {
> -       return unwind_next_common(state, on_accessible_stack,
> -                                 kvm_nvhe_stack_kern_va);
> +       return unwind_next_frame_record(state, on_accessible_stack,
> +                                       kvm_nvhe_stack_kern_va);
>  }
>
>  static void unwind(struct unwind_state *state,
> --
> 2.30.2
>

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code
  2022-08-01 12:12 ` [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code Mark Rutland
  2022-08-01 12:48   ` Mark Brown
@ 2022-08-02  4:53   ` Kalesh Singh
  2022-08-02 12:26     ` Mark Rutland
  2022-08-04 14:38   ` Mark Rutland
  2 siblings, 1 reply; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02  4:53 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> For clarity and ease of maintenance, it would be helpful for all the
> stack helpers to be in the same place.
>
> Move the SDEI stack helpers into the stacktrace code where all the other
> stack helpers live.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/sdei.h       | 17 ------------
>  arch/arm64/include/asm/stacktrace.h | 40 ++++++++++++++++++++++++++++-
>  arch/arm64/kernel/sdei.c            | 35 -------------------------
>  arch/arm64/kernel/stacktrace.c      | 13 ++++++++--
>  4 files changed, 50 insertions(+), 55 deletions(-)
>
> diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
> index 7bea1d705dd64..4292d9bafb9d2 100644
> --- a/arch/arm64/include/asm/sdei.h
> +++ b/arch/arm64/include/asm/sdei.h
> @@ -43,22 +43,5 @@ unsigned long do_sdei_event(struct pt_regs *regs,
>  unsigned long sdei_arch_get_entry_point(int conduit);
>  #define sdei_arch_get_entry_point(x)   sdei_arch_get_entry_point(x)
>
> -struct stack_info;
> -
> -bool _on_sdei_stack(unsigned long sp, unsigned long size,
> -                   struct stack_info *info);
> -static inline bool on_sdei_stack(unsigned long sp, unsigned long size,
> -                               struct stack_info *info)
> -{
> -       if (!IS_ENABLED(CONFIG_VMAP_STACK))
> -               return false;
> -       if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
> -               return false;
> -       if (in_nmi())
> -               return _on_sdei_stack(sp, size, info);
> -
> -       return false;
> -}
> -
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_SDEI_H */
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 6ebdcdff77f56..fa2df1ea22ebc 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -54,7 +54,45 @@ static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
>  }
>  #else
>  static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
> -                       struct stack_info *info) { return false; }
> +                                    struct stack_info *info)
> +{
> +       return false;
> +}
> +#endif
> +
> +#if defined(CONFIG_ARM_SDE_INTERFACE) && defined(CONFIG_VMAP_STACK)
> +DECLARE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
> +DECLARE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
> +
> +static inline bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
> +                                       struct stack_info *info)
> +{
> +       unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
> +       unsigned long high = low + SDEI_STACK_SIZE;
> +
> +       return on_stack(sp, size, low, high, STACK_TYPE_SDEI_NORMAL, info);
> +}
> +
> +static inline bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
> +                                         struct stack_info *info)
> +{
> +       unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
> +       unsigned long high = low + SDEI_STACK_SIZE;
> +
> +       return on_stack(sp, size, low, high, STACK_TYPE_SDEI_CRITICAL, info);
> +}
> +#else
> +static inline bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
> +                                       struct stack_info *info)
> +{
> +       return false;
> +}
> +
> +static inline bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
> +                                         struct stack_info *info)
> +{
> +       return false;
> +}
>  #endif
>
>  #endif /* __ASM_STACKTRACE_H */
> diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
> index d20620a1c51a4..881eece3422ca 100644
> --- a/arch/arm64/kernel/sdei.c
> +++ b/arch/arm64/kernel/sdei.c
> @@ -31,9 +31,6 @@ unsigned long sdei_exit_mode;
>   * sdei stack.
>   * For now, we allocate stacks when the driver is probed.
>   */
> -DECLARE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
> -DECLARE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
> -
>  #ifdef CONFIG_VMAP_STACK
>  DEFINE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
>  DEFINE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
> @@ -162,38 +159,6 @@ static int init_sdei_scs(void)
>         return err;
>  }
>
> -static bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
> -                                struct stack_info *info)
> -{
> -       unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
> -       unsigned long high = low + SDEI_STACK_SIZE;
> -
> -       return on_stack(sp, size, low, high, STACK_TYPE_SDEI_NORMAL, info);
> -}
> -
> -static bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
> -                                  struct stack_info *info)
> -{
> -       unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
> -       unsigned long high = low + SDEI_STACK_SIZE;
> -
> -       return on_stack(sp, size, low, high, STACK_TYPE_SDEI_CRITICAL, info);
> -}
> -
> -bool _on_sdei_stack(unsigned long sp, unsigned long size, struct stack_info *info)
> -{
> -       if (!IS_ENABLED(CONFIG_VMAP_STACK))
> -               return false;
> -
> -       if (on_sdei_critical_stack(sp, size, info))
> -               return true;
> -
> -       if (on_sdei_normal_stack(sp, size, info))
> -               return true;
> -
> -       return false;
> -}
> -
>  unsigned long sdei_arch_get_entry_point(int conduit)
>  {
>         /*
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 4c8865e495fea..04a9b56b114c1 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -86,8 +86,17 @@ static bool on_accessible_stack(const struct task_struct *tsk,
>                 return true;
>         if (on_overflow_stack(sp, size, info))
>                 return true;
> -       if (on_sdei_stack(sp, size, info))
> -               return true;
> +
> +       if (IS_ENABLED(CONFIG_VMAP_STACK) &&
> +           IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) &&
> +           in_nmi())

I think we can remove the IS_ENABLED() checks since it's handled by
ifdefs in asm/stacktrace.h

Otherwise, Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

> +       {
> +               if (on_sdei_critical_stack(sp, size, info))
> +                       return true;
> +
> +               if (on_sdei_normal_stack(sp, size, info))
> +                       return true;
> +       }
>
>         return false;
>  }
> --
> 2.30.2
>

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 4/8] arm64: stacktrace: add stackinfo_on_stack() helper
  2022-08-01 12:12 ` [PATCH 4/8] arm64: stacktrace: add stackinfo_on_stack() helper Mark Rutland
  2022-08-01 15:56   ` Mark Brown
@ 2022-08-02  5:00   ` Kalesh Singh
  1 sibling, 0 replies; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02  5:00 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Factor the core predicate out of on_stack() into a helper which can be
> used on a pre-populated stack_info.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>

Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

> ---
>  arch/arm64/include/asm/stacktrace/common.h | 29 ++++++++++++++++------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 676002d7d333c..9ed7feb493a36 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -66,21 +66,34 @@ struct unwind_state {
>         struct task_struct *task;
>  };
>
> +static inline bool stackinfo_on_stack(const struct stack_info *info,
> +                                     unsigned long sp, unsigned long size)
> +{
> +       if (!info->low)
> +               return false;
> +
> +       if (sp < info->low || sp + size < sp || sp + size > info->high)
> +               return false;
> +
> +       return true;
> +}
> +
>  static inline bool on_stack(unsigned long sp, unsigned long size,
>                             unsigned long low, unsigned long high,
>                             enum stack_type type, struct stack_info *info)
>  {
> -       if (!low)
> -               return false;
> +       struct stack_info tmp = {
> +               .low = low,
> +               .high = high,
> +               .type = type,
> +       };
>
> -       if (sp < low || sp + size < sp || sp + size > high)
> +       if (!stackinfo_on_stack(&tmp, sp, size))
>                 return false;
>
> -       if (info) {
> -               info->low = low;
> -               info->high = high;
> -               info->type = type;
> -       }
> +       if (info)
> +               *info = tmp;
> +
>         return true;
>  }
>
> --
> 2.30.2
>

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery
  2022-08-01 12:12 ` [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery Mark Rutland
  2022-08-01 17:14   ` Mark Brown
@ 2022-08-02  5:22   ` Kalesh Singh
  2022-08-02 12:29     ` Mark Rutland
  1 sibling, 1 reply; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02  5:22 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> In subsequent patches we'll want to acquire the stack boundaries
> ahead-of-time, and we'll need to be able to acquire the relevant
> stack_info regardless of whether we have an object the happens to be on
> the stack.
>
> This patch replaces the on_XXX_stack() helpers with stackinfo_get_XXX()
> helpers, with the caller being responsible for the checking whether an
> object is on a relevant stack. For the moment this is moved into the
> on_accessible_stack() functions, making these slightly larger;
> subsequent patches will remove the on_accessible_stack() functions and
> simplify the logic.
>
> The on_irq_stack() and on_task_stack() helpers are kept as these are
> used by IRQ entry sequences and stackleak respectively. As they're only
> used as predicates, the stack_info pointer parameter is removed in both
> cases.
>
> As the on_accessible_stack() functions are always passed a non-NULL info
> pointer, these now update info unconditionally. When updating the type
> to STACK_TYPE_UNKNOWN, the low/high bounds are also modified, but as
> these will not be consumed this should have no adverse affect.
>
> There should be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/processor.h         |  2 +-
>  arch/arm64/include/asm/stacktrace.h        | 78 +++++++++++++---------
>  arch/arm64/include/asm/stacktrace/common.h | 28 +++-----
>  arch/arm64/kernel/ptrace.c                 |  2 +-
>  arch/arm64/kernel/stacktrace.c             | 67 ++++++++++++-------
>  arch/arm64/kvm/hyp/nvhe/stacktrace.c       | 37 +++++++---
>  arch/arm64/kvm/stacktrace.c                | 39 ++++++++---
>  7 files changed, 154 insertions(+), 99 deletions(-)
>
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 9e58749db21df..5035e0394a8a0 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -409,7 +409,7 @@ long get_tagged_addr_ctrl(struct task_struct *task);
>   * The top of the current task's task stack
>   */
>  #define current_top_of_stack() ((unsigned long)current->stack + THREAD_SIZE)
> -#define on_thread_stack()      (on_task_stack(current, current_stack_pointer, 1, NULL))
> +#define on_thread_stack()      (on_task_stack(current, current_stack_pointer, 1))
>
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_PROCESSOR_H */
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index fa2df1ea22ebc..aad0c6258721d 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -22,77 +22,91 @@ extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>
>  DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
>
> -static inline bool on_irq_stack(unsigned long sp, unsigned long size,
> -                               struct stack_info *info)
> +static inline struct stack_info stackinfo_get_irq(void)
>  {
>         unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
>         unsigned long high = low + IRQ_STACK_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_IRQ, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_IRQ,
> +       };
>  }
>
> -static inline bool on_task_stack(const struct task_struct *tsk,
> -                                unsigned long sp, unsigned long size,
> -                                struct stack_info *info)
> +static inline bool on_irq_stack(unsigned long sp, unsigned long size)
> +{
> +       struct stack_info info = stackinfo_get_irq();
> +       return stackinfo_on_stack(&info, sp, size);
> +}
> +
> +static inline struct stack_info stackinfo_get_task(const struct task_struct *tsk)
>  {
>         unsigned long low = (unsigned long)task_stack_page(tsk);
>         unsigned long high = low + THREAD_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_TASK, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_TASK,
> +       };
> +}
> +
> +static inline bool on_task_stack(const struct task_struct *tsk,
> +                                unsigned long sp, unsigned long size)
> +{
> +       struct stack_info info = stackinfo_get_task(tsk);
> +       return stackinfo_on_stack(&info, sp, size);
>  }
>
>  #ifdef CONFIG_VMAP_STACK
>  DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
>
> -static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
> -                               struct stack_info *info)
> +static inline struct stack_info stackinfo_get_overflow(void)
>  {
>         unsigned long low = (unsigned long)raw_cpu_ptr(overflow_stack);
>         unsigned long high = low + OVERFLOW_STACK_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_OVERFLOW, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_OVERFLOW,
> +       };
>  }
>  #else
> -static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
> -                                    struct stack_info *info)
> -{
> -       return false;
> -}
> +#define stackinfo_get_overflow()       stackinfo_get_unknown()
>  #endif
>
>  #if defined(CONFIG_ARM_SDE_INTERFACE) && defined(CONFIG_VMAP_STACK)
>  DECLARE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
>  DECLARE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
>
> -static inline bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
> -                                       struct stack_info *info)
> +static inline struct stack_info stackinfo_get_sdei_normal(void)
>  {
>         unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
>         unsigned long high = low + SDEI_STACK_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_SDEI_NORMAL, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_SDEI_NORMAL,
> +       };
>  }
>
> -static inline bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
> -                                         struct stack_info *info)
> +static inline struct stack_info stackinfo_get_sdei_critical(void)
>  {
>         unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
>         unsigned long high = low + SDEI_STACK_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_SDEI_CRITICAL, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_SDEI_CRITICAL,
> +       };
>  }
>  #else
> -static inline bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
> -                                       struct stack_info *info)
> -{
> -       return false;
> -}
> -
> -static inline bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
> -                                         struct stack_info *info)
> -{
> -       return false;
> -}
> +#define stackinfo_get_sdei_normal()    stackinfo_get_unknown()
> +#define stackinfo_get_sdei_critical()  stackinfo_get_unknown()
>  #endif
>
>  #endif /* __ASM_STACKTRACE_H */
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 9ed7feb493a36..0071f2459c703 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -66,6 +66,15 @@ struct unwind_state {
>         struct task_struct *task;
>  };
>
> +static inline struct stack_info stackinfo_get_unknown(void)
> +{
> +       return (struct stack_info) {
> +               .low = 0,
> +               .high = 0,
> +               .type = STACK_TYPE_UNKNOWN,
> +       };
> +}
> +
>  static inline bool stackinfo_on_stack(const struct stack_info *info,
>                                       unsigned long sp, unsigned long size)
>  {
> @@ -78,25 +87,6 @@ static inline bool stackinfo_on_stack(const struct stack_info *info,
>         return true;
>  }
>
> -static inline bool on_stack(unsigned long sp, unsigned long size,
> -                           unsigned long low, unsigned long high,
> -                           enum stack_type type, struct stack_info *info)
> -{
> -       struct stack_info tmp = {
> -               .low = low,
> -               .high = high,
> -               .type = type,
> -       };
> -
> -       if (!stackinfo_on_stack(&tmp, sp, size))
> -               return false;
> -
> -       if (info)
> -               *info = tmp;
> -
> -       return true;
> -}
> -
>  static inline void unwind_init_common(struct unwind_state *state,
>                                       struct task_struct *task)
>  {
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 21da83187a602..2e1b721497794 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -121,7 +121,7 @@ static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
>  {
>         return ((addr & ~(THREAD_SIZE - 1))  ==
>                 (kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1))) ||
> -               on_irq_stack(addr, sizeof(unsigned long), NULL);
> +               on_irq_stack(addr, sizeof(unsigned long));
>  }
>
>  /**
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 04a9b56b114c1..ca56fd732c2a9 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -67,38 +67,55 @@ static inline void unwind_init_from_task(struct unwind_state *state,
>         state->pc = thread_saved_pc(task);
>  }
>
> -/*
> - * We can only safely access per-cpu stacks from current in a non-preemptible
> - * context.
> - */
>  static bool on_accessible_stack(const struct task_struct *tsk,
>                                 unsigned long sp, unsigned long size,
>                                 struct stack_info *info)
>  {
> -       if (info)
> -               info->type = STACK_TYPE_UNKNOWN;
> +       struct stack_info tmp;
>
> -       if (on_task_stack(tsk, sp, size, info))
> -               return true;
> -       if (tsk != current || preemptible())
> -               return false;
> -       if (on_irq_stack(sp, size, info))
> -               return true;
> -       if (on_overflow_stack(sp, size, info))
> -               return true;
> -
> -       if (IS_ENABLED(CONFIG_VMAP_STACK) &&
> -           IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) &&
> -           in_nmi())
> -       {
> -               if (on_sdei_critical_stack(sp, size, info))
> -                       return true;
> -
> -               if (on_sdei_normal_stack(sp, size, info))
> -                       return true;
> -       }
> +       tmp = stackinfo_get_task(tsk);
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
>
> +       /*
> +        * We can only safely access per-cpu stacks when unwinding the current
> +        * task in a non-preemptible context.
> +        */
> +       if (tsk != current || preemptible())
> +               goto not_found;
> +
> +       tmp = stackinfo_get_irq();
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
> +
> +       tmp = stackinfo_get_overflow();
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
> +
> +       /*
> +        * We can only safely access SDEI stacks which unwinding the current
> +        * task in an NMI context.
> +        */
> +       if (!IS_ENABLED(CONFIG_VMAP_STACK) ||
> +           !IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) ||
> +           !in_nmi())
> +               goto not_found;
> +
> +       tmp = stackinfo_get_sdei_normal();
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
> +
> +       tmp = stackinfo_get_sdei_critical();
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
> +
> +not_found:
> +       *info = stackinfo_get_unknown();
>         return false;
> +
> +found:
> +       *info = tmp;
> +       return true;
>  }
>
>  /*
> diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> index 579b46aa9a553..5da0d44f61b73 100644
> --- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> +++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> @@ -39,34 +39,51 @@ static void hyp_prepare_backtrace(unsigned long fp, unsigned long pc)
>
>  DEFINE_PER_CPU(unsigned long [NVHE_STACKTRACE_SIZE/sizeof(long)], pkvm_stacktrace);
>
> -static bool on_overflow_stack(unsigned long sp, unsigned long size,
> -                             struct stack_info *info)
> +static struct stack_info stackinfo_get_overflow(void)
>  {
>         unsigned long low = (unsigned long)this_cpu_ptr(overflow_stack);
>         unsigned long high = low + OVERFLOW_STACK_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_OVERFLOW, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_OVERFLOW,
> +       };
>  }
>
> -static bool on_hyp_stack(unsigned long sp, unsigned long size,
> -                             struct stack_info *info)
> +static struct stack_info stackinfo_get_hyp(void)
>  {
>         struct kvm_nvhe_init_params *params = this_cpu_ptr(&kvm_init_params);
>         unsigned long high = params->stack_hyp_va;
>         unsigned long low = high - PAGE_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_HYP, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_HYP,
> +       };
>  }
>
>  static bool on_accessible_stack(const struct task_struct *tsk,
>                                 unsigned long sp, unsigned long size,
>                                 struct stack_info *info)
>  {
> -       if (info)
> -               info->type = STACK_TYPE_UNKNOWN;
> +       struct stack_info tmp;
>
> -       return (on_overflow_stack(sp, size, info) ||
> -               on_hyp_stack(sp, size, info));
> +       tmp = stackinfo_get_overflow();
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
> +
> +       tmp = stackinfo_get_hyp();
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
> +
> +       *info = stackinfo_get_unknown();
> +       return false;
> +
> +found:
> +       *info = tmp;
> +       return true;
>  }
>
>  static int unwind_next(struct unwind_state *state)
> diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
> index b69c18a26567d..3b005530ac02a 100644
> --- a/arch/arm64/kvm/stacktrace.c
> +++ b/arch/arm64/kvm/stacktrace.c
> @@ -62,37 +62,54 @@ static bool kvm_nvhe_stack_kern_va(unsigned long *addr,
>         return true;
>  }
>
> -static bool on_overflow_stack(unsigned long sp, unsigned long size,
> -                             struct stack_info *info)
> +static struct stack_info stackinfo_get_overflow(void)
>  {
>         struct kvm_nvhe_stacktrace_info *stacktrace_info
>                                 = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
>         unsigned long low = (unsigned long)stacktrace_info->overflow_stack_base;
>         unsigned long high = low + OVERFLOW_STACK_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_OVERFLOW, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_OVERFLOW,
> +       };
>  }
>
> -static bool on_hyp_stack(unsigned long sp, unsigned long size,
> -                        struct stack_info *info)
> +static struct stack_info stackinfo_get_hyp(void)
>  {
>         struct kvm_nvhe_stacktrace_info *stacktrace_info
>                                 = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
>         unsigned long low = (unsigned long)stacktrace_info->stack_base;
>         unsigned long high = low + PAGE_SIZE;
>
> -       return on_stack(sp, size, low, high, STACK_TYPE_HYP, info);
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_HYP,
> +       };
>  }
>
>  static bool on_accessible_stack(const struct task_struct *tsk,
>                                 unsigned long sp, unsigned long size,
>                                 struct stack_info *info)
>  {
> -       if (info)
> -               info->type = STACK_TYPE_UNKNOWN;
> -
> -       return (on_overflow_stack(sp, size, info) ||
> -               on_hyp_stack(sp, size, info));
> +       struct stack_info tmp;
> +
> +       tmp = stackinfo_get_overflow();
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
> +
> +       tmp = stackinfo_get_hyp();
> +       if (stackinfo_on_stack(&tmp, sp, size))
> +               goto found;
> +

nit: Some trailing whitespaces in this function, checkpatch should catch it

Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

> +       *info = stackinfo_get_unknown();
> +       return false;
> +
> +found:
> +       *info = tmp;
> +       return true;
>  }
>
>  static int unwind_next(struct unwind_state *state)
> --
> 2.30.2
>

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 6/8] arm64: stacktrace: remove stack type from fp translator
  2022-08-01 12:12 ` [PATCH 6/8] arm64: stacktrace: remove stack type from fp translator Mark Rutland
@ 2022-08-02  5:32   ` Kalesh Singh
  2022-08-03 12:36   ` Mark Brown
  1 sibling, 0 replies; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02  5:32 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> In subsequent patches we'll remove the stack_type enum, and move the FP
> translation logic out of the raw FP unwind code.
>
> In preparation for doing so, this patch removes the type parameter from
> the FP translation callback, and modifies kvm_nvhe_stack_kern_va() to
> determine the relevant stack directly.
>
> So that kvm_nvhe_stack_kern_va() can use the stackinfo_*() helpers,
> these are moved earlier in the file, but are not modified in any way.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>

Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

> ---
>  arch/arm64/include/asm/stacktrace/common.h |  6 +-
>  arch/arm64/kvm/stacktrace.c                | 78 ++++++++++++----------
>  2 files changed, 43 insertions(+), 41 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 0071f2459c703..0bd9d7ad295e0 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -114,13 +114,11 @@ static inline void unwind_init_common(struct unwind_state *state,
>   * a kernel address.
>   *
>   * @fp:   the frame pointer to be updated to its kernel address.
> - * @type: the stack type associated with frame pointer @fp
>   *
>   * Returns true and success and @fp is updated to the corresponding
>   * kernel virtual address; otherwise returns false.
>   */
> -typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp,
> -                                           enum stack_type type);
> +typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp);
>
>  /*
>   * on_accessible_stack_fn() - Check whether a stack range is on any
> @@ -165,7 +163,7 @@ unwind_next_frame_record(struct unwind_state *state,
>          * If fp is not from the current address space perform the necessary
>          * translation before dereferencing it to get the next fp.
>          */
> -       if (translate_fp && !translate_fp(&kern_fp, info.type))
> +       if (translate_fp && !translate_fp(&kern_fp))
>                 return -EINVAL;
>
>         /*
> diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
> index 3b005530ac02a..62ffa2b40da10 100644
> --- a/arch/arm64/kvm/stacktrace.c
> +++ b/arch/arm64/kvm/stacktrace.c
> @@ -21,6 +21,34 @@
>
>  #include <asm/stacktrace/nvhe.h>
>
> +static struct stack_info stackinfo_get_overflow(void)
> +{
> +       struct kvm_nvhe_stacktrace_info *stacktrace_info
> +                               = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
> +       unsigned long low = (unsigned long)stacktrace_info->overflow_stack_base;
> +       unsigned long high = low + OVERFLOW_STACK_SIZE;
> +
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_OVERFLOW,
> +       };
> +}
> +
> +static struct stack_info stackinfo_get_hyp(void)
> +{
> +       struct kvm_nvhe_stacktrace_info *stacktrace_info
> +                               = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
> +       unsigned long low = (unsigned long)stacktrace_info->stack_base;
> +       unsigned long high = low + PAGE_SIZE;
> +
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +               .type = STACK_TYPE_HYP,
> +       };
> +}
> +
>  /*
>   * kvm_nvhe_stack_kern_va - Convert KVM nVHE HYP stack addresses to a kernel VAs
>   *
> @@ -34,27 +62,31 @@
>   * Returns true on success and updates @addr to its corresponding kernel VA;
>   * otherwise returns false.
>   */
> -static bool kvm_nvhe_stack_kern_va(unsigned long *addr,
> -                                  enum stack_type type)
> +static bool kvm_nvhe_stack_kern_va(unsigned long *addr)
>  {
>         struct kvm_nvhe_stacktrace_info *stacktrace_info;
>         unsigned long hyp_base, kern_base, hyp_offset;
> +       struct stack_info stack;
>
>         stacktrace_info = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
>
> -       switch (type) {
> -       case STACK_TYPE_HYP:
> +       stack = stackinfo_get_hyp();
> +       if (stackinfo_on_stack(&stack, *addr, 1)) {
>                 kern_base = (unsigned long)*this_cpu_ptr(&kvm_arm_hyp_stack_page);
>                 hyp_base = (unsigned long)stacktrace_info->stack_base;
> -               break;
> -       case STACK_TYPE_OVERFLOW:
> +               goto found;
> +       }
> +
> +       stack = stackinfo_get_overflow();
> +       if (stackinfo_on_stack(&stack, *addr, 1)) {
>                 kern_base = (unsigned long)this_cpu_ptr_nvhe_sym(overflow_stack);
>                 hyp_base = (unsigned long)stacktrace_info->overflow_stack_base;
> -               break;
> -       default:
> -               return false;
> +               goto found;
>         }
>
> +       return false;
> +
> +found:
>         hyp_offset = *addr - hyp_base;
>
>         *addr = kern_base + hyp_offset;
> @@ -62,34 +94,6 @@ static bool kvm_nvhe_stack_kern_va(unsigned long *addr,
>         return true;
>  }
>
> -static struct stack_info stackinfo_get_overflow(void)
> -{
> -       struct kvm_nvhe_stacktrace_info *stacktrace_info
> -                               = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
> -       unsigned long low = (unsigned long)stacktrace_info->overflow_stack_base;
> -       unsigned long high = low + OVERFLOW_STACK_SIZE;
> -
> -       return (struct stack_info) {
> -               .low = low,
> -               .high = high,
> -               .type = STACK_TYPE_OVERFLOW,
> -       };
> -}
> -
> -static struct stack_info stackinfo_get_hyp(void)
> -{
> -       struct kvm_nvhe_stacktrace_info *stacktrace_info
> -                               = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
> -       unsigned long low = (unsigned long)stacktrace_info->stack_base;
> -       unsigned long high = low + PAGE_SIZE;
> -
> -       return (struct stack_info) {
> -               .low = low,
> -               .high = high,
> -               .type = STACK_TYPE_HYP,
> -       };
> -}
> -
>  static bool on_accessible_stack(const struct task_struct *tsk,
>                                 unsigned long sp, unsigned long size,
>                                 struct stack_info *info)
> --
> 2.30.2
>

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code
  2022-08-02  4:53   ` Kalesh Singh
@ 2022-08-02 12:26     ` Mark Rutland
  2022-08-02 16:31       ` Kalesh Singh
  0 siblings, 1 reply; 28+ messages in thread
From: Mark Rutland @ 2022-08-02 12:26 UTC (permalink / raw)
  To: Kalesh Singh
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

Hi Kalesh,

On Mon, Aug 01, 2022 at 09:53:35PM -0700, Kalesh Singh wrote:
> On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > index 4c8865e495fea..04a9b56b114c1 100644
> > --- a/arch/arm64/kernel/stacktrace.c
> > +++ b/arch/arm64/kernel/stacktrace.c
> > @@ -86,8 +86,17 @@ static bool on_accessible_stack(const struct task_struct *tsk,
> >                 return true;
> >         if (on_overflow_stack(sp, size, info))
> >                 return true;
> > -       if (on_sdei_stack(sp, size, info))
> > -               return true;
> > +
> > +       if (IS_ENABLED(CONFIG_VMAP_STACK) &&
> > +           IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) &&
> > +           in_nmi())
> 
> I think we can remove the IS_ENABLED() checks since it's handled by
> ifdefs in asm/stacktrace.h
> 
> Otherwise, Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

I'd kept the IS_ENABLED() checks here to avoid code being generated of in_nmi()
when SDEI is not configured in. Since in_nmi() uses preempt_count() and that
uses READ_ONCE(), the compiler can't optimize the read away in case there are
side effects (but I imagine will discard the value immediately).

With that in mind, are you happy if I leave this as is, and take your
Reviewed-by?

Thanks,
Mark.

> 
> > +       {
> > +               if (on_sdei_critical_stack(sp, size, info))
> > +                       return true;
> > +
> > +               if (on_sdei_normal_stack(sp, size, info))
> > +                       return true;
> > +       }
> >
> >         return false;
> >  }
> > --
> > 2.30.2
> >

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery
  2022-08-02  5:22   ` Kalesh Singh
@ 2022-08-02 12:29     ` Mark Rutland
  0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-02 12:29 UTC (permalink / raw)
  To: Kalesh Singh
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 01, 2022 at 10:22:45PM -0700, Kalesh Singh wrote:
> On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >  static bool on_accessible_stack(const struct task_struct *tsk,
> >                                 unsigned long sp, unsigned long size,
> >                                 struct stack_info *info)
> >  {
> > -       if (info)
> > -               info->type = STACK_TYPE_UNKNOWN;
> > -
> > -       return (on_overflow_stack(sp, size, info) ||
> > -               on_hyp_stack(sp, size, info));
> > +       struct stack_info tmp;
> > +
> > +       tmp = stackinfo_get_overflow();
> > +       if (stackinfo_on_stack(&tmp, sp, size))
> > +               goto found;
> > +
> > +       tmp = stackinfo_get_hyp();
> > +       if (stackinfo_on_stack(&tmp, sp, size))
> > +               goto found;
> > +
> 
> nit: Some trailing whitespaces in this function, checkpatch should catch it

Whoops; I'll go throug the series and double-check for any more. Thanks for the
heads up!

> Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

Thanks!

Mark.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 7/8] arm64: stacktrace: track all stack boundaries explicitly
  2022-08-01 12:12 ` [PATCH 7/8] arm64: stacktrace: track all stack boundaries explicitly Mark Rutland
@ 2022-08-02 16:05   ` Kalesh Singh
  0 siblings, 0 replies; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02 16:05 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Currently we call an on_accessible_stack() callback for each step of the
> unwinder, requiring redundant work to be performed in the core of the
> unwind loop (e.g. disabling preemption around accesses to per-cpu
> variables containing stack boundaries). To prevent unwind loops which go
> through a stack multiple times, we have to track the set of unwound
> stacks, requiring a stack_type enum which needs to cater for all the
> stacks of all possible callees. To prevent loops within a stack, we must
> track the prior FP values.
>
> This patch reworks the unwinder to minimize the work in the core of the
> unwinder, and to remove the need for the stack_type enum. The set of
> accessible stacks (and their boundaries) are determined at the start of
> the unwind, and the current stack is tracked during the unwind, with
> completed stacks removed from the set of accessible stacks. This makes
> the boundary checks more accurate (e.g. detecting overlapped frame
> records), and removes the need for separate tracking of the prior FP and
> visited stacks.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>

Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

> ---
>  arch/arm64/include/asm/stacktrace.h        |   5 -
>  arch/arm64/include/asm/stacktrace/common.h | 158 ++++++++++-----------
>  arch/arm64/kernel/stacktrace.c             |  91 +++++-------
>  arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  35 ++---
>  arch/arm64/kvm/stacktrace.c                |  36 ++---
>  5 files changed, 131 insertions(+), 194 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index aad0c6258721d..5a0edb064ea47 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -30,7 +30,6 @@ static inline struct stack_info stackinfo_get_irq(void)
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_IRQ,
>         };
>  }
>
> @@ -48,7 +47,6 @@ static inline struct stack_info stackinfo_get_task(const struct task_struct *tsk
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_TASK,
>         };
>  }
>
> @@ -70,7 +68,6 @@ static inline struct stack_info stackinfo_get_overflow(void)
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_OVERFLOW,
>         };
>  }
>  #else
> @@ -89,7 +86,6 @@ static inline struct stack_info stackinfo_get_sdei_normal(void)
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_SDEI_NORMAL,
>         };
>  }
>
> @@ -101,7 +97,6 @@ static inline struct stack_info stackinfo_get_sdei_critical(void)
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_SDEI_CRITICAL,
>         };
>  }
>  #else
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 0bd9d7ad295e0..c594f332bb946 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -9,26 +9,12 @@
>  #ifndef __ASM_STACKTRACE_COMMON_H
>  #define __ASM_STACKTRACE_COMMON_H
>
> -#include <linux/bitmap.h>
> -#include <linux/bitops.h>
>  #include <linux/kprobes.h>
>  #include <linux/types.h>
>
> -enum stack_type {
> -       STACK_TYPE_UNKNOWN,
> -       STACK_TYPE_TASK,
> -       STACK_TYPE_IRQ,
> -       STACK_TYPE_OVERFLOW,
> -       STACK_TYPE_SDEI_NORMAL,
> -       STACK_TYPE_SDEI_CRITICAL,
> -       STACK_TYPE_HYP,
> -       __NR_STACK_TYPES
> -};
> -
>  struct stack_info {
>         unsigned long low;
>         unsigned long high;
> -       enum stack_type type;
>  };
>
>  /*
> @@ -38,32 +24,27 @@ struct stack_info {
>   * @fp:          The fp value in the frame record (or the real fp)
>   * @pc:          The lr value in the frame record (or the real lr)
>   *
> - * @stacks_done: Stacks which have been entirely unwound, for which it is no
> - *               longer valid to unwind to.
> - *
> - * @prev_fp:     The fp that pointed to this frame record, or a synthetic value
> - *               of 0. This is used to ensure that within a stack, each
> - *               subsequent frame record is at an increasing address.
> - * @prev_type:   The type of stack this frame record was on, or a synthetic
> - *               value of STACK_TYPE_UNKNOWN. This is used to detect a
> - *               transition from one stack to another.
> - *
>   * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
>   *               associated with the most recently encountered replacement lr
>   *               value.
>   *
>   * @task:        The task being unwound.
> + *
> + * @stack:       The stack currently being unwound.
> + * @stacks:      An array of stacks which can be unwound.
> + * @nr_stacks:   The number of stacks in @stacks.
>   */
>  struct unwind_state {
>         unsigned long fp;
>         unsigned long pc;
> -       DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> -       unsigned long prev_fp;
> -       enum stack_type prev_type;
>  #ifdef CONFIG_KRETPROBES
>         struct llist_node *kr_cur;
>  #endif
>         struct task_struct *task;
> +
> +       struct stack_info stack;
> +       struct stack_info *stacks;
> +       int nr_stacks;
>  };
>
>  static inline struct stack_info stackinfo_get_unknown(void)
> @@ -71,7 +52,6 @@ static inline struct stack_info stackinfo_get_unknown(void)
>         return (struct stack_info) {
>                 .low = 0,
>                 .high = 0,
> -               .type = STACK_TYPE_UNKNOWN,
>         };
>  }
>
> @@ -95,18 +75,7 @@ static inline void unwind_init_common(struct unwind_state *state,
>         state->kr_cur = NULL;
>  #endif
>
> -       /*
> -        * Prime the first unwind.
> -        *
> -        * In unwind_next() we'll check that the FP points to a valid stack,
> -        * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
> -        * treated as a transition to whichever stack that happens to be. The
> -        * prev_fp value won't be used, but we set it to 0 such that it is
> -        * definitely not an accessible stack address.
> -        */
> -       bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
> -       state->prev_fp = 0;
> -       state->prev_type = STACK_TYPE_UNKNOWN;
> +       state->stack = stackinfo_get_unknown();
>  }
>
>  /*
> @@ -120,44 +89,91 @@ static inline void unwind_init_common(struct unwind_state *state,
>   */
>  typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp);
>
> +static struct stack_info *unwind_find_next_stack(const struct unwind_state *state,
> +                                                unsigned long sp,
> +                                                unsigned long size)
> +{
> +       for (int i = 0; i < state->nr_stacks; i++) {
> +               struct stack_info *info = &state->stacks[i];
> +
> +               if (stackinfo_on_stack(info, sp, size))
> +                       return info;
> +       }
> +
> +       return NULL;
> +}
> +
>  /*
> - * on_accessible_stack_fn() - Check whether a stack range is on any
> - * of the possible stacks.
> + * unwind_consume_stack - Check if an object is on an accessible stack,
> + * updating stack boundaries so that future unwind steps cannot consume this
> + * object again.
>   *
> - * @tsk:  task whose stack is being unwound
> - * @sp:   stack address being checked
> - * @size: size of the stack range being checked
> - * @info: stack unwinding context
> + * @state: the current unwind state.
> + * @sp:    the base address of the object.
> + * @size:  the size of the object.
>   */
> -typedef bool (*on_accessible_stack_fn)(const struct task_struct *tsk,
> -                                      unsigned long sp, unsigned long size,
> -                                      struct stack_info *info);
> +static inline int unwind_consume_stack(struct unwind_state *state,
> +                                      unsigned long sp,
> +                                      unsigned long size)
> +{
> +       struct stack_info *next;
> +
> +       if (stackinfo_on_stack(&state->stack, sp, size))
> +               goto found;
> +
> +       next = unwind_find_next_stack(state, sp, size);
> +       if (!next)
> +               return -EINVAL;
> +
> +       /*
> +        * Stack transitions are strictly one-way, and once we've
> +        * transitioned from one stack to another, it's never valid to
> +        * unwind back to the old stack.
> +        *
> +        * Remove the current stack from the list of stacks so that it cannot
> +        * be found on a subsequent transition.
> +        *
> +        * Note that stacks can nest in several valid orders, e.g.
> +        *
> +        *   TASK -> IRQ -> OVERFLOW -> SDEI_NORMAL
> +        *   TASK -> SDEI_NORMAL -> SDEI_CRITICAL -> OVERFLOW
> +        *   HYP -> OVERFLOW
> +        *
> +        * ... so we do not check the specific order of stack
> +        * transitions.
> +        */
> +       state->stack = *next;
> +       *next = stackinfo_get_unknown();
> +
> +found:
> +       /*
> +        * Future unwind steps can only consume stack above this frame record.
> +        * Update the current stack to start immediately above it.
> +        */
> +       state->stack.low = sp + size;
> +       return 0;
> +}
>
>  /*
>   * unwind_next_frame_record() - Unwind to the next frame record indicated by
>   * @state->fp.
>   *
>   * @state:        the current unwind state.
> - * @accessible:   determines whether the frame record is accessible
>   * @translate_fp: translates the fp prior to access (may be NULL)
>   */
>  static inline int
>  unwind_next_frame_record(struct unwind_state *state,
> -                        on_accessible_stack_fn accessible,
>                          stack_trace_translate_fp_fn translate_fp)
>  {
> -       struct stack_info info;
>         unsigned long fp = state->fp, kern_fp = fp;
> -       struct task_struct *tsk = state->task;
> +       int err;
>
>         if (fp & 0x7)
>                 return -EINVAL;
>
> -       if (!accessible(tsk, fp, 16, &info))
> -               return -EINVAL;
> -
> -       if (test_bit(info.type, state->stacks_done))
> -               return -EINVAL;
> +       err = unwind_consume_stack(state, fp, 16);
> +       if (err)
> +               return err;
>
>         /*
>          * If fp is not from the current address space perform the necessary
> @@ -167,34 +183,10 @@ unwind_next_frame_record(struct unwind_state *state,
>                 return -EINVAL;
>
>         /*
> -        * As stacks grow downward, any valid record on the same stack must be
> -        * at a strictly higher address than the prior record.
> -        *
> -        * Stacks can nest in several valid orders, e.g.
> -        *
> -        * TASK -> IRQ -> OVERFLOW -> SDEI_NORMAL
> -        * TASK -> SDEI_NORMAL -> SDEI_CRITICAL -> OVERFLOW
> -        * HYP -> OVERFLOW
> -        *
> -        * ... but the nesting itself is strict. Once we transition from one
> -        * stack to another, it's never valid to unwind back to that first
> -        * stack.
> -        */
> -       if (info.type == state->prev_type) {
> -               if (fp <= state->prev_fp)
> -                       return -EINVAL;
> -       } else {
> -               __set_bit(state->prev_type, state->stacks_done);
> -       }
> -
> -       /*
> -        * Record this frame record's values and location. The prev_fp and
> -        * prev_type are only meaningful to the next unwind_next() invocation.
> +        * Record this frame record's values.
>          */
>         state->fp = READ_ONCE(*(unsigned long *)(kern_fp));
>         state->pc = READ_ONCE(*(unsigned long *)(kern_fp + 8));
> -       state->prev_fp = fp;
> -       state->prev_type = info.type;
>
>         return 0;
>  }
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index ca56fd732c2a9..1133be3ff774d 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -67,57 +67,6 @@ static inline void unwind_init_from_task(struct unwind_state *state,
>         state->pc = thread_saved_pc(task);
>  }
>
> -static bool on_accessible_stack(const struct task_struct *tsk,
> -                               unsigned long sp, unsigned long size,
> -                               struct stack_info *info)
> -{
> -       struct stack_info tmp;
> -
> -       tmp = stackinfo_get_task(tsk);
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -       /*
> -        * We can only safely access per-cpu stacks when unwinding the current
> -        * task in a non-preemptible context.
> -        */
> -       if (tsk != current || preemptible())
> -               goto not_found;
> -
> -       tmp = stackinfo_get_irq();
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -       tmp = stackinfo_get_overflow();
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -       /*
> -        * We can only safely access SDEI stacks which unwinding the current
> -        * task in an NMI context.
> -        */
> -       if (!IS_ENABLED(CONFIG_VMAP_STACK) ||
> -           !IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) ||
> -           !in_nmi())
> -               goto not_found;
> -
> -       tmp = stackinfo_get_sdei_normal();
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -       tmp = stackinfo_get_sdei_critical();
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -not_found:
> -       *info = stackinfo_get_unknown();
> -       return false;
> -
> -found:
> -       *info = tmp;
> -       return true;
> -}
> -
>  /*
>   * Unwind from one frame record (A) to the next frame record (B).
>   *
> @@ -135,7 +84,7 @@ static int notrace unwind_next(struct unwind_state *state)
>         if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
>                 return -ENOENT;
>
> -       err = unwind_next_frame_record(state, on_accessible_stack, NULL);
> +       err = unwind_next_frame_record(state, NULL);
>         if (err)
>                 return err;
>
> @@ -215,11 +164,47 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl)
>         barrier();
>  }
>
> +/*
> + * Per-cpu stacks are only accessible when unwinding the current task in a
> + * non-preemptible context.
> + */
> +#define STACKINFO_CPU(name)                                    \
> +       ({                                                      \
> +               ((task == current) && !preemptible())           \
> +                       ? stackinfo_get_##name()                \
> +                       : stackinfo_get_unknown();              \
> +       })
> +
> +/*
> + * SDEI stacks are only accessible when unwinding the current task in an NMI
> + * context.
> + */
> +#define STACKINFO_SDEI(name)                                   \
> +       ({                                                      \
> +               ((task == current) && in_nmi())                 \
> +                       ? stackinfo_get_sdei_##name()           \
> +                       : stackinfo_get_unknown();              \
> +       })
> +
>  noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>                               void *cookie, struct task_struct *task,
>                               struct pt_regs *regs)
>  {
> -       struct unwind_state state;
> +       struct stack_info stacks[] = {
> +               stackinfo_get_task(task),
> +               STACKINFO_CPU(irq),
> +#if defined (CONFIG_VMAP_STACK)
> +               STACKINFO_CPU(overflow),
> +#endif
> +#if defined(CONFIG_VMAP_STACK) && defined(CONFIG_ARM_SDE_INTERFACE)
> +               STACKINFO_SDEI(normal),
> +               STACKINFO_SDEI(critical),
> +#endif
> +       };
> +       struct unwind_state state = {
> +               .stacks = stacks,
> +               .nr_stacks = ARRAY_SIZE(stacks),
> +       };
>
>         if (regs) {
>                 if (task != current)
> diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> index 5da0d44f61b73..08e1325ead73f 100644
> --- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> +++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> @@ -47,7 +47,6 @@ static struct stack_info stackinfo_get_overflow(void)
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_OVERFLOW,
>         };
>  }
>
> @@ -60,35 +59,12 @@ static struct stack_info stackinfo_get_hyp(void)
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_HYP,
>         };
>  }
>
> -static bool on_accessible_stack(const struct task_struct *tsk,
> -                               unsigned long sp, unsigned long size,
> -                               struct stack_info *info)
> -{
> -       struct stack_info tmp;
> -
> -       tmp = stackinfo_get_overflow();
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -       tmp = stackinfo_get_hyp();
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -       *info = stackinfo_get_unknown();
> -       return false;
> -
> -found:
> -       *info = tmp;
> -       return true;
> -}
> -
>  static int unwind_next(struct unwind_state *state)
>  {
> -       return unwind_next_frame_record(state, on_accessible_stack, NULL);
> +       return unwind_next_frame_record(state, NULL);
>  }
>
>  static void notrace unwind(struct unwind_state *state,
> @@ -144,7 +120,14 @@ static bool pkvm_save_backtrace_entry(void *arg, unsigned long where)
>   */
>  static void pkvm_save_backtrace(unsigned long fp, unsigned long pc)
>  {
> -       struct unwind_state state;
> +       struct stack_info stacks[] = {
> +               stackinfo_get_overflow(),
> +               stackinfo_get_hyp(),
> +       };
> +       struct unwind_state state = {
> +               .stacks = stacks,
> +               .nr_stacks = ARRAY_SIZE(stacks),
> +       };
>         int idx = 0;
>
>         kvm_nvhe_unwind_init(&state, fp, pc);
> diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
> index 62ffa2b40da10..8295e132da2f0 100644
> --- a/arch/arm64/kvm/stacktrace.c
> +++ b/arch/arm64/kvm/stacktrace.c
> @@ -31,7 +31,6 @@ static struct stack_info stackinfo_get_overflow(void)
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_OVERFLOW,
>         };
>  }
>
> @@ -45,7 +44,6 @@ static struct stack_info stackinfo_get_hyp(void)
>         return (struct stack_info) {
>                 .low = low,
>                 .high = high,
> -               .type = STACK_TYPE_HYP,
>         };
>  }
>
> @@ -94,32 +92,9 @@ static bool kvm_nvhe_stack_kern_va(unsigned long *addr)
>         return true;
>  }
>
> -static bool on_accessible_stack(const struct task_struct *tsk,
> -                               unsigned long sp, unsigned long size,
> -                               struct stack_info *info)
> -{
> -       struct stack_info tmp;
> -
> -       tmp = stackinfo_get_overflow();
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -       tmp = stackinfo_get_hyp();
> -       if (stackinfo_on_stack(&tmp, sp, size))
> -               goto found;
> -
> -       *info = stackinfo_get_unknown();
> -       return false;
> -
> -found:
> -       *info = tmp;
> -       return true;
> -}
> -
>  static int unwind_next(struct unwind_state *state)
>  {
> -       return unwind_next_frame_record(state, on_accessible_stack,
> -                                       kvm_nvhe_stack_kern_va);
> +       return unwind_next_frame_record(state, kvm_nvhe_stack_kern_va);
>  }
>
>  static void unwind(struct unwind_state *state,
> @@ -177,7 +152,14 @@ static void kvm_nvhe_dump_backtrace_end(void)
>  static void hyp_dump_backtrace(unsigned long hyp_offset)
>  {
>         struct kvm_nvhe_stacktrace_info *stacktrace_info;
> -       struct unwind_state state;
> +       struct stack_info stacks[] = {
> +               stackinfo_get_overflow(),
> +               stackinfo_get_hyp(),
> +       };
> +       struct unwind_state state = {
> +               .stacks = stacks,
> +               .nr_stacks = ARRAY_SIZE(stacks),
> +       };
>
>         stacktrace_info = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
>
> --
> 2.30.2
>

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 8/8] arm64: stacktrace: track hyp stacks in unwinder's address space
  2022-08-01 12:12 ` [PATCH 8/8] arm64: stacktrace: track hyp stacks in unwinder's address space Mark Rutland
@ 2022-08-02 16:27   ` Kalesh Singh
  2022-08-02 17:48     ` Mark Rutland
  0 siblings, 1 reply; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02 16:27 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Currently unwind_next_frame_record() has an optional callback to convert
> the address space of the FP. This is necessary for the NVHE unwinder,
> which tracks the stacks in the hyp VA space, but accesses the frame
> records in the kernel VA space.
>
> This is a bit unfortunate since it clutters unwind_next_frame_record(),
> which will get in the way of future rework.
>
> Instead, this patch changes the NVHE unwinder to track the stacks in the
> kernel's VA space and translate to FP prior to calling
> unwind_next_frame_record(). This removes the need for the translate_fp()
> callback, as all unwinders consistently track stacks in the native
> address space of the unwinder.
>
> At the same time, this patch consolidates the generation of the stack
> addreses behind the stackinfo_get_*() helpers.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Fuad Tabba <tabba@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/stacktrace/common.h | 28 ++--------
>  arch/arm64/kernel/stacktrace.c             |  2 +-
>  arch/arm64/kvm/hyp/nvhe/stacktrace.c       |  2 +-
>  arch/arm64/kvm/stacktrace.c                | 62 ++++++++++++++--------
>  4 files changed, 46 insertions(+), 48 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index c594f332bb946..0f634bb14ceb3 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -78,17 +78,6 @@ static inline void unwind_init_common(struct unwind_state *state,
>         state->stack = stackinfo_get_unknown();
>  }
>
> -/*
> - * stack_trace_translate_fp_fn() - Translates a non-kernel frame pointer to
> - * a kernel address.
> - *
> - * @fp:   the frame pointer to be updated to its kernel address.
> - *
> - * Returns true and success and @fp is updated to the corresponding
> - * kernel virtual address; otherwise returns false.
> - */
> -typedef bool (*stack_trace_translate_fp_fn)(unsigned long *fp);
> -
>  static struct stack_info *unwind_find_next_stack(const struct unwind_state *state,
>                                                  unsigned long sp,
>                                                  unsigned long size)
> @@ -159,13 +148,11 @@ static inline int unwind_consume_stack(struct unwind_state *state,
>   * @state->fp.
>   *
>   * @state:        the current unwind state.
> - * @translate_fp: translates the fp prior to access (may be NULL)
>   */
>  static inline int
> -unwind_next_frame_record(struct unwind_state *state,
> -                        stack_trace_translate_fp_fn translate_fp)
> +unwind_next_frame_record(struct unwind_state *state)
>  {
> -       unsigned long fp = state->fp, kern_fp = fp;
> +       unsigned long fp = state->fp;
>         int err;
>
>         if (fp & 0x7)
> @@ -175,18 +162,11 @@ unwind_next_frame_record(struct unwind_state *state,
>         if (err)
>                 return err;
>
> -       /*
> -        * If fp is not from the current address space perform the necessary
> -        * translation before dereferencing it to get the next fp.
> -        */
> -       if (translate_fp && !translate_fp(&kern_fp))
> -               return -EINVAL;
> -
>         /*
>          * Record this frame record's values.
>          */
> -       state->fp = READ_ONCE(*(unsigned long *)(kern_fp));
> -       state->pc = READ_ONCE(*(unsigned long *)(kern_fp + 8));
> +       state->fp = READ_ONCE(*(unsigned long *)(fp));
> +       state->pc = READ_ONCE(*(unsigned long *)(fp + 8));
>
>         return 0;
>  }
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 1133be3ff774d..6b447319ce5b9 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -84,7 +84,7 @@ static int notrace unwind_next(struct unwind_state *state)
>         if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
>                 return -ENOENT;
>
> -       err = unwind_next_frame_record(state, NULL);
> +       err = unwind_next_frame_record(state);
>         if (err)
>                 return err;
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/stacktrace.c b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> index 08e1325ead73f..ed6b58b19cfa5 100644
> --- a/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> +++ b/arch/arm64/kvm/hyp/nvhe/stacktrace.c
> @@ -64,7 +64,7 @@ static struct stack_info stackinfo_get_hyp(void)
>
>  static int unwind_next(struct unwind_state *state)
>  {
> -       return unwind_next_frame_record(state, NULL);
> +       return unwind_next_frame_record(state);
>  }
>
>  static void notrace unwind(struct unwind_state *state,
> diff --git a/arch/arm64/kvm/stacktrace.c b/arch/arm64/kvm/stacktrace.c
> index 8295e132da2f0..fde1ec757d03a 100644
> --- a/arch/arm64/kvm/stacktrace.c
> +++ b/arch/arm64/kvm/stacktrace.c
> @@ -34,6 +34,17 @@ static struct stack_info stackinfo_get_overflow(void)
>         };
>  }
>
> +static struct stack_info stackinfo_get_overflow_kern_va(void)
> +{
> +       unsigned long low = (unsigned long)this_cpu_ptr_nvhe_sym(overflow_stack);
> +       unsigned long high = low + OVERFLOW_STACK_SIZE;
> +
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +       };
> +}
> +
>  static struct stack_info stackinfo_get_hyp(void)
>  {
>         struct kvm_nvhe_stacktrace_info *stacktrace_info
> @@ -47,6 +58,17 @@ static struct stack_info stackinfo_get_hyp(void)
>         };
>  }
>
> +static struct stack_info stackinfo_get_hyp_kern_va(void)
> +{
> +       unsigned long low = (unsigned long)*this_cpu_ptr(&kvm_arm_hyp_stack_page);
> +       unsigned long high = low + PAGE_SIZE;
> +
> +       return (struct stack_info) {
> +               .low = low,
> +               .high = high,
> +       };
> +}
> +
>  /*
>   * kvm_nvhe_stack_kern_va - Convert KVM nVHE HYP stack addresses to a kernel VAs
>   *
> @@ -62,39 +84,35 @@ static struct stack_info stackinfo_get_hyp(void)
>   */
>  static bool kvm_nvhe_stack_kern_va(unsigned long *addr)
>  {
> -       struct kvm_nvhe_stacktrace_info *stacktrace_info;
> -       unsigned long hyp_base, kern_base, hyp_offset;
> -       struct stack_info stack;
> +       struct stack_info stack_hyp, stack_kern;
>
> -       stacktrace_info = this_cpu_ptr_nvhe_sym(kvm_stacktrace_info);
> -
> -       stack = stackinfo_get_hyp();
> -       if (stackinfo_on_stack(&stack, *addr, 1)) {
> -               kern_base = (unsigned long)*this_cpu_ptr(&kvm_arm_hyp_stack_page);
> -               hyp_base = (unsigned long)stacktrace_info->stack_base;
> +       stack_hyp = stackinfo_get_hyp();
> +       stack_kern = stackinfo_get_hyp_kern_va();
> +       if (stackinfo_on_stack(&stack_hyp, *addr, 1))
>                 goto found;
> -       }
>
> -       stack = stackinfo_get_overflow();
> -       if (stackinfo_on_stack(&stack, *addr, 1)) {
> -               kern_base = (unsigned long)this_cpu_ptr_nvhe_sym(overflow_stack);
> -               hyp_base = (unsigned long)stacktrace_info->overflow_stack_base;
> +       stack_hyp = stackinfo_get_overflow();
> +       stack_kern = stackinfo_get_overflow_kern_va();
> +       if (stackinfo_on_stack(&stack_hyp, *addr, 1))
>                 goto found;
> -       }
>
>         return false;
>
>  found:
> -       hyp_offset = *addr - hyp_base;
> -
> -       *addr = kern_base + hyp_offset;
> -
> +       *addr = *addr - stack_hyp.low + stack_kern.low;
>         return true;
>  }
>
>  static int unwind_next(struct unwind_state *state)
>  {
> -       return unwind_next_frame_record(state, kvm_nvhe_stack_kern_va);
> +       /*
> +        * The FP is in the hypervisor VA space. Convert it to the kernel VA
> +        * space so it can be unwound be the regular unwind functions.

s/be the/by the

Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

> +        */
> +       if (!kvm_nvhe_stack_kern_va(&state->fp))
> +               return -EINVAL;
> +
> +       return unwind_next_frame_record(state);
>  }
>
>  static void unwind(struct unwind_state *state,
> @@ -153,8 +171,8 @@ static void hyp_dump_backtrace(unsigned long hyp_offset)
>  {
>         struct kvm_nvhe_stacktrace_info *stacktrace_info;
>         struct stack_info stacks[] = {
> -               stackinfo_get_overflow(),
> -               stackinfo_get_hyp(),
> +               stackinfo_get_overflow_kern_va(),
> +               stackinfo_get_hyp_kern_va(),
>         };
>         struct unwind_state state = {
>                 .stacks = stacks,
> --
> 2.30.2
>

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code
  2022-08-02 12:26     ` Mark Rutland
@ 2022-08-02 16:31       ` Kalesh Singh
  0 siblings, 0 replies; 28+ messages in thread
From: Kalesh Singh @ 2022-08-02 16:31 UTC (permalink / raw)
  To: Mark Rutland
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Tue, Aug 2, 2022 at 5:26 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Kalesh,
>
> On Mon, Aug 01, 2022 at 09:53:35PM -0700, Kalesh Singh wrote:
> > On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
> > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > > index 4c8865e495fea..04a9b56b114c1 100644
> > > --- a/arch/arm64/kernel/stacktrace.c
> > > +++ b/arch/arm64/kernel/stacktrace.c
> > > @@ -86,8 +86,17 @@ static bool on_accessible_stack(const struct task_struct *tsk,
> > >                 return true;
> > >         if (on_overflow_stack(sp, size, info))
> > >                 return true;
> > > -       if (on_sdei_stack(sp, size, info))
> > > -               return true;
> > > +
> > > +       if (IS_ENABLED(CONFIG_VMAP_STACK) &&
> > > +           IS_ENABLED(CONFIG_ARM_SDE_INTERFACE) &&
> > > +           in_nmi())
> >
> > I think we can remove the IS_ENABLED() checks since it's handled by
> > ifdefs in asm/stacktrace.h
> >
> > Otherwise, Reviewed-by: Kalesh Singh <kaleshsingh@google.com>
>
> I'd kept the IS_ENABLED() checks here to avoid code being generated of in_nmi()
> when SDEI is not configured in. Since in_nmi() uses preempt_count() and that
> uses READ_ONCE(), the compiler can't optimize the read away in case there are
> side effects (but I imagine will discard the value immediately).
>
> With that in mind, are you happy if I leave this as is, and take your
> Reviewed-by?

Hi Mark,

Feel free to keep the Reviewed-by. Thanks for the improvements.

--Kalesh


>
> Thanks,
> Mark.
>
> >
> > > +       {
> > > +               if (on_sdei_critical_stack(sp, size, info))
> > > +                       return true;
> > > +
> > > +               if (on_sdei_normal_stack(sp, size, info))
> > > +                       return true;
> > > +       }
> > >
> > >         return false;
> > >  }
> > > --
> > > 2.30.2
> > >

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 8/8] arm64: stacktrace: track hyp stacks in unwinder's address space
  2022-08-02 16:27   ` Kalesh Singh
@ 2022-08-02 17:48     ` Mark Rutland
  0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-02 17:48 UTC (permalink / raw)
  To: Kalesh Singh
  Cc: moderated list:ARM64 PORT (AARCH64 ARCHITECTURE),
	Mark Brown, Catalin Marinas, James Morse,
	Madhavan T. Venkataraman, Marc Zyngier, Fuad Tabba, Will Deacon

On Tue, Aug 02, 2022 at 09:27:33AM -0700, Kalesh Singh wrote:
> On Mon, Aug 1, 2022 at 5:12 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >  static int unwind_next(struct unwind_state *state)
> >  {
> > -       return unwind_next_frame_record(state, kvm_nvhe_stack_kern_va);
> > +       /*
> > +        * The FP is in the hypervisor VA space. Convert it to the kernel VA
> > +        * space so it can be unwound be the regular unwind functions.
> 
> s/be the/by the

Whoops; I've fixed that up locally now.

> Reviewed-by: Kalesh Singh <kaleshsingh@google.com>

Thanks!

Mark.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 6/8] arm64: stacktrace: remove stack type from fp translator
  2022-08-01 12:12 ` [PATCH 6/8] arm64: stacktrace: remove stack type from fp translator Mark Rutland
  2022-08-02  5:32   ` Kalesh Singh
@ 2022-08-03 12:36   ` Mark Brown
  1 sibling, 0 replies; 28+ messages in thread
From: Mark Brown @ 2022-08-03 12:36 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, catalin.marinas, james.morse, kaleshsingh,
	madvenka, maz, tabba, will


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

On Mon, Aug 01, 2022 at 01:12:07PM +0100, Mark Rutland wrote:
> In subsequent patches we'll remove the stack_type enum, and move the FP
> translation logic out of the raw FP unwind code.

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

[-- 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

^ permalink raw reply	[flat|nested] 28+ messages in thread
* Re: [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code
  2022-08-01 12:12 ` [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code Mark Rutland
  2022-08-01 12:48   ` Mark Brown
  2022-08-02  4:53   ` Kalesh Singh
@ 2022-08-04 14:38   ` Mark Rutland
  2 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2022-08-04 14:38 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: broonie, catalin.marinas, james.morse, kaleshsingh, madvenka,
	maz, tabba, will

On Mon, Aug 01, 2022 at 01:12:04PM +0100, Mark Rutland wrote:
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 6ebdcdff77f56..fa2df1ea22ebc 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h

> +#if defined(CONFIG_ARM_SDE_INTERFACE) && defined(CONFIG_VMAP_STACK)
> +DECLARE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
> +DECLARE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);

> diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
> index d20620a1c51a4..881eece3422ca 100644
> --- a/arch/arm64/kernel/sdei.c
> +++ b/arch/arm64/kernel/sdei.c
> @@ -31,9 +31,6 @@ unsigned long sdei_exit_mode;
>   * sdei stack.
>   * For now, we allocate stacks when the driver is probed.
>   */
> -DECLARE_PER_CPU(unsigned long *, sdei_stack_normal_ptr);
> -DECLARE_PER_CPU(unsigned long *, sdei_stack_critical_ptr);
> -

The kbuild test robot reported that removing these lines broke builds with
CONFIG_SDE_INTERFACE=y && CONFIG_VMAP_STACK=n:

  https://lore.kernel.org/r/16ba6649-d680-69d4-dbd3-3b1ffb06319b@intel.com

For now I've restored the declarations in sdei.c, as the alternative was to
make the ifdeffery in stacktrace.h more convoluted.

Thanks,
Mark.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-08-04 14:40 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-01 12:12 [PATCH 0/8] arm64: stacktrace: cleanups and improvements Mark Rutland
2022-08-01 12:12 ` [PATCH 1/8] arm64: stacktrace: simplify unwind_next_common() Mark Rutland
2022-08-01 12:46   ` Mark Brown
2022-08-02  4:34   ` Kalesh Singh
2022-08-01 12:12 ` [PATCH 2/8] arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record() Mark Rutland
2022-08-01 12:52   ` Mark Brown
2022-08-02  4:38   ` Kalesh Singh
2022-08-01 12:12 ` [PATCH 3/8] arm64: stacktrace: move SDEI stack helpers to stacktrace code Mark Rutland
2022-08-01 12:48   ` Mark Brown
2022-08-02  4:53   ` Kalesh Singh
2022-08-02 12:26     ` Mark Rutland
2022-08-02 16:31       ` Kalesh Singh
2022-08-04 14:38   ` Mark Rutland
2022-08-01 12:12 ` [PATCH 4/8] arm64: stacktrace: add stackinfo_on_stack() helper Mark Rutland
2022-08-01 15:56   ` Mark Brown
2022-08-02  5:00   ` Kalesh Singh
2022-08-01 12:12 ` [PATCH 5/8] arm64: stacktrace: rework stack boundary discovery Mark Rutland
2022-08-01 17:14   ` Mark Brown
2022-08-02  5:22   ` Kalesh Singh
2022-08-02 12:29     ` Mark Rutland
2022-08-01 12:12 ` [PATCH 6/8] arm64: stacktrace: remove stack type from fp translator Mark Rutland
2022-08-02  5:32   ` Kalesh Singh
2022-08-03 12:36   ` Mark Brown
2022-08-01 12:12 ` [PATCH 7/8] arm64: stacktrace: track all stack boundaries explicitly Mark Rutland
2022-08-02 16:05   ` Kalesh Singh
2022-08-01 12:12 ` [PATCH 8/8] arm64: stacktrace: track hyp stacks in unwinder's address space Mark Rutland
2022-08-02 16:27   ` Kalesh Singh
2022-08-02 17:48     ` Mark Rutland

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).