diff mbox series

[v6,04/17] arm64: stacktrace: Handle frame pointer from different address spaces

Message ID 20220726073750.3219117-5-kaleshsingh@google.com (mailing list archive)
State New, archived
Headers show
Series KVM nVHE Hypervisor stack unwinder | expand

Commit Message

Kalesh Singh July 26, 2022, 7:37 a.m. UTC 
The unwinder code is made reusable so that it can be used to
unwind various types of stacks. One usecase is unwinding the
nVHE hyp stack from the host (EL1) in non-protected mode. This
means that the unwinder must be able to translate HYP stack
addresses to kernel addresses.

Add a callback (stack_trace_translate_fp_fn) to allow specifying
the translation function.

Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
Reviewed-by: Fuad Tabba <tabba@google.com>
Tested-by: Fuad Tabba <tabba@google.com>
---

Changes in v6:
  - Fix typo in comment, per Fuad
  - Add Fuad’s Reviewed-by tag and Tested-by tags

Changes in v5:
  - Fix typo in commit text, per Fuad
  - Update unwind_next_common() to not have side effects on failure, per Fuad
  - Use regular comment instead of doc comments, per Fuad

 arch/arm64/include/asm/stacktrace/common.h | 29 +++++++++++++++++++---
 arch/arm64/kernel/stacktrace.c             |  2 +-
 2 files changed, 26 insertions(+), 5 deletions(-)

Comments

Mark Brown July 26, 2022, 2:34 p.m. UTC | #1 
On Tue, Jul 26, 2022 at 12:37:37AM -0700, Kalesh Singh wrote:
> The unwinder code is made reusable so that it can be used to
> unwind various types of stacks. One usecase is unwinding the
> nVHE hyp stack from the host (EL1) in non-protected mode. This
> means that the unwinder must be able to translate HYP stack
> addresses to kernel addresses.
> 
> Add a callback (stack_trace_translate_fp_fn) to allow specifying
> the translation function.

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

with or without one very minor thing:

>  static inline int unwind_next_common(struct unwind_state *state,
> -				     struct stack_info *info)
> +				     struct stack_info *info,
> +				     stack_trace_translate_fp_fn translate_fp)
>  {
> +	unsigned long fp = state->fp, kern_fp = fp;

As a coding style nit I don't love having multiple assignments on a
single line especially as part of declarations.
Kalesh Singh July 26, 2022, 3:30 p.m. UTC | #2 
On Tue, Jul 26, 2022 at 7:34 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jul 26, 2022 at 12:37:37AM -0700, Kalesh Singh wrote:
> > The unwinder code is made reusable so that it can be used to
> > unwind various types of stacks. One usecase is unwinding the
> > nVHE hyp stack from the host (EL1) in non-protected mode. This
> > means that the unwinder must be able to translate HYP stack
> > addresses to kernel addresses.
> >
> > Add a callback (stack_trace_translate_fp_fn) to allow specifying
> > the translation function.
>
> Reviewed-by: Mark Brown <broonie@kernel.org>
>
> with or without one very minor thing:
>
> >  static inline int unwind_next_common(struct unwind_state *state,
> > -                                  struct stack_info *info)
> > +                                  struct stack_info *info,
> > +                                  stack_trace_translate_fp_fn translate_fp)
> >  {
> > +     unsigned long fp = state->fp, kern_fp = fp;
>
> As a coding style nit I don't love having multiple assignments on a
> single line especially as part of declarations.

Hi Mark,

Thanks for the reviews. I'll update this if a respin is needed.

--Kalesh
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index 0c5cbfdb56b5..b241edba5c76 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -124,11 +124,25 @@  static inline void unwind_init_common(struct unwind_state *state,
 	state->prev_type = STACK_TYPE_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.
+ * @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);
+
 static inline int unwind_next_common(struct unwind_state *state,
-				     struct stack_info *info)
+				     struct stack_info *info,
+				     stack_trace_translate_fp_fn translate_fp)
 {
+	unsigned long fp = state->fp, kern_fp = fp;
 	struct task_struct *tsk = state->task;
-	unsigned long fp = state->fp;
 
 	if (fp & 0x7)
 		return -EINVAL;
@@ -139,6 +153,13 @@  static inline int unwind_next_common(struct unwind_state *state,
 	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))
+		return -EINVAL;
+
 	/*
 	 * As stacks grow downward, any valid record on the same stack must be
 	 * at a strictly higher address than the prior record.
@@ -163,8 +184,8 @@  static inline int unwind_next_common(struct unwind_state *state,
 	 * Record this frame record's values and location. The prev_fp and
 	 * prev_type are only meaningful to the next unwind_next() invocation.
 	 */
-	state->fp = READ_ONCE(*(unsigned long *)(fp));
-	state->pc = READ_ONCE(*(unsigned long *)(fp + 8));
+	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;
 
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 834851939364..eef3cf6bf2d7 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -87,7 +87,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, &info);
+	err = unwind_next_common(state, &info, NULL);
 	if (err)
 		return err;