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 tracnslate 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 <[email protected]>
---
arch/arm64/include/asm/stacktrace/common.h | 26 ++++++++++++++++++++--
arch/arm64/kernel/stacktrace.c | 2 +-
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
index 0c5cbfdb56b5..5f5d74a286f3 100644
--- a/arch/arm64/include/asm/stacktrace/common.h
+++ b/arch/arm64/include/asm/stacktrace/common.h
@@ -123,9 +123,22 @@ static inline void unwind_init_common(struct unwind_state *state,
state->prev_fp = 0;
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.
+ * @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)
{
struct task_struct *tsk = state->task;
unsigned long fp = state->fp;
@@ -159,13 +172,22 @@ static inline int unwind_next_common(struct unwind_state *state,
__set_bit(state->prev_type, state->stacks_done);
}
+ /* Record fp as prev_fp before attempting to get the next fp */
+ state->prev_fp = fp;
+
+ /*
+ * 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(&fp, info->type))
+ return -EINVAL;
+
/*
* 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->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 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
Hi Kalesh,
On Fri, Jul 15, 2022 at 7:11 AM 'Kalesh Singh' via kernel-team
<[email protected]> 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 tracnslate HYP stack
s/tracnslate/translate
> addresses to kernel addresses.
>
> Add a callback (stack_trace_translate_fp_fn) to allow specifying
> the translation function.
>
> Signed-off-by: Kalesh Singh <[email protected]>
> ---
> arch/arm64/include/asm/stacktrace/common.h | 26 ++++++++++++++++++++--
> arch/arm64/kernel/stacktrace.c | 2 +-
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 0c5cbfdb56b5..5f5d74a286f3 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -123,9 +123,22 @@ static inline void unwind_init_common(struct unwind_state *state,
> state->prev_fp = 0;
> 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.
> + * @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.
> + */
Please add a newline before the new block.
Also, something which you have done in comment blocks in this patch as
well as future patches (so I won't mention them again) is use the
opening comment mark /** , which is meant for kernel-doc comments
(https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html).
However, this block, and many if not most of the others don't seem to
be conformant (scripts/kernel-doc -v -none
arch/arm64/include/asm/stacktrace/common.h).
I think the easiest thing to do is to format them as a normal block: /*.
> +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)
> {
> struct task_struct *tsk = state->task;
> unsigned long fp = state->fp;
> @@ -159,13 +172,22 @@ static inline int unwind_next_common(struct unwind_state *state,
> __set_bit(state->prev_type, state->stacks_done);
> }
>
> + /* Record fp as prev_fp before attempting to get the next fp */
> + state->prev_fp = fp;
> +
> + /*
> + * 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(&fp, info->type))
> + return -EINVAL;
> +
A call to unwind_next_common could fail having updated state->prev_fp
as well as state->stacks_done. I think that it might be better to
rework it so that there aren't any side effects should a call fail.
Thanks,
/fuad
> /*
> * 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->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 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
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Fri, Jul 15, 2022 at 6:57 AM 'Fuad Tabba' via kernel-team
<[email protected]> wrote:
>
> Hi Kalesh,
>
> On Fri, Jul 15, 2022 at 7:11 AM 'Kalesh Singh' via kernel-team
> <[email protected]> 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 tracnslate HYP stack
>
> s/tracnslate/translate
>
> > addresses to kernel addresses.
> >
> > Add a callback (stack_trace_translate_fp_fn) to allow specifying
> > the translation function.
> >
> > Signed-off-by: Kalesh Singh <[email protected]>
> > ---
> > arch/arm64/include/asm/stacktrace/common.h | 26 ++++++++++++++++++++--
> > arch/arm64/kernel/stacktrace.c | 2 +-
> > 2 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> > index 0c5cbfdb56b5..5f5d74a286f3 100644
> > --- a/arch/arm64/include/asm/stacktrace/common.h
> > +++ b/arch/arm64/include/asm/stacktrace/common.h
> > @@ -123,9 +123,22 @@ static inline void unwind_init_common(struct unwind_state *state,
> > state->prev_fp = 0;
> > 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.
> > + * @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.
> > + */
>
> Please add a newline before the new block.
>
> Also, something which you have done in comment blocks in this patch as
> well as future patches (so I won't mention them again) is use the
> opening comment mark /** , which is meant for kernel-doc comments
> (https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html).
> However, this block, and many if not most of the others don't seem to
> be conformant (scripts/kernel-doc -v -none
> arch/arm64/include/asm/stacktrace/common.h).
>
> I think the easiest thing to do is to format them as a normal block: /*.
>
>
> > +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)
> > {
> > struct task_struct *tsk = state->task;
> > unsigned long fp = state->fp;
> > @@ -159,13 +172,22 @@ static inline int unwind_next_common(struct unwind_state *state,
> > __set_bit(state->prev_type, state->stacks_done);
> > }
> >
> > + /* Record fp as prev_fp before attempting to get the next fp */
> > + state->prev_fp = fp;
> > +
> > + /*
> > + * 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(&fp, info->type))
> > + return -EINVAL;
> > +
>
> A call to unwind_next_common could fail having updated state->prev_fp
> as well as state->stacks_done. I think that it might be better to
> rework it so that there aren't any side effects should a call fail.
Hi Fuad,
Thanks for the comments. I'll address them in the next version.
--Kalesh
>
> Thanks,
> /fuad
>
>
>
>
>
> > /*
> > * 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->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 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
> >
> > --
> > To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> >
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>