2022-12-15 04:10:51

by Jinyang He

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

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 <[email protected]>
---
arch/loongarch/include/asm/unwind.h | 22 ++++
arch/loongarch/kernel/Makefile | 3 +-
arch/loongarch/kernel/unwind.c | 52 +++++++++
arch/loongarch/kernel/unwind_guess.c | 41 ++-----
arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
5 files changed, 135 insertions(+), 118 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..a16aff1d086a 100644
--- a/arch/loongarch/include/asm/unwind.h
+++ b/arch/loongarch/include/asm/unwind.h
@@ -18,6 +18,8 @@ enum unwinder_type {
UNWINDER_PROLOGUE,
};

+struct unwinder_ops;
+
struct unwind_state {
char type; /* UNWINDER_XXX */
struct stack_info stack_info;
@@ -25,8 +27,22 @@ struct unwind_state {
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 +65,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

obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
new file mode 100644
index 000000000000..568c6fe707d1
--- /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) {
+ 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 8ce32c37c587..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) {
- 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 d464c533c64f..9677e13c4b4c 100644
--- a/arch/loongarch/kernel/unwind_prologue.c
+++ b/arch/loongarch/kernel/unwind_prologue.c
@@ -9,6 +9,8 @@
#include <asm/ptrace.h>
#include <asm/unwind.h>

+static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
+
static inline void unwind_state_fixup(struct unwind_state *state)
{
#ifdef CONFIG_DYNAMIC_FTRACE
@@ -19,31 +21,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 +79,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 +100,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 +109,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) {
- 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 guard unwinder.
+ */
+ if (!__kernel_text_address(state->pc)) {
+ unwind_register_unwinder(state, guard_unwinder);
+ 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 +174,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

2022-12-15 10:13:31

by Qing Zhang

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

Hi, Jinyang

On 2022/12/15 下午12:01, Jinyang He 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 <[email protected]>
> ---
> arch/loongarch/include/asm/unwind.h | 22 ++++
> arch/loongarch/kernel/Makefile | 3 +-
> arch/loongarch/kernel/unwind.c | 52 +++++++++
> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
> arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
> 5 files changed, 135 insertions(+), 118 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..a16aff1d086a 100644
> --- a/arch/loongarch/include/asm/unwind.h
> +++ b/arch/loongarch/include/asm/unwind.h
> @@ -18,6 +18,8 @@ enum unwinder_type {
> UNWINDER_PROLOGUE,
> };
>
> +struct unwinder_ops;
> +
> struct unwind_state {
> char type; /* UNWINDER_XXX */
> struct stack_info stack_info;
> @@ -25,8 +27,22 @@ struct unwind_state {
> 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 +65,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
>
> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
> diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
> new file mode 100644
> index 000000000000..568c6fe707d1
> --- /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) {
> + 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 8ce32c37c587..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) {
> - 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_add(state, state->pc, state->sp);

Do we need to unwind_graph_add again here? unwinder_guess and
unwind_prologue have already been done.

> - 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 d464c533c64f..9677e13c4b4c 100644
> --- a/arch/loongarch/kernel/unwind_prologue.c
> +++ b/arch/loongarch/kernel/unwind_prologue.c
> @@ -9,6 +9,8 @@
> #include <asm/ptrace.h>
> #include <asm/unwind.h>
>
> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
> +
> static inline void unwind_state_fixup(struct unwind_state *state)
> {
> #ifdef CONFIG_DYNAMIC_FTRACE
> @@ -19,31 +21,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 +79,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 +100,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 +109,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) {
> - 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 guard unwinder.
> + */
> + if (!__kernel_text_address(state->pc)) {
> + unwind_register_unwinder(state, guard_unwinder);

Just add a comment here. Instead of using guard_unwinder, can we still
use guess_unwinder?

Thanks,
-Qing

> + 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 +174,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,
> +};
>

2022-12-16 02:03:14

by Jinyang He

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

On 2022-12-15 17:15, Qing Zhang wrote:

> Hi, Jinyang
>
> On 2022/12/15 下午12:01, Jinyang He 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 <[email protected]>
>> ---
>>   arch/loongarch/include/asm/unwind.h     |  22 ++++
>>   arch/loongarch/kernel/Makefile          |   3 +-
>>   arch/loongarch/kernel/unwind.c          |  52 +++++++++
>>   arch/loongarch/kernel/unwind_guess.c    |  41 ++-----
>>   arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
>>   5 files changed, 135 insertions(+), 118 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..a16aff1d086a 100644
>> --- a/arch/loongarch/include/asm/unwind.h
>> +++ b/arch/loongarch/include/asm/unwind.h
>> @@ -18,6 +18,8 @@ enum unwinder_type {
>>       UNWINDER_PROLOGUE,
>>   };
>>   +struct unwinder_ops;
>> +
>>   struct unwind_state {
>>       char type; /* UNWINDER_XXX */
>>       struct stack_info stack_info;
>> @@ -25,8 +27,22 @@ struct unwind_state {
>>       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 +65,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
>>     obj-$(CONFIG_PERF_EVENTS)    += perf_event.o perf_regs.o
>> diff --git a/arch/loongarch/kernel/unwind.c
>> b/arch/loongarch/kernel/unwind.c
>> new file mode 100644
>> index 000000000000..568c6fe707d1
>> --- /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) {
>> +        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 8ce32c37c587..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) {
>> -        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_add(state, state->pc, state->sp);
>
> Do we need to unwind_graph_add again here? unwinder_guess and
> unwind_prologue have already been done.

Hi, Qing


Sorry I don't what meanning here, as here is a delete line.


>
>> -    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 d464c533c64f..9677e13c4b4c 100644
>> --- a/arch/loongarch/kernel/unwind_prologue.c
>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>> @@ -9,6 +9,8 @@
>>   #include <asm/ptrace.h>
>>   #include <asm/unwind.h>
>>   +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>> +
>>   static inline void unwind_state_fixup(struct unwind_state *state)
>>   {
>>   #ifdef CONFIG_DYNAMIC_FTRACE
>> @@ -19,31 +21,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 +79,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 +100,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 +109,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) {
>> -        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 guard unwinder.
>> +     */
>> +    if (!__kernel_text_address(state->pc)) {
>> +        unwind_register_unwinder(state, guard_unwinder);
>
> Just add a comment here. Instead of using guard_unwinder, can we still
> use guess_unwinder?

Yes, and I'll use '&guess_unwinder' in next version. And I'll
drop unwind type in next version, too.


Thanks,

Jinyang

2022-12-16 02:27:31

by Qing Zhang

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

Hi, Jinyang

On 2022/12/16 上午9:40, Jinyang He wrote:
> On 2022-12-15 17:15, Qing Zhang wrote:
>
>> Hi, Jinyang
>>
>> On 2022/12/15 下午12:01, Jinyang He 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 <[email protected]>
>>> ---
>>>   arch/loongarch/include/asm/unwind.h     |  22 ++++
>>>   arch/loongarch/kernel/Makefile          |   3 +-
>>>   arch/loongarch/kernel/unwind.c          |  52 +++++++++
>>>   arch/loongarch/kernel/unwind_guess.c    |  41 ++-----
>>>   arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
>>>   5 files changed, 135 insertions(+), 118 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..a16aff1d086a 100644
>>> --- a/arch/loongarch/include/asm/unwind.h
>>> +++ b/arch/loongarch/include/asm/unwind.h
>>> @@ -18,6 +18,8 @@ enum unwinder_type {
>>>       UNWINDER_PROLOGUE,
>>>   };
>>>   +struct unwinder_ops;
>>> +
>>>   struct unwind_state {
>>>       char type; /* UNWINDER_XXX */
>>>       struct stack_info stack_info;
>>> @@ -25,8 +27,22 @@ struct unwind_state {
>>>       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 +65,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
>>>     obj-$(CONFIG_PERF_EVENTS)    += perf_event.o perf_regs.o
>>> diff --git a/arch/loongarch/kernel/unwind.c
>>> b/arch/loongarch/kernel/unwind.c
>>> new file mode 100644
>>> index 000000000000..568c6fe707d1
>>> --- /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) {
>>> +        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);

here. :)

>>> +    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 8ce32c37c587..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) {
>>> -        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_add(state, state->pc, state->sp);
>>
>> Do we need to unwind_graph_add again here? unwinder_guess and
>> unwind_prologue have already been done.
>
> Hi, Qing
>
>
> Sorry I don't what meanning here, as here is a delete line.
>
Sorry, It's the unwind_start up here.
>
>>
>>> -    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 d464c533c64f..9677e13c4b4c 100644
>>> --- a/arch/loongarch/kernel/unwind_prologue.c
>>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>>> @@ -9,6 +9,8 @@
>>>   #include <asm/ptrace.h>
>>>   #include <asm/unwind.h>
>>>   +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>>> +
>>>   static inline void unwind_state_fixup(struct unwind_state *state)
>>>   {
>>>   #ifdef CONFIG_DYNAMIC_FTRACE
>>> @@ -19,31 +21,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 +79,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 +100,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 +109,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) {
>>> -        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 guard unwinder.
>>> +     */
>>> +    if (!__kernel_text_address(state->pc)) {
>>> +        unwind_register_unwinder(state, guard_unwinder);
>>
>> Just add a comment here. Instead of using guard_unwinder, can we still
>> use guess_unwinder?
>
> Yes, and I'll use '&guess_unwinder' in next version. And I'll
> drop unwind type in next version, too.
>
>
> Thanks,
>
> Jinyang
>

2022-12-17 02:45:33

by Jinyang He

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

On 2022-12-16 10:14, Qing Zhang wrote:

> Hi, Jinyang
>
> On 2022/12/16 上午9:40, Jinyang He wrote:
>> On 2022-12-15 17:15, Qing Zhang wrote:
>>
>>> Hi, Jinyang
>>>
>>> On 2022/12/15 下午12:01, Jinyang He 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 <[email protected]>
>>>> ---
>>>>   arch/loongarch/include/asm/unwind.h     |  22 ++++
>>>>   arch/loongarch/kernel/Makefile          |   3 +-
>>>>   arch/loongarch/kernel/unwind.c          |  52 +++++++++
>>>>   arch/loongarch/kernel/unwind_guess.c    |  41 ++-----
>>>>   arch/loongarch/kernel/unwind_prologue.c | 135
>>>> +++++++++---------------
>>>>   5 files changed, 135 insertions(+), 118 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..a16aff1d086a 100644
>>>> --- a/arch/loongarch/include/asm/unwind.h
>>>> +++ b/arch/loongarch/include/asm/unwind.h
>>>> @@ -18,6 +18,8 @@ enum unwinder_type {
>>>>       UNWINDER_PROLOGUE,
>>>>   };
>>>>   +struct unwinder_ops;
>>>> +
>>>>   struct unwind_state {
>>>>       char type; /* UNWINDER_XXX */
>>>>       struct stack_info stack_info;
>>>> @@ -25,8 +27,22 @@ struct unwind_state {
>>>>       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 +65,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
>>>>     obj-$(CONFIG_PERF_EVENTS)    += perf_event.o perf_regs.o
>>>> diff --git a/arch/loongarch/kernel/unwind.c
>>>> b/arch/loongarch/kernel/unwind.c
>>>> new file mode 100644
>>>> index 000000000000..568c6fe707d1
>>>> --- /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) {
>>>> +        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);
>
>  here. :)

The PC from branch 'task == current' is the RA of current frame. And the
PC from branch 'regs' is regs->csr_era, which may be RA of frame where
to get the regs->csr_era. They all may traced by function_graph.
Ftrace_graph_ret_addr() returns orignal addr and not destroys result, if
the addr was not traced, so calling it for deal with above cases.

Thanks,

Jinyang

>
>>>> + 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 8ce32c37c587..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) {
>>>> -        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_add(state, state->pc, state->sp);
>>>
>>> Do we need to unwind_graph_add again here? unwinder_guess and
>>> unwind_prologue have already been done.
>>
>> Hi, Qing
>>
>>
>> Sorry I don't what meanning here, as here is a delete line.
>>
> Sorry, It's the unwind_start up here.
>>
>>>
>>>> -    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 d464c533c64f..9677e13c4b4c 100644
>>>> --- a/arch/loongarch/kernel/unwind_prologue.c
>>>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>>>> @@ -9,6 +9,8 @@
>>>>   #include <asm/ptrace.h>
>>>>   #include <asm/unwind.h>
>>>>   +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>>>> +
>>>>   static inline void unwind_state_fixup(struct unwind_state *state)
>>>>   {
>>>>   #ifdef CONFIG_DYNAMIC_FTRACE
>>>> @@ -19,31 +21,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 +79,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 +100,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 +109,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) {
>>>> -        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 guard unwinder.
>>>> +     */
>>>> +    if (!__kernel_text_address(state->pc)) {
>>>> +        unwind_register_unwinder(state, guard_unwinder);
>>>
>>> Just add a comment here. Instead of using guard_unwinder, can we still
>>> use guess_unwinder?
>>
>> Yes, and I'll use '&guess_unwinder' in next version. And I'll
>> drop unwind type in next version, too.
>>
>>
>> Thanks,
>>
>> Jinyang
>>

2022-12-17 03:21:10

by Qing Zhang

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

On 2022/12/17 上午9:47, Jinyang He wrote:
> On 2022-12-16 10:14, Qing Zhang wrote:
>
>> Hi, Jinyang
>>
>> On 2022/12/16 上午9:40, Jinyang He wrote:
>>> On 2022-12-15 17:15, Qing Zhang wrote:
>>>
>>>> Hi, Jinyang
>>>>
>>>> On 2022/12/15 下午12:01, Jinyang He 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 <[email protected]>
>>>>> ---
>>>>>   arch/loongarch/include/asm/unwind.h     |  22 ++++
>>>>>   arch/loongarch/kernel/Makefile          |   3 +-
>>>>>   arch/loongarch/kernel/unwind.c          |  52 +++++++++
>>>>>   arch/loongarch/kernel/unwind_guess.c    |  41 ++-----
>>>>>   arch/loongarch/kernel/unwind_prologue.c | 135
>>>>> +++++++++---------------
>>>>>   5 files changed, 135 insertions(+), 118 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..a16aff1d086a 100644
>>>>> --- a/arch/loongarch/include/asm/unwind.h
>>>>> +++ b/arch/loongarch/include/asm/unwind.h
>>>>> @@ -18,6 +18,8 @@ enum unwinder_type {
>>>>>       UNWINDER_PROLOGUE,
>>>>>   };
>>>>>   +struct unwinder_ops;
>>>>> +
>>>>>   struct unwind_state {
>>>>>       char type; /* UNWINDER_XXX */
>>>>>       struct stack_info stack_info;
>>>>> @@ -25,8 +27,22 @@ struct unwind_state {
>>>>>       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 +65,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
>>>>>     obj-$(CONFIG_PERF_EVENTS)    += perf_event.o perf_regs.o
>>>>> diff --git a/arch/loongarch/kernel/unwind.c
>>>>> b/arch/loongarch/kernel/unwind.c
>>>>> new file mode 100644
>>>>> index 000000000000..568c6fe707d1
>>>>> --- /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) {
>>>>> +        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);
>>
>>  here. :)
>
> The PC from branch 'task == current' is the RA of current frame. And the
> PC from branch 'regs' is regs->csr_era, which may be RA of frame where
> to get the regs->csr_era. They all may traced by function_graph.
> Ftrace_graph_ret_addr() returns orignal addr and not destroys result, if
> the addr was not traced, so calling it for deal with above cases.
>
ok, you are right. The first state>pc obtained by cat/proc/self/stack
after the function graph is a "return to handler".

Thank,
-Qing
> Thanks,
>
> Jinyang
>
>>
>>>>> + 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 8ce32c37c587..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) {
>>>>> -        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_add(state, state->pc, state->sp);
>>>>
>>>> Do we need to unwind_graph_add again here? unwinder_guess and
>>>> unwind_prologue have already been done.
>>>
>>> Hi, Qing
>>>
>>>
>>> Sorry I don't what meanning here, as here is a delete line.
>>>
>> Sorry, It's the unwind_start up here.
>>>
>>>>
>>>>> -    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 d464c533c64f..9677e13c4b4c 100644
>>>>> --- a/arch/loongarch/kernel/unwind_prologue.c
>>>>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>>>>> @@ -9,6 +9,8 @@
>>>>>   #include <asm/ptrace.h>
>>>>>   #include <asm/unwind.h>
>>>>>   +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
>>>>> +
>>>>>   static inline void unwind_state_fixup(struct unwind_state *state)
>>>>>   {
>>>>>   #ifdef CONFIG_DYNAMIC_FTRACE
>>>>> @@ -19,31 +21,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 +79,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 +100,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 +109,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) {
>>>>> -        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 guard unwinder.
>>>>> +     */
>>>>> +    if (!__kernel_text_address(state->pc)) {
>>>>> +        unwind_register_unwinder(state, guard_unwinder);
>>>>
>>>> Just add a comment here. Instead of using guard_unwinder, can we still
>>>> use guess_unwinder?
>>>
>>> Yes, and I'll use '&guess_unwinder' in next version. And I'll
>>> drop unwind type in next version, too.
>>>
>>>
>>> Thanks,
>>>
>>> Jinyang
>>>
>

2022-12-19 04:24:16

by Qing Zhang

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

Hi, Jinyang

On 2022/12/15 下午12:01, Jinyang He 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 <[email protected]>
> ---
> arch/loongarch/include/asm/unwind.h | 22 ++++
> arch/loongarch/kernel/Makefile | 3 +-
> arch/loongarch/kernel/unwind.c | 52 +++++++++
> arch/loongarch/kernel/unwind_guess.c | 41 ++-----
> arch/loongarch/kernel/unwind_prologue.c | 135 +++++++++---------------
> 5 files changed, 135 insertions(+), 118 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..a16aff1d086a 100644
> --- a/arch/loongarch/include/asm/unwind.h
> +++ b/arch/loongarch/include/asm/unwind.h
> @@ -18,6 +18,8 @@ enum unwinder_type {
> UNWINDER_PROLOGUE,
> };
>
> +struct unwinder_ops;
> +
> struct unwind_state {
> char type; /* UNWINDER_XXX *
we don't need type anymore.

Thanks,
-Qing
> struct stack_info stack_info;
> @@ -25,8 +27,22 @@ struct unwind_state {
> 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 +65,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
>
> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
> diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
> new file mode 100644
> index 000000000000..568c6fe707d1
> --- /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) {
> + 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 8ce32c37c587..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) {
> - 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 d464c533c64f..9677e13c4b4c 100644
> --- a/arch/loongarch/kernel/unwind_prologue.c
> +++ b/arch/loongarch/kernel/unwind_prologue.c
> @@ -9,6 +9,8 @@
> #include <asm/ptrace.h>
> #include <asm/unwind.h>
>
> +static const struct unwinder_ops *guard_unwinder = &unwinder_guess;
> +
> static inline void unwind_state_fixup(struct unwind_state *state)
> {
> #ifdef CONFIG_DYNAMIC_FTRACE
> @@ -19,31 +21,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 +79,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 +100,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 +109,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) {
> - 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 guard unwinder.
> + */
> + if (!__kernel_text_address(state->pc)) {
> + unwind_register_unwinder(state, guard_unwinder);
> + 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 +174,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,
> +};
>