Re: [PATCH v5 04/17] arm64: stacktrace: Handle frame pointer from different address spaces

Fuad Tabba
2022-07-21T09:58:09Z

Hi Kalesh,

On Thu, Jul 21, 2022 at 6:57 AM Kalesh Singh <kaleshsingh@google.com> 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.
>
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
> 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(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 0c5cbfdb56b5..e89c8c39858d 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 it's kernel address.

nit: it's -> its

Otherwise

Reviewed-by: Fuad Tabba <tabba@google.com>

Cheers,
/fuad

> + * @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;
>
> --
> 2.37.0.170.g444d1eabd0-goog
>