lkml.org 
[lkml]   [2022]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v4 04/18] arm64: stacktrace: Handle frame pointer from different address spaces
    On Fri, Jul 15, 2022 at 6:57 AM 'Fuad Tabba' via kernel-team
    <kernel-team@android.com> wrote:
    >
    > Hi Kalesh,
    >
    > On Fri, Jul 15, 2022 at 7:11 AM 'Kalesh Singh' via kernel-team
    > <kernel-team@android.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 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@google.com>
    > > ---
    > > 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@android.com.
    > >
    >
    > --
    > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
    >

    \
     
     \ /
      Last update: 2022-07-18 19:41    [W:3.325 / U:0.292 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site