| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 3 Jan 2023 16:40:56 +0800
From: Jinyang He <hejinyang@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: WANG Xuerui <kernel@...0n.name>,
Qing Zhang <zhangqing@...ngson.cn>, loongarch@...ts.linux.dev,
linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v2 4/6] LoongArch: Strip guess_unwinder out from
prologue_unwinder
On 2023-01-01 17:56, Huacai Chen wrote:
> Hi, Jinyang,
>
> On Thu, Dec 29, 2022 at 2:15 PM Jinyang He <hejinyang@...ngson.cn> wrote:
>> The prolugue unwinder rely on symbol info. When PC is not in kernel
>> text address, it cannot find relative symbol info and it will be broken.
>> The guess unwinder will be used in this case. And the guess unwinder
>> codes in prolugue unwinder is redundant. Strip it out and set the
>> unwinder info in unwind_state.
>>
>> Signed-off-by: Jinyang He <hejinyang@...ngson.cn>
>> ---
>> arch/loongarch/include/asm/unwind.h | 26 ++++-
>> arch/loongarch/kernel/Makefile | 3 +-
>> arch/loongarch/kernel/traps.c | 3 -
>> arch/loongarch/kernel/unwind.c | 52 +++++++++
>> arch/loongarch/kernel/unwind_guess.c | 41 ++------
>> arch/loongarch/kernel/unwind_prologue.c | 133 +++++++++---------------
>> 6 files changed, 132 insertions(+), 126 deletions(-)
>> create mode 100644 arch/loongarch/kernel/unwind.c
>>
>> diff --git a/arch/loongarch/include/asm/unwind.h b/arch/loongarch/include/asm/unwind.h
>> index 6ece48f0ff77..4a55fd7b77ad 100644
>> --- a/arch/loongarch/include/asm/unwind.h
>> +++ b/arch/loongarch/include/asm/unwind.h
>> @@ -13,20 +13,30 @@
>> #include <asm/ptrace.h>
>> #include <asm/stacktrace.h>
>>
>> -enum unwinder_type {
>> - UNWINDER_GUESS,
>> - UNWINDER_PROLOGUE,
>> -};
>> +struct unwinder_ops;
>>
>> struct unwind_state {
>> - char type; /* UNWINDER_XXX */
>> struct stack_info stack_info;
>> struct task_struct *task;
>> bool first, error, is_ftrace;
>> int graph_idx;
>> unsigned long sp, pc, ra;
>> + const struct unwinder_ops *ops;
>> +};
>> +
>> +struct unwinder_ops {
>> + void (*unwind_start)(struct unwind_state *state,
>> + struct task_struct *task, struct pt_regs *regs);
>> + bool (*unwind_next_frame)(struct unwind_state *state);
>> + unsigned long (*unwind_get_return_address)(struct unwind_state *state);
>> };
>>
>> +extern const struct unwinder_ops *default_unwinder;
>> +extern const struct unwinder_ops unwinder_guess;
>> +#ifdef CONFIG_UNWINDER_PROLOGUE
>> +extern const struct unwinder_ops unwinder_prologue;
>> +#endif
>> +
>> void unwind_start(struct unwind_state *state,
>> struct task_struct *task, struct pt_regs *regs);
>> bool unwind_next_frame(struct unwind_state *state);
>> @@ -49,4 +59,10 @@ static inline unsigned long unwind_graph_addr(struct unwind_state *state,
>> return ftrace_graph_ret_addr(state->task, &state->graph_idx,
>> pc, (unsigned long *)(cfa - GRAPH_FAKE_OFFSET));
>> }
>> +
>> +static inline void unwind_register_unwinder(struct unwind_state *state,
>> + const struct unwinder_ops *unwinder)
>> +{
>> + state->ops = unwinder;
>> +}
>> #endif /* _ASM_UNWIND_H */
>> diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
>> index 7ca65195f7f8..cb6029ea3ea9 100644
>> --- a/arch/loongarch/kernel/Makefile
>> +++ b/arch/loongarch/kernel/Makefile
>> @@ -8,7 +8,7 @@ extra-y := vmlinux.lds
>> obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o entry.o genex.o \
>> traps.o irq.o idle.o process.o dma.o mem.o io.o reset.o switch.o \
>> elf.o syscall.o signal.o time.o topology.o inst.o ptrace.o vdso.o \
>> - alternative.o unaligned.o
>> + alternative.o unaligned.o unwind.o unwind_guess.o
>>
>> obj-$(CONFIG_ACPI) += acpi.o
>> obj-$(CONFIG_EFI) += efi.o
>> @@ -42,7 +42,6 @@ obj-$(CONFIG_MAGIC_SYSRQ) += sysrq.o
>> obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
>> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>>
>> -obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>> obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o
> We do have CONFIG_UNWINDER_GUESS and I don't like to build guess
> unwinder unconditionally, maybe we can refactor it to make the
> unwinder still user selectable.
In order to provide backtrace info to the user, prologue unwinder
will use guess unwinder when its first pc is not a kernel address,
such as the first PC is in ebpf generated codes. This patch is intended
to extract guess unwinder codes to avoid redundant codes. And if we provide
a new unwinder in the future like orc unwinder which take effect after
orc_init is set, we can use another backup unwinder like guess unwinder in
case we need backtrace before orc_init being set.
If you mind this, could we just do as follows,
obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
obj-$(CONFIG_UNWINDER_PROLOGUE) += unwind_prologue.o unwind_guess.o
Or copy the guess unwinder codes to unwind_prologue.c and set another
name for guess unwinder.
Thanks,
Jinyang
> Huacai
>> obj-$(CONFIG_PERF_EVENTS) += perf_event.o perf_regs.o
>> diff --git a/arch/loongarch/kernel/traps.c b/arch/loongarch/kernel/traps.c
>> index 3adc44832205..1ea14f6c18d3 100644
>> --- a/arch/loongarch/kernel/traps.c
>> +++ b/arch/loongarch/kernel/traps.c
>> @@ -72,9 +72,6 @@ static void show_backtrace(struct task_struct *task, const struct pt_regs *regs,
>> if (!task)
>> task = current;
>>
>> - if (user_mode(regs))
>> - state.type = UNWINDER_GUESS;
>> -
>> printk("%sCall Trace:", loglvl);
>> for (unwind_start(&state, task, pregs);
>> !unwind_done(&state); unwind_next_frame(&state)) {
>> diff --git a/arch/loongarch/kernel/unwind.c b/arch/loongarch/kernel/unwind.c
>> new file mode 100644
>> index 000000000000..24d2cf99bfb6
>> --- /dev/null
>> +++ b/arch/loongarch/kernel/unwind.c
>> @@ -0,0 +1,52 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2022 Loongson Technology Corporation Limited
>> + */
>> +#include <asm/unwind.h>
>> +
>> +#if defined(CONFIG_UNWINDER_GUESS)
>> +const struct unwinder_ops *default_unwinder = &unwinder_guess;
>> +#elif defined(CONFIG_UNWINDER_PROLOGUE)
>> +const struct unwinder_ops *default_unwinder = &unwinder_prologue;
>> +#endif
>> +
>> +unsigned long unwind_get_return_address(struct unwind_state *state)
>> +{
>> + if (!state->ops || unwind_done(state))
>> + return 0;
>> + return state->ops->unwind_get_return_address(state);
>> +}
>> +EXPORT_SYMBOL_GPL(unwind_get_return_address);
>> +
>> +void unwind_start(struct unwind_state *state, struct task_struct *task,
>> + struct pt_regs *regs)
>> +{
>> + memset(state, 0, sizeof(*state));
>> + unwind_register_unwinder(state, default_unwinder);
>> + if (regs) {
>> + state->sp = regs->regs[3];
>> + state->pc = regs->csr_era;
>> + state->ra = regs->regs[1];
>> + } else if (task == current || task == NULL) {
>> + state->sp = (unsigned long)__builtin_frame_address(0);
>> + state->pc = (unsigned long)__builtin_return_address(0);
>> + state->ra = 0;
>> + } else {
>> + state->sp = thread_saved_fp(task);
>> + state->pc = thread_saved_ra(task);
>> + state->ra = 0;
>> + }
>> + state->task = task;
>> + get_stack_info(state->sp, state->task, &state->stack_info);
>> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
>> + state->ops->unwind_start(state, task, regs);
>> +}
>> +EXPORT_SYMBOL_GPL(unwind_start);
>> +
>> +bool unwind_next_frame(struct unwind_state *state)
>> +{
>> + if (!state->ops || unwind_done(state))
>> + return false;
>> + return state->ops->unwind_next_frame(state);
>> +}
>> +EXPORT_SYMBOL_GPL(unwind_next_frame);
>> diff --git a/arch/loongarch/kernel/unwind_guess.c b/arch/loongarch/kernel/unwind_guess.c
>> index 935d24f8c95c..b7ca2b88ac63 100644
>> --- a/arch/loongarch/kernel/unwind_guess.c
>> +++ b/arch/loongarch/kernel/unwind_guess.c
>> @@ -7,51 +7,23 @@
>>
>> #include <asm/unwind.h>
>>
>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>> +static unsigned long get_return_address(struct unwind_state *state)
>> {
>> - if (unwind_done(state))
>> - return 0;
>> return state->pc;
>> }
>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>>
>> -void unwind_start(struct unwind_state *state, struct task_struct *task,
>> +static void start(struct unwind_state *state, struct task_struct *task,
>> struct pt_regs *regs)
>> {
>> - memset(state, 0, sizeof(*state));
>> -
>> - if (regs) {
>> - state->sp = regs->regs[3];
>> - state->pc = regs->csr_era;
>> - } else if (task == current || task == NULL) {
>> - state->sp = (unsigned long)__builtin_frame_address(0);
>> - state->pc = (unsigned long)__builtin_return_address(0);
>> - } else {
>> - state->sp = thread_saved_fp(task);
>> - state->pc = thread_saved_ra(task);
>> - }
>> -
>> - state->task = task;
>> - state->first = true;
>> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
>> - get_stack_info(state->sp, state->task, &state->stack_info);
>> -
>> if (!unwind_done(state) && !__kernel_text_address(state->pc))
>> unwind_next_frame(state);
>> }
>> -EXPORT_SYMBOL_GPL(unwind_start);
>>
>> -bool unwind_next_frame(struct unwind_state *state)
>> +static bool next_frame(struct unwind_state *state)
>> {
>> struct stack_info *info = &state->stack_info;
>> unsigned long addr;
>>
>> - if (unwind_done(state))
>> - return false;
>> -
>> - if (state->first)
>> - state->first = false;
>> -
>> do {
>> for (state->sp += sizeof(unsigned long);
>> state->sp < info->end;
>> @@ -68,4 +40,9 @@ bool unwind_next_frame(struct unwind_state *state)
>>
>> return false;
>> }
>> -EXPORT_SYMBOL_GPL(unwind_next_frame);
>> +
>> +const struct unwinder_ops unwinder_guess = {
>> + .unwind_start = start,
>> + .unwind_next_frame = next_frame,
>> + .unwind_get_return_address = get_return_address,
>> +};
>> diff --git a/arch/loongarch/kernel/unwind_prologue.c b/arch/loongarch/kernel/unwind_prologue.c
>> index f7994ed05f04..beb57ea24da2 100644
>> --- a/arch/loongarch/kernel/unwind_prologue.c
>> +++ b/arch/loongarch/kernel/unwind_prologue.c
>> @@ -19,31 +19,19 @@ static inline void unwind_state_fixup(struct unwind_state *state)
>> #endif
>> }
>>
>> -unsigned long unwind_get_return_address(struct unwind_state *state)
>> +static unsigned long get_return_address(struct unwind_state *state)
>> {
>> - if (unwind_done(state))
>> - return 0;
>> return state->pc;
>> }
>> -EXPORT_SYMBOL_GPL(unwind_get_return_address);
>> -
>> -static bool unwind_by_guess(struct unwind_state *state)
>> -{
>> - struct stack_info *info = &state->stack_info;
>> - unsigned long addr;
>> -
>> - for (state->sp += sizeof(unsigned long);
>> - state->sp < info->end;
>> - state->sp += sizeof(unsigned long)) {
>> - addr = *(unsigned long *)(state->sp);
>> - state->pc = unwind_graph_addr(state, addr, state->sp + 8);
>> - if (__kernel_text_address(state->pc))
>> - return true;
>> - }
>> -
>> - return false;
>> -}
>>
>> +/*
>> + * LoongArch function prologue like follows,
>> + * [others instructions not use stack var]
>> + * addi.d sp, sp, -imm
>> + * st.d xx, sp, offset <- save callee saved regs and
>> + * st.d yy, sp, offset save ra if function is nest.
>> + * [others instructions]
>> + */
>> static bool unwind_by_prologue(struct unwind_state *state)
>> {
>> long frame_ra = -1;
>> @@ -89,6 +77,10 @@ static bool unwind_by_prologue(struct unwind_state *state)
>> ip++;
>> }
>>
>> + /*
>> + * Not find stack alloc action, PC may be in a leaf function. Only the
>> + * first being true is reasonable, otherwise indicate analysis is broken.
>> + */
>> if (!frame_size) {
>> if (state->first)
>> goto first;
>> @@ -106,6 +98,7 @@ static bool unwind_by_prologue(struct unwind_state *state)
>> ip++;
>> }
>>
>> + /* Not find save $ra action, PC may be in a leaf function, too. */
>> if (frame_ra < 0) {
>> if (state->first) {
>> state->sp = state->sp + frame_size;
>> @@ -114,96 +107,63 @@ static bool unwind_by_prologue(struct unwind_state *state)
>> return false;
>> }
>>
>> - if (state->first)
>> - state->first = false;
>> -
>> state->pc = *(unsigned long *)(state->sp + frame_ra);
>> state->sp = state->sp + frame_size;
>> goto out;
>>
>> first:
>> - state->first = false;
>> - if (state->pc == state->ra)
>> - return false;
>> -
>> state->pc = state->ra;
>>
>> out:
>> + state->first = false;
>> unwind_state_fixup(state);
>> return !!__kernel_text_address(state->pc);
>> }
>>
>> -void unwind_start(struct unwind_state *state, struct task_struct *task,
>> +static void start(struct unwind_state *state, struct task_struct *task,
>> struct pt_regs *regs)
>> {
>> - memset(state, 0, sizeof(*state));
>> - state->type = UNWINDER_PROLOGUE;
>> -
>> - if (regs) {
>> - state->sp = regs->regs[3];
>> - state->pc = regs->csr_era;
>> - state->ra = regs->regs[1];
>> - if (!__kernel_text_address(state->pc))
>> - state->type = UNWINDER_GUESS;
>> - } else if (task == current || task == NULL) {
>> - state->sp = (unsigned long)__builtin_frame_address(0);
>> - state->pc = (unsigned long)__builtin_return_address(0);
>> - state->ra = 0;
>> - } else {
>> - state->sp = thread_saved_fp(task);
>> - state->pc = thread_saved_ra(task);
>> - state->ra = 0;
>> - }
>> -
>> - state->task = task;
>> state->first = true;
>> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
>> - get_stack_info(state->sp, state->task, &state->stack_info);
>>
>> - if (!unwind_done(state) && !__kernel_text_address(state->pc))
>> - unwind_next_frame(state);
>> + /*
>> + * The current PC is not kernel text address, we cannot find its
>> + * relative symbol. Thus, prologue analysis will be broken. Luckly,
>> + * we can use the guess unwinder.
>> + */
>> + if (!__kernel_text_address(state->pc)) {
>> + unwind_register_unwinder(state, &unwinder_guess);
>> + if (!unwind_done(state))
>> + unwind_next_frame(state);
>> + }
>> }
>> -EXPORT_SYMBOL_GPL(unwind_start);
>>
>> -bool unwind_next_frame(struct unwind_state *state)
>> +static bool next_frame(struct unwind_state *state)
>> {
>> struct stack_info *info = &state->stack_info;
>> struct pt_regs *regs;
>> unsigned long pc;
>>
>> - if (unwind_done(state))
>> - return false;
>> -
>> do {
>> - switch (state->type) {
>> - case UNWINDER_GUESS:
>> - state->first = false;
>> - if (unwind_by_guess(state))
>> - return true;
>> - break;
>> -
>> - case UNWINDER_PROLOGUE:
>> - if (unwind_by_prologue(state)) {
>> - state->pc = unwind_graph_addr(state, state->pc, state->sp);
>> - return true;
>> - }
>> + if (unwind_by_prologue(state)) {
>> + state->pc = unwind_graph_addr(state, state->pc, state->sp);
>> + return true;
>> + }
>>
>> - if (info->type == STACK_TYPE_IRQ &&
>> - info->end == state->sp) {
>> - regs = (struct pt_regs *)info->next_sp;
>> - pc = regs->csr_era;
>> + if (info->type == STACK_TYPE_IRQ &&
>> + info->end == state->sp) {
>> + regs = (struct pt_regs *)info->next_sp;
>> + pc = regs->csr_era;
>>
>> - if (user_mode(regs) || !__kernel_text_address(pc))
>> - return false;
>> + if (user_mode(regs) || !__kernel_text_address(pc))
>> + return false;
>>
>> - state->first = true;
>> - state->ra = regs->regs[1];
>> - state->sp = regs->regs[3];
>> - state->pc = pc;
>> - get_stack_info(state->sp, state->task, info);
>> + state->first = true;
>> + state->ra = regs->regs[1];
>> + state->sp = regs->regs[3];
>> + state->pc = pc;
>> + get_stack_info(state->sp, state->task, info);
>>
>> - return true;
>> - }
>> + return true;
>> }
>>
>> state->sp = info->next_sp;
>> @@ -212,4 +172,9 @@ bool unwind_next_frame(struct unwind_state *state)
>>
>> return false;
>> }
>> -EXPORT_SYMBOL_GPL(unwind_next_frame);
>> +
>> +const struct unwinder_ops unwinder_prologue = {
>> + .unwind_start = start,
>> + .unwind_next_frame = next_frame,
>> + .unwind_get_return_address = get_return_address,
>> +};
>> --
>> 2.34.3
>>
>>
Powered by blists - more mailing lists