| Message ID | 1710901169-22763-1-git-send-email-xiaojiangfeng@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: unwind: improve unwinders for noreturn case | expand |
On Wed, Mar 20, 2024 at 10:19:29AM +0800, Jiangfeng Xiao wrote: > This is an off-by-one bug which is common in unwinders, > due to the fact that the address on the stack points > to the return address rather than the call address. > > So, for example, when the last instruction of a function > is a function call (e.g., to a noreturn function), it can > cause the unwinder to incorrectly try to unwind from > the function after the callee. > > foo: > ... > bl bar > ... end of function and thus next function ... > > which results in LR pointing into the next function. > > Fixed this by subtracting 1 from frmae->pc in the call frame > (but not exception frames) like ORC on x86 does. > > Refer to the unwind_next_frame function in the unwind_orc.c > > Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org> > Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/ > Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> > --- > arch/arm/include/asm/stacktrace.h | 4 ---- > arch/arm/kernel/stacktrace.c | 2 -- > arch/arm/kernel/traps.c | 4 ++-- > arch/arm/kernel/unwind.c | 18 +++++++++++++++--- > 4 files changed, 17 insertions(+), 11 deletions(-) > > diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h > index 360f0d2..07e4c16 100644 > --- a/arch/arm/include/asm/stacktrace.h > +++ b/arch/arm/include/asm/stacktrace.h > @@ -21,9 +21,7 @@ struct stackframe { > struct llist_node *kr_cur; > struct task_struct *tsk; > #endif > -#ifdef CONFIG_UNWINDER_FRAME_POINTER > bool ex_frame; > -#endif > }; > > static __always_inline > @@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame) > frame->kr_cur = NULL; > frame->tsk = current; > #endif > -#ifdef CONFIG_UNWINDER_FRAME_POINTER > frame->ex_frame = in_entry_text(frame->pc); > -#endif > } > > extern int unwind_frame(struct stackframe *frame); > diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c > index 620aa82..1abd4f9 100644 > --- a/arch/arm/kernel/stacktrace.c > +++ b/arch/arm/kernel/stacktrace.c > @@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task > frame->kr_cur = NULL; > frame->tsk = task; > #endif > -#ifdef CONFIG_UNWINDER_FRAME_POINTER > frame->ex_frame = in_entry_text(frame->pc); > -#endif > } > > void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 3bad79d..b64e442 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from, > printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n", > loglvl, where, from); > #elif defined CONFIG_BACKTRACE_VERBOSE > - printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n", > + pr_warn("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n", > loglvl, where, (void *)where, from, (void *)from); This should stay printk("%s...", loglvl, ...) or loglvl should be dropped when converting to pr_warn(): pr_warn([<%08lx>] (%ps) from [<%08lx>] (%pB)\n", where, (void *)where, from, (void *)from); Why did you want to force the "warn" log level? > #else > - printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from); > + pr_warn("%s %ps from %pB\n", loglvl, (void *)where, (void *)from); Ditto. -Kees
On 2024/3/20 10:46, Kees Cook wrote: > On Wed, Mar 20, 2024 at 10:19:29AM +0800, Jiangfeng Xiao wrote: >> This is an off-by-one bug which is common in unwinders, >> due to the fact that the address on the stack points >> to the return address rather than the call address. >> >> So, for example, when the last instruction of a function >> is a function call (e.g., to a noreturn function), it can >> cause the unwinder to incorrectly try to unwind from >> the function after the callee. >> >> foo: >> ... >> bl bar >> ... end of function and thus next function ... >> >> which results in LR pointing into the next function. >> >> Fixed this by subtracting 1 from frmae->pc in the call frame >> (but not exception frames) like ORC on x86 does. >> >> Refer to the unwind_next_frame function in the unwind_orc.c >> >> Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org> >> Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/ >> Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> >> --- >> arch/arm/include/asm/stacktrace.h | 4 ---- >> arch/arm/kernel/stacktrace.c | 2 -- >> arch/arm/kernel/traps.c | 4 ++-- >> arch/arm/kernel/unwind.c | 18 +++++++++++++++--- >> 4 files changed, 17 insertions(+), 11 deletions(-) >> >> diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h >> index 360f0d2..07e4c16 100644 >> --- a/arch/arm/include/asm/stacktrace.h >> +++ b/arch/arm/include/asm/stacktrace.h >> @@ -21,9 +21,7 @@ struct stackframe { >> struct llist_node *kr_cur; >> struct task_struct *tsk; >> #endif >> -#ifdef CONFIG_UNWINDER_FRAME_POINTER >> bool ex_frame; >> -#endif >> }; >> >> static __always_inline >> @@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame) >> frame->kr_cur = NULL; >> frame->tsk = current; >> #endif >> -#ifdef CONFIG_UNWINDER_FRAME_POINTER >> frame->ex_frame = in_entry_text(frame->pc); >> -#endif >> } >> >> extern int unwind_frame(struct stackframe *frame); >> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c >> index 620aa82..1abd4f9 100644 >> --- a/arch/arm/kernel/stacktrace.c >> +++ b/arch/arm/kernel/stacktrace.c >> @@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task >> frame->kr_cur = NULL; >> frame->tsk = task; >> #endif >> -#ifdef CONFIG_UNWINDER_FRAME_POINTER >> frame->ex_frame = in_entry_text(frame->pc); >> -#endif >> } >> >> void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c >> index 3bad79d..b64e442 100644 >> --- a/arch/arm/kernel/traps.c >> +++ b/arch/arm/kernel/traps.c >> @@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from, >> printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n", >> loglvl, where, from); >> #elif defined CONFIG_BACKTRACE_VERBOSE >> - printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n", >> + pr_warn("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n", >> loglvl, where, (void *)where, from, (void *)from); > > This should stay printk("%s...", loglvl, ...) or loglvl should be > dropped when converting to pr_warn(): > > pr_warn([<%08lx>] (%ps) from [<%08lx>] (%pB)\n", > where, (void *)where, from, (void *)from); > > Why did you want to force the "warn" log level? > Thank you for your review. I think I'm wrong. The checkpatch.pl script reports the "WARNING: printk() should include KERN_<LEVEL> facility level" warning. That's why I changed printk to pr_warn. I should change printk to printk(KERN_DEFAULT). >> #else >> - printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from); >> + pr_warn("%s %ps from %pB\n", loglvl, (void *)where, (void *)from); > > Ditto. > > -Kees >
On Wed, Mar 20, 2024 at 11:30:13AM +0800, Jiangfeng Xiao wrote: > The checkpatch.pl script reports the "WARNING: printk() should > include KERN_<LEVEL> facility level" warning. > > That's why I changed printk to pr_warn. > I should change printk to printk(KERN_DEFAULT). No, you should ignore checkpatch. For bonus points, figure out why you should ignore it specifically in this case.
On 2024/3/20 11:34, Matthew Wilcox wrote: > On Wed, Mar 20, 2024 at 11:30:13AM +0800, Jiangfeng Xiao wrote: >> The checkpatch.pl script reports the "WARNING: printk() should >> include KERN_<LEVEL> facility level" warning. >> >> That's why I changed printk to pr_warn. >> I should change printk to printk(KERN_DEFAULT). > > No, you should ignore checkpatch. For bonus points, figure out why you > should ignore it specifically in this case. > . > Thank you. I think I understand. The checkpatch.pl file is a false report because the 'loglvl' already exists. I'd better keep printk(" %s...", loglvl, ...) still.
diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h index 360f0d2..07e4c16 100644 --- a/arch/arm/include/asm/stacktrace.h +++ b/arch/arm/include/asm/stacktrace.h @@ -21,9 +21,7 @@ struct stackframe { struct llist_node *kr_cur; struct task_struct *tsk; #endif -#ifdef CONFIG_UNWINDER_FRAME_POINTER bool ex_frame; -#endif }; static __always_inline @@ -37,9 +35,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame) frame->kr_cur = NULL; frame->tsk = current; #endif -#ifdef CONFIG_UNWINDER_FRAME_POINTER frame->ex_frame = in_entry_text(frame->pc); -#endif } extern int unwind_frame(struct stackframe *frame); diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c index 620aa82..1abd4f9 100644 --- a/arch/arm/kernel/stacktrace.c +++ b/arch/arm/kernel/stacktrace.c @@ -154,9 +154,7 @@ static void start_stack_trace(struct stackframe *frame, struct task_struct *task frame->kr_cur = NULL; frame->tsk = task; #endif -#ifdef CONFIG_UNWINDER_FRAME_POINTER frame->ex_frame = in_entry_text(frame->pc); -#endif } void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie, diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 3bad79d..b64e442 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -84,10 +84,10 @@ void dump_backtrace_entry(unsigned long where, unsigned long from, printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n", loglvl, where, from); #elif defined CONFIG_BACKTRACE_VERBOSE - printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n", + pr_warn("%s[<%08lx>] (%ps) from [<%08lx>] (%pB)\n", loglvl, where, (void *)where, from, (void *)from); #else - printk("%s %ps from %pS\n", loglvl, (void *)where, (void *)from); + pr_warn("%s %ps from %pB\n", loglvl, (void *)where, (void *)from); #endif if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE)) diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c index 9d21921..f2baf92 100644 --- a/arch/arm/kernel/unwind.c +++ b/arch/arm/kernel/unwind.c @@ -30,6 +30,7 @@ #include <linux/list.h> #include <linux/module.h> +#include <asm/sections.h> #include <asm/stacktrace.h> #include <asm/traps.h> #include <asm/unwind.h> @@ -416,8 +417,14 @@ int unwind_frame(struct stackframe *frame) pr_debug("%s(pc = %08lx lr = %08lx sp = %08lx)\n", __func__, frame->pc, frame->lr, frame->sp); - - idx = unwind_find_idx(frame->pc); + /* + * For a call frame (as opposed to a exception frame), when the last + * instruction of a function is a function call (e.g., to a noreturn + * function), it can cause the unwinder incorrectly try to unwind + * from the function after the callee, fixed this by subtracting 1 + * from frame->pc in the call frame like ORC on x86 does. + */ + idx = unwind_find_idx(frame->ex_frame ? frame->pc : frame->pc - 1); if (!idx) { if (frame->pc && kernel_text_address(frame->pc)) { if (in_module_plt(frame->pc) && frame->pc != frame->lr) { @@ -427,6 +434,7 @@ int unwind_frame(struct stackframe *frame) * the state of the stack or the register file */ frame->pc = frame->lr; + frame->ex_frame = in_entry_text(frame->pc); return URC_OK; } pr_warn("unwind: Index not found %08lx\n", frame->pc); @@ -454,6 +462,7 @@ int unwind_frame(struct stackframe *frame) if (frame->pc == frame->lr) return -URC_FAILURE; frame->pc = frame->lr; + frame->ex_frame = in_entry_text(frame->pc); return URC_OK; } else if ((idx->insn & 0x80000000) == 0) /* prel31 to the unwind table */ @@ -515,6 +524,7 @@ int unwind_frame(struct stackframe *frame) frame->lr = ctrl.vrs[LR]; frame->pc = ctrl.vrs[PC]; frame->lr_addr = ctrl.lr_addr; + frame->ex_frame = in_entry_text(frame->pc); return URC_OK; } @@ -544,6 +554,7 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk, */ here: frame.pc = (unsigned long)&&here; + frame.ex_frame = false; } else { /* task blocked in __switch_to */ frame.fp = thread_saved_fp(tsk); @@ -554,11 +565,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk, */ frame.lr = 0; frame.pc = thread_saved_pc(tsk); + frame.ex_frame = false; } while (1) { int urc; - unsigned long where = frame.pc; + unsigned long where = frame.ex_frame ? frame.pc : frame.pc - 1; urc = unwind_frame(&frame); if (urc < 0)
This is an off-by-one bug which is common in unwinders, due to the fact that the address on the stack points to the return address rather than the call address. So, for example, when the last instruction of a function is a function call (e.g., to a noreturn function), it can cause the unwinder to incorrectly try to unwind from the function after the callee. foo: ... bl bar ... end of function and thus next function ... which results in LR pointing into the next function. Fixed this by subtracting 1 from frmae->pc in the call frame (but not exception frames) like ORC on x86 does. Refer to the unwind_next_frame function in the unwind_orc.c Suggested-by: Josh Poimboeuf <jpoimboe@kernel.org> Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/ Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@huawei.com> --- arch/arm/include/asm/stacktrace.h | 4 ---- arch/arm/kernel/stacktrace.c | 2 -- arch/arm/kernel/traps.c | 4 ++-- arch/arm/kernel/unwind.c | 18 +++++++++++++++--- 4 files changed, 17 insertions(+), 11 deletions(-)