On Wed, Mar 20, 2024 at 11:44:38AM +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.
The reason that I'm not accepting this patch is because the above says that it fixes it by subtracting 1 from the PC value, but the patch is *way* more complicated than that and there's no explanation why. For example, the following are unexplained: - Why do we always need ex_frame - What is the purpose of the change in format string for the display of backtraces > > Refer to the unwind_next_frame function in the unwind_orc.c > > Suggested-by: Josh Poimboeuf <[email protected]> > Link: https://lkml.kernel.org/lkml/20240305175846.qnyiru7uaa7itqba@treble/ > Signed-off-by: Jiangfeng Xiao <[email protected]> > --- > ChangeLog v1->v2 > - stay printk("%s...", loglvl, ...) > --- > 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..46a5b1e 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", > + printk("%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); > + printk("%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) > -- > 1.8.5.6 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
