Re: [PATCH v2 4/6] LoongArch: Strip guess_unwinder out from prologue_unwinder

[Jinyang He]

On 2023-01-01 17:56, Huacai Chen wrote:

> Hi, Jinyang,
>
> On Thu, Dec 29, 2022 at 2:15 PM Jinyang He <hejinyang@xxxxxxxxxxx> wrote:
> > The prolugue unwinder rely on symbol info. When PC is not in kernel
> > text address, it cannot find relative symbol info and it will be broken.
> > The guess unwinder will be used in this case. And the guess unwinder
> > codes in prolugue unwinder is redundant. Strip it out and set the
> > unwinder info in unwind_state.
> >
> > Signed-off-by: Jinyang He <hejinyang@xxxxxxxxxxx>
> > ---
> >   arch/loongarch/include/asm/unwind.h     |  26 ++++-
> >   arch/loongarch/kernel/Makefile          |   3 +-
> >   arch/loongarch/kernel/traps.c           |   3 -
> >   arch/loongarch/kernel/unwind.c          |  52 +++++++++
> >   arch/loongarch/kernel/unwind_guess.c    |  41 ++------
> >   arch/loongarch/kernel/unwind_prologue.c | 133 +++++++++---------------
> >   6 files changed, 132 insertions(+), 126 deletions(-)
> >   create mode 100644 arch/loongarch/kernel/unwind.c
> >
> > diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
> > index 6ece48f0ff77..4a55fd7b77ad 100644
> > --- a/arch/loongarch/include/asm/unwind.h
> > +++ b/arch/loongarch/include/asm/unwind.h
> > @@ -13,20 +13,30 @@
> >   #include <asm/ptrace.h>
> >   #include <asm/stacktrace.h>
> >
> > -enum unwinder_type {
> > -       UNWINDER_GUESS,
> > -       UNWINDER_PROLOGUE,
> > -};
> > +struct unwinder_ops;
> >
> >   struct unwind_state {
> > -       char type; /* UNWINDER_XXX */
> >          struct stack_info stack_info;
> >          struct task_struct *task;
> >          bool first, error, is_ftrace;
> >          int graph_idx;
> >          unsigned long sp, pc, ra;
> > +       const struct unwinder_ops *ops;
> > +};
> > +
> > +struct unwinder_ops {
> > +       void (*unwind_start)(struct unwind_state *state,
> > +                            struct task_struct *task, struct pt_regs *regs);
> > +       bool (*unwind_next_frame)(struct unwind_state *state);
> > +       unsigned long (*unwind_get_return_address)(struct unwind_state *state);
> >   };
> >
> > +extern const struct unwinder_ops *default_unwinder;
> > +extern const struct unwinder_ops unwinder_guess;
> > +#ifdef CONFIG_UNWINDER_PROLOGUE
> > +extern const struct unwinder_ops unwinder_prologue;
> > +#endif
> > +
> >   void unwind_start(struct unwind_state *state,
> >                    struct task_struct *task, struct pt_regs *regs);
> >   bool unwind_next_frame(struct unwind_state *state);
> > @@ -49,4 +59,10 @@ static inline unsigned long unwind_graph_addr(struct unwind_state *state,
> >          return ftrace_graph_ret_addr(state->task, &state->graph_idx,
> >                                       pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
> >   }
> > +
> > +static inline void unwind_register_unwinder(struct unwind_state *state,
> > +                                         const struct unwinder_ops *unwinder)
> > +{
> > +       state->ops = unwinder;
> > +}
> >   #endif /* _ASM_UNWIND_H */
> > diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
> > index 7ca65195f7f8..cb6029ea3ea9 100644
> > --- a/arch/loongarch/kernel/Makefile
> > +++ b/arch/loongarch/kernel/Makefile
> > @@ -8,7 +8,7 @@ extra-y         := vmlinux.lds
> >   obj-y          += head.o cpu-probe.o cacheinfo.o env.o setup.o entry.o genex.o \
> >                     traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o switch.o \
> >                     elf.o syscall.o signal.o time.o topology.o inst.o ptrace.o vdso.o \
> > -                  alternative.o unaligned.o
> > +                  alternative.o unaligned.o unwind.o unwind_guess.o
> >
> >   obj-$(CONFIG_ACPI)             += acpi.o
> >   obj-$(CONFIG_EFI)              += efi.o
> > @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ)     += sysrq.o
> >   obj-$(CONFIG_KEXEC)            += machine_kexec.o relocate_kernel.o
> >   obj-$(CONFIG_CRASH_DUMP)       += crash_dump.o
> >
> > -obj-$(CONFIG_UNWINDER_GUESS)   += unwind_guess.o
> >   obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
> We do have CONFIG_UNWINDER_GUESS and I don't like to build guess
> unwinder unconditionally, maybe we can refactor it to make the
> unwinder still user selectable.


In order to provide backtrace info to the user, prologue unwinder
will use guess unwinder when its first pc is not a kernel address,
such as the first PC is in ebpf generated codes. This patch is intended
to extract guess unwinder codes to avoid redundant codes. And if we provide
a new unwinder in the future like orc unwinder which take effect after
orc_init is set, we can use another backup unwinder like guess unwinder in
case we need backtrace before orc_init being set.

If you mind this, could we just do as follows,

obj-$(CONFIG_UNWINDER_GUESS)   += unwind_guess.o
obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o unwind_guess.o

Or copy the guess unwinder codes to unwind_prologue.c and set another
name for guess unwinder.


Thanks,

Jinyang

> Huacai
> >   obj-$(CONFIG_PERF_EVENTS)      += perf_event.o perf_regs.o
> > diff --git a/arch/loongarch/kernel/traps.c b/arch/loongarch/kernel/traps.c
> > index 3adc44832205..1ea14f6c18d3 100644
> > --- a/arch/loongarch/kernel/traps.c
> > +++ b/arch/loongarch/kernel/traps.c
> > @@ -72,9 +72,6 @@ static void show_backtrace(struct task_struct *task, const struct pt_regs *regs,
> >          if (!task)
> >                  task = current;
> >
> > -       if (user_mode(regs))
> > -               state.type = UNWINDER_GUESS;
> > -
> >          printk("%sCall Trace:", loglvl);
> >          for (unwind_start(&state, task, pregs);
> >                !unwind_done(&state); unwind_next_frame(&state)) {
> > diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
> > new file mode 100644
> > index 000000000000..24d2cf99bfb6
> > --- /dev/null
> > +++ b/arch/loongarch/kernel/unwind.c
> > @@ -0,0 +1,52 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2022 Loongson Technology Corporation Limited
> > + */
> > +#include <asm/unwind.h>
> > +
> > +#if defined(CONFIG_UNWINDER_GUESS)
> > +const struct unwinder_ops *default_unwinder = &unwinder_guess;
> > +#elif defined(CONFIG_UNWINDER_PROLOGUE)
> > +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
> > +#endif
> > +
> > +unsigned long unwind_get_return_address(struct unwind_state *state)
> > +{
> > +       if (!state->ops || unwind_done(state))
> > +               return 0;
> > +       return state->ops->unwind_get_return_address(state);
> > +}
> > +EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > +
> > +void unwind_start(struct unwind_state *state, struct task_struct *task,
> > +                   struct pt_regs *regs)
> > +{
> > +       memset(state, 0, sizeof(*state));
> > +       unwind_register_unwinder(state, default_unwinder);
> > +       if (regs) {
> > +               state->sp = regs->regs[3];
> > +               state->pc = regs->csr_era;
> > +               state->ra = regs->regs[1];
> > +       } else if (task == current || task == NULL) {
> > +               state->sp = (unsigned long)__builtin_frame_address(0);
> > +               state->pc = (unsigned long)__builtin_return_address(0);
> > +               state->ra = 0;
> > +       } else {
> > +               state->sp = thread_saved_fp(task);
> > +               state->pc = thread_saved_ra(task);
> > +               state->ra = 0;
> > +       }
> > +       state->task = task;
> > +       get_stack_info(state->sp, state->task, &state->stack_info);
> > +       state->pc = unwind_graph_addr(state, state->pc, state->sp);
> > +       state->ops->unwind_start(state, task, regs);
> > +}
> > +EXPORT_SYMBOL_GPL(unwind_start);
> > +
> > +bool unwind_next_frame(struct unwind_state *state)
> > +{
> > +       if (!state->ops || unwind_done(state))
> > +               return false;
> > +       return state->ops->unwind_next_frame(state);
> > +}
> > +EXPORT_SYMBOL_GPL(unwind_next_frame);
> > diff --git a/arch/loongarch/kernel/unwind_guess.c b/arch/loongarch/kernel/unwind_guess.c
> > index 935d24f8c95c..b7ca2b88ac63 100644
> > --- a/arch/loongarch/kernel/unwind_guess.c
> > +++ b/arch/loongarch/kernel/unwind_guess.c
> > @@ -7,51 +7,23 @@
> >
> >   #include <asm/unwind.h>
> >
> > -unsigned long unwind_get_return_address(struct unwind_state *state)
> > +static unsigned long get_return_address(struct unwind_state *state)
> >   {
> > -       if (unwind_done(state))
> > -               return 0;
> >          return state->pc;
> >   }
> > -EXPORT_SYMBOL_GPL(unwind_get_return_address);
> >
> > -void unwind_start(struct unwind_state *state, struct task_struct *task,
> > +static void start(struct unwind_state *state, struct task_struct *task,
> >                      struct pt_regs *regs)
> >   {
> > -       memset(state, 0, sizeof(*state));
> > -
> > -       if (regs) {
> > -               state->sp = regs->regs[3];
> > -               state->pc = regs->csr_era;
> > -       } else if (task == current || task == NULL) {
> > -               state->sp = (unsigned long)__builtin_frame_address(0);
> > -               state->pc = (unsigned long)__builtin_return_address(0);
> > -       } else {
> > -               state->sp = thread_saved_fp(task);
> > -               state->pc = thread_saved_ra(task);
> > -       }
> > -
> > -       state->task = task;
> > -       state->first = true;
> > -       state->pc = unwind_graph_addr(state, state->pc, state->sp);
> > -       get_stack_info(state->sp, state->task, &state->stack_info);
> > -
> >          if (!unwind_done(state) && !__kernel_text_address(state->pc))
> >                  unwind_next_frame(state);
> >   }
> > -EXPORT_SYMBOL_GPL(unwind_start);
> >
> > -bool unwind_next_frame(struct unwind_state *state)
> > +static bool next_frame(struct unwind_state *state)
> >   {
> >          struct stack_info *info = &state->stack_info;
> >          unsigned long addr;
> >
> > -       if (unwind_done(state))
> > -               return false;
> > -
> > -       if (state->first)
> > -               state->first = false;
> > -
> >          do {
> >                  for (state->sp += sizeof(unsigned long);
> >                       state->sp < info->end;
> > @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
> >
> >          return false;
> >   }
> > -EXPORT_SYMBOL_GPL(unwind_next_frame);
> > +
> > +const struct unwinder_ops unwinder_guess = {
> > +       .unwind_start = start,
> > +       .unwind_next_frame = next_frame,
> > +       .unwind_get_return_address = get_return_address,
> > +};
> > diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
> > index f7994ed05f04..beb57ea24da2 100644
> > --- a/arch/loongarch/kernel/unwind_prologue.c
> > +++ b/arch/loongarch/kernel/unwind_prologue.c
> > @@ -19,31 +19,19 @@ static inline void unwind_state_fixup(struct unwind_state *state)
> >   #endif
> >   }
> >
> > -unsigned long unwind_get_return_address(struct unwind_state *state)
> > +static unsigned long get_return_address(struct unwind_state *state)
> >   {
> > -       if (unwind_done(state))
> > -               return 0;
> >          return state->pc;
> >   }
> > -EXPORT_SYMBOL_GPL(unwind_get_return_address);
> > -
> > -static bool unwind_by_guess(struct unwind_state *state)
> > -{
> > -       struct stack_info *info = &state->stack_info;
> > -       unsigned long addr;
> > -
> > -       for (state->sp += sizeof(unsigned long);
> > -            state->sp < info->end;
> > -            state->sp += sizeof(unsigned long)) {
> > -               addr = *(unsigned long *)(state->sp);
> > -               state->pc = unwind_graph_addr(state, addr, state->sp + 8);
> > -               if (__kernel_text_address(state->pc))
> > -                       return true;
> > -       }
> > -
> > -       return false;
> > -}
> >
> > +/*
> > + * LoongArch function prologue like follows,
> > + *     [others instructions not use stack var]
> > + *     addi.d sp, sp, -imm
> > + *     st.d   xx, sp, offset <- save callee saved regs and
> > + *     st.d   yy, sp, offset    save ra if function is nest.
> > + *     [others instructions]
> > + */
> >   static bool unwind_by_prologue(struct unwind_state *state)
> >   {
> >          long frame_ra = -1;
> > @@ -89,6 +77,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
> >                  ip++;
> >          }
> >
> > +       /*
> > +        * Not find stack alloc action, PC may be in a leaf function. Only the
> > +        * first being true is reasonable, otherwise indicate analysis is broken.
> > +        */
> >          if (!frame_size) {
> >                  if (state->first)
> >                          goto first;
> > @@ -106,6 +98,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
> >                  ip++;
> >          }
> >
> > +       /* Not find save $ra action, PC may be in a leaf function, too. */
> >          if (frame_ra < 0) {
> >                  if (state->first) {
> >                          state->sp = state->sp + frame_size;
> > @@ -114,96 +107,63 @@ static bool unwind_by_prologue(struct unwind_state *state)
> >                  return false;
> >          }
> >
> > -       if (state->first)
> > -               state->first = false;
> > -
> >          state->pc = *(unsigned long *)(state->sp + frame_ra);
> >          state->sp = state->sp + frame_size;
> >          goto out;
> >
> >   first:
> > -       state->first = false;
> > -       if (state->pc == state->ra)
> > -               return false;
> > -
> >          state->pc = state->ra;
> >
> >   out:
> > +       state->first = false;
> >          unwind_state_fixup(state);
> >          return !!__kernel_text_address(state->pc);
> >   }
> >
> > -void unwind_start(struct unwind_state *state, struct task_struct *task,
> > +static void start(struct unwind_state *state, struct task_struct *task,
> >                      struct pt_regs *regs)
> >   {
> > -       memset(state, 0, sizeof(*state));
> > -       state->type = UNWINDER_PROLOGUE;
> > -
> > -       if (regs) {
> > -               state->sp = regs->regs[3];
> > -               state->pc = regs->csr_era;
> > -               state->ra = regs->regs[1];
> > -               if (!__kernel_text_address(state->pc))
> > -                       state->type = UNWINDER_GUESS;
> > -       } else if (task == current || task == NULL) {
> > -               state->sp = (unsigned long)__builtin_frame_address(0);
> > -               state->pc = (unsigned long)__builtin_return_address(0);
> > -               state->ra = 0;
> > -       } else {
> > -               state->sp = thread_saved_fp(task);
> > -               state->pc = thread_saved_ra(task);
> > -               state->ra = 0;
> > -       }
> > -
> > -       state->task = task;
> >          state->first = true;
> > -       state->pc = unwind_graph_addr(state, state->pc, state->sp);
> > -       get_stack_info(state->sp, state->task, &state->stack_info);
> >
> > -       if (!unwind_done(state) && !__kernel_text_address(state->pc))
> > -               unwind_next_frame(state);
> > +       /*
> > +        * The current PC is not kernel text address, we cannot find its
> > +        * relative symbol. Thus, prologue analysis will be broken. Luckly,
> > +        * we can use the guess unwinder.
> > +        */
> > +       if (!__kernel_text_address(state->pc)) {
> > +               unwind_register_unwinder(state, &unwinder_guess);
> > +               if (!unwind_done(state))
> > +                       unwind_next_frame(state);
> > +       }
> >   }
> > -EXPORT_SYMBOL_GPL(unwind_start);
> >
> > -bool unwind_next_frame(struct unwind_state *state)
> > +static bool next_frame(struct unwind_state *state)
> >   {
> >          struct stack_info *info = &state->stack_info;
> >          struct pt_regs *regs;
> >          unsigned long pc;
> >
> > -       if (unwind_done(state))
> > -               return false;
> > -
> >          do {
> > -               switch (state->type) {
> > -               case UNWINDER_GUESS:
> > -                       state->first = false;
> > -                       if (unwind_by_guess(state))
> > -                               return true;
> > -                       break;
> > -
> > -               case UNWINDER_PROLOGUE:
> > -                       if (unwind_by_prologue(state)) {
> > -                               state->pc = unwind_graph_addr(state, state->pc, state->sp);
> > -                               return true;
> > -                       }
> > +               if (unwind_by_prologue(state)) {
> > +                       state->pc = unwind_graph_addr(state, state->pc, state->sp);
> > +                       return true;
> > +               }
> >
> > -                       if (info->type == STACK_TYPE_IRQ &&
> > -                               info->end == state->sp) {
> > -                               regs = (struct pt_regs *)info->next_sp;
> > -                               pc = regs->csr_era;
> > +               if (info->type == STACK_TYPE_IRQ &&
> > +                   info->end == state->sp) {
> > +                       regs = (struct pt_regs *)info->next_sp;
> > +                       pc = regs->csr_era;
> >
> > -                               if (user_mode(regs) || !__kernel_text_address(pc))
> > -                                       return false;
> > +                       if (user_mode(regs) || !__kernel_text_address(pc))
> > +                               return false;
> >
> > -                               state->first = true;
> > -                               state->ra = regs->regs[1];
> > -                               state->sp = regs->regs[3];
> > -                               state->pc = pc;
> > -                               get_stack_info(state->sp, state->task, info);
> > +                       state->first = true;
> > +                       state->ra = regs->regs[1];
> > +                       state->sp = regs->regs[3];
> > +                       state->pc = pc;
> > +                       get_stack_info(state->sp, state->task, info);
> >
> > -                               return true;
> > -                       }
> > +                       return true;
> >                  }
> >
> >                  state->sp = info->next_sp;
> > @@ -212,4 +172,9 @@ bool unwind_next_frame(struct unwind_state *state)
> >
> >          return false;
> >   }
> > -EXPORT_SYMBOL_GPL(unwind_next_frame);
> > +
> > +const struct unwinder_ops unwinder_prologue = {
> > +       .unwind_start = start,
> > +       .unwind_next_frame = next_frame,
> > +       .unwind_get_return_address = get_return_address,
> > +};
> > --
> > 2.34.3