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

[Kalesh Singh]

On Fri, Jul 15, 2022 at 6:57 AM 'Fuad Tabba' via kernel-team
<kernel-team@xxxxxxxxxxx> wrote:
>
> Hi Kalesh,
>
> On Fri, Jul 15, 2022 at 7:11 AM 'Kalesh Singh' via kernel-team
> <kernel-team@xxxxxxxxxxx> 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 <kaleshsingh@xxxxxxxxxx>
> > ---
> >  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 kernel-team+unsubscribe@xxxxxxxxxxx.
> >
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@xxxxxxxxxxx.
>