| Message ID | 20220712021527.109921-3-lihuafei1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Convert to ARCH_STACKWALK | expand |
On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote: > Because an exception stack frame is not created in the exception entry, > save_trace() does special handling for the exception PC, but this is > only needed when CONFIG_FRAME_POINTER_UNWIND=y. When > CONFIG_ARM_UNWIND=y, unwind annotations have been added to the exception > entry and save_trace() will repeatedly save the exception PC: > > [0x7f000090] hrtimer_hander+0x8/0x10 [hrtimer] > [0x8019ec50] __hrtimer_run_queues+0x18c/0x394 > [0x8019f760] hrtimer_run_queues+0xbc/0xd0 > [0x8019def0] update_process_times+0x34/0x80 > [0x801ad2a4] tick_periodic+0x48/0xd0 > [0x801ad3dc] tick_handle_periodic+0x1c/0x7c > [0x8010f2e0] twd_handler+0x30/0x40 > [0x80177620] handle_percpu_devid_irq+0xa0/0x23c > [0x801718d0] generic_handle_domain_irq+0x24/0x34 > [0x80502d28] gic_handle_irq+0x74/0x88 > [0x8085817c] generic_handle_arch_irq+0x58/0x78 > [0x80100ba8] __irq_svc+0x88/0xc8 > [0x80108114] arch_cpu_idle+0x38/0x3c > [0x80108114] arch_cpu_idle+0x38/0x3c <==== duplicate saved exception PC > [0x80861bf8] default_idle_call+0x38/0x130 > [0x8015d5cc] do_idle+0x150/0x214 > [0x8015d978] cpu_startup_entry+0x18/0x1c > [0x808589c0] rest_init+0xd8/0xdc > [0x80c00a44] arch_post_acpi_subsys_init+0x0/0x8 > > We can move the special handling of the exception PC in save_trace() to > the unwind_frame() of the frame pointer unwinder. > > Signed-off-by: Li Huafei <lihuafei1@huawei.com> This is another very nice patch! Reviewed-by: Linus Waleij <linus.walleij@linaro.org> Nitpick: > + if ((unsigned long)®s[1] > ALIGN(frame->sp, THREAD_SIZE)) > + return -EINVAL; It'd be nice to add a comment saying what is in regs[1] at this point so it is easier to understand the code. Not your fault as it is just moved code, but if you have time please add a small comment. Yours, Linus Walleij
On 2022/7/18 17:01, Linus Walleij wrote: > On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote: > >> Because an exception stack frame is not created in the exception entry, >> save_trace() does special handling for the exception PC, but this is >> only needed when CONFIG_FRAME_POINTER_UNWIND=y. When >> CONFIG_ARM_UNWIND=y, unwind annotations have been added to the exception >> entry and save_trace() will repeatedly save the exception PC: >> >> [0x7f000090] hrtimer_hander+0x8/0x10 [hrtimer] >> [0x8019ec50] __hrtimer_run_queues+0x18c/0x394 >> [0x8019f760] hrtimer_run_queues+0xbc/0xd0 >> [0x8019def0] update_process_times+0x34/0x80 >> [0x801ad2a4] tick_periodic+0x48/0xd0 >> [0x801ad3dc] tick_handle_periodic+0x1c/0x7c >> [0x8010f2e0] twd_handler+0x30/0x40 >> [0x80177620] handle_percpu_devid_irq+0xa0/0x23c >> [0x801718d0] generic_handle_domain_irq+0x24/0x34 >> [0x80502d28] gic_handle_irq+0x74/0x88 >> [0x8085817c] generic_handle_arch_irq+0x58/0x78 >> [0x80100ba8] __irq_svc+0x88/0xc8 >> [0x80108114] arch_cpu_idle+0x38/0x3c >> [0x80108114] arch_cpu_idle+0x38/0x3c <==== duplicate saved exception PC >> [0x80861bf8] default_idle_call+0x38/0x130 >> [0x8015d5cc] do_idle+0x150/0x214 >> [0x8015d978] cpu_startup_entry+0x18/0x1c >> [0x808589c0] rest_init+0xd8/0xdc >> [0x80c00a44] arch_post_acpi_subsys_init+0x0/0x8 >> >> We can move the special handling of the exception PC in save_trace() to >> the unwind_frame() of the frame pointer unwinder. >> >> Signed-off-by: Li Huafei <lihuafei1@huawei.com> > > This is another very nice patch! > Reviewed-by: Linus Waleij <linus.walleij@linaro.org> > > Nitpick: > >> + if ((unsigned long)®s[1] > ALIGN(frame->sp, THREAD_SIZE)) >> + return -EINVAL; > > It'd be nice to add a comment saying what is in regs[1] at this point > so it is easier to understand the code. Not your fault as it is just > moved code, but if you have time please add a small comment. > It is necessary to add the comment. This check is to ensure that 'regs + sizeof(struct pt_regs)' (that is, ®s[1]) does not go beyond the bottom of the stack, to avoid accessing data outside the task's stack, see commit 40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in stacktrace") for details. I will add a comment in the next version. Thanks, Huafei > Yours, > Linus Walleij > . >
On Tue, Jul 12, 2022 at 10:15:24AM +0800, Li Huafei wrote: > @@ -34,6 +37,9 @@ 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) ? true : false; in_entry_text() returns a bool, so there's no need for the ternary operator. The same comment applies throughout this patch.
On 2022/7/26 18:22, Russell King (Oracle) wrote: > On Tue, Jul 12, 2022 at 10:15:24AM +0800, Li Huafei wrote: >> @@ -34,6 +37,9 @@ 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) ? true : false; > > in_entry_text() returns a bool, so there's no need for the ternary > operator. The same comment applies throughout this patch. > OK, I will fix it in v3. Thanks, Huafei
diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h index 3e78f921b8b2..25282ff645fb 100644 --- a/arch/arm/include/asm/stacktrace.h +++ b/arch/arm/include/asm/stacktrace.h @@ -21,6 +21,9 @@ struct stackframe { struct llist_node *kr_cur; struct task_struct *tsk; #endif +#ifdef CONFIG_UNWINDER_FRAME_POINTER + bool ex_frame; +#endif }; static __always_inline @@ -34,6 +37,9 @@ 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) ? true : false; +#endif } extern int unwind_frame(struct stackframe *frame); diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c index 8aac1e10b117..38f1ea9c724d 100644 --- a/arch/arm/kernel/return_address.c +++ b/arch/arm/kernel/return_address.c @@ -47,6 +47,7 @@ void *return_address(unsigned int level) frame.kr_cur = NULL; frame.tsk = current; #endif + frame.ex_frame = false; walk_stackframe(&frame, save_return_addr, &data); diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c index af87040b0353..3acf51ee46bb 100644 --- a/arch/arm/kernel/stacktrace.c +++ b/arch/arm/kernel/stacktrace.c @@ -82,6 +82,21 @@ int notrace unwind_frame(struct stackframe *frame) if (frame_pointer_check(frame)) return -EINVAL; + /* + * When we unwind through an exception stack, include the saved PC + * value into the stack trace. + */ + if (frame->ex_frame) { + struct pt_regs *regs = (struct pt_regs *)frame->sp; + + if ((unsigned long)®s[1] > ALIGN(frame->sp, THREAD_SIZE)) + return -EINVAL; + + frame->pc = regs->ARM_pc; + frame->ex_frame = false; + return 0; + } + /* restore the registers from the stack frame */ #ifdef CONFIG_CC_IS_CLANG frame->sp = frame->fp; @@ -98,6 +113,9 @@ int notrace unwind_frame(struct stackframe *frame) (void *)frame->fp, &frame->kr_cur); #endif + if (in_entry_text(frame->pc)) + frame->ex_frame = true; + return 0; } #endif @@ -128,7 +146,6 @@ static int save_trace(struct stackframe *frame, void *d) { struct stack_trace_data *data = d; struct stack_trace *trace = data->trace; - struct pt_regs *regs; unsigned long addr = frame->pc; if (data->no_sched_functions && in_sched_functions(addr)) @@ -139,19 +156,6 @@ static int save_trace(struct stackframe *frame, void *d) } trace->entries[trace->nr_entries++] = addr; - - if (trace->nr_entries >= trace->max_entries) - return 1; - - if (!in_entry_text(frame->pc)) - return 0; - - regs = (struct pt_regs *)frame->sp; - if ((unsigned long)®s[1] > ALIGN(frame->sp, THREAD_SIZE)) - return 0; - - trace->entries[trace->nr_entries++] = regs->ARM_pc; - return trace->nr_entries >= trace->max_entries; } @@ -193,6 +197,9 @@ static noinline void __save_stack_trace(struct task_struct *tsk, frame.kr_cur = NULL; frame.tsk = tsk; #endif +#ifdef CONFIG_UNWINDER_FRAME_POINTER + frame.ex_frame = false; +#endif walk_stackframe(&frame, save_trace, &data); } @@ -214,6 +221,9 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace) frame.kr_cur = NULL; frame.tsk = current; #endif +#ifdef CONFIG_UNWINDER_FRAME_POINTER + frame.ex_frame = in_entry_text(frame.pc) ? true : false; +#endif walk_stackframe(&frame, save_trace, &data); }
Because an exception stack frame is not created in the exception entry, save_trace() does special handling for the exception PC, but this is only needed when CONFIG_FRAME_POINTER_UNWIND=y. When CONFIG_ARM_UNWIND=y, unwind annotations have been added to the exception entry and save_trace() will repeatedly save the exception PC: [0x7f000090] hrtimer_hander+0x8/0x10 [hrtimer] [0x8019ec50] __hrtimer_run_queues+0x18c/0x394 [0x8019f760] hrtimer_run_queues+0xbc/0xd0 [0x8019def0] update_process_times+0x34/0x80 [0x801ad2a4] tick_periodic+0x48/0xd0 [0x801ad3dc] tick_handle_periodic+0x1c/0x7c [0x8010f2e0] twd_handler+0x30/0x40 [0x80177620] handle_percpu_devid_irq+0xa0/0x23c [0x801718d0] generic_handle_domain_irq+0x24/0x34 [0x80502d28] gic_handle_irq+0x74/0x88 [0x8085817c] generic_handle_arch_irq+0x58/0x78 [0x80100ba8] __irq_svc+0x88/0xc8 [0x80108114] arch_cpu_idle+0x38/0x3c [0x80108114] arch_cpu_idle+0x38/0x3c <==== duplicate saved exception PC [0x80861bf8] default_idle_call+0x38/0x130 [0x8015d5cc] do_idle+0x150/0x214 [0x8015d978] cpu_startup_entry+0x18/0x1c [0x808589c0] rest_init+0xd8/0xdc [0x80c00a44] arch_post_acpi_subsys_init+0x0/0x8 We can move the special handling of the exception PC in save_trace() to the unwind_frame() of the frame pointer unwinder. Signed-off-by: Li Huafei <lihuafei1@huawei.com> --- arch/arm/include/asm/stacktrace.h | 6 +++++ arch/arm/kernel/return_address.c | 1 + arch/arm/kernel/stacktrace.c | 38 +++++++++++++++++++------------ 3 files changed, 31 insertions(+), 14 deletions(-)