Messages in this thread Patch in this message | | | From | Nick Desaulniers <> | Date | Mon, 11 Oct 2021 11:45:22 -0700 | Subject | Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace |
| |
On Fri, Oct 8, 2021 at 5:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > Currently the stacktrace on clang compiled arm kernel uses the 'lr' > register to find the first frame address from pt_regs. However, that > is wrong after calling another function, because the 'lr' register > is used by 'bl' instruction and never be recovered. > > As same as gcc arm kernel, directly use the frame pointer (x11) of > the pt_regs to find the first frame address.
Hi Masami, Thanks for the patch. Testing with ARCH=arm defconfig (multi_v7_defconfig)
Before this patch:
$ mount -t proc /proc $ echo 0 > /proc/sys/kernel/kptr_restrict $ cat /proc/self/stack [<0>] proc_single_show+0x4c/0xb8 [<0>] seq_read_iter+0x174/0x4d8 [<0>] seq_read+0x134/0x158 [<0>] vfs_read+0xcc/0x2f8 [<0>] ksys_read+0x74/0xd0 [<0>] __entry_text_start+0x14/0x14 [<0>] 0xbea38cc0
After this patch: $ mount -t proc /proc $ echo 0 > /proc/sys/kernel/kptr_restrict $ cat /proc/self/stack [<0>] proc_single_show+0x4c/0xb8 [<0>] seq_read_iter+0x174/0x4d8 [<0>] seq_read+0x134/0x158 [<0>] vfs_read+0xcc/0x2f8 [<0>] ksys_read+0x74/0xd0 [<0>] __entry_text_start+0x14/0x14 [<0>] 0xbeb55cc0
Is there a different way to test/verify this patch? (I'm pretty sure we had verified the WARN_ONCE functionality with this, too.)
If I change from CONFIG_UNWINDER_ARM=y to CONFIG_UNWINDER_FRAME_POINTER=y, before:
# cat /proc/self/stack [<0>] stack_trace_save_tsk+0x50/0x6c [<0>] proc_pid_stack+0xa0/0xf8 [<0>] proc_single_show+0x50/0xbc [<0>] seq_read_iter+0x178/0x4ec [<0>] seq_read+0x138/0x15c [<0>] vfs_read+0xd0/0x304 [<0>] ksys_read+0x78/0xd4 [<0>] sys_read+0xc/0x10
after: # cat /proc/self/stack [<0>] proc_pid_stack+0xa0/0xf8 [<0>] proc_single_show+0x50/0xbc [<0>] seq_read_iter+0x178/0x4ec [<0>] seq_read+0x138/0x15c [<0>] vfs_read+0xd0/0x304 [<0>] ksys_read+0x78/0xd4 [<0>] sys_read+0xc/0x10 [<0>] __entry_text_start+0x14/0x14 [<0>] 0xffffffff
So I guess this helps the CONFIG_UNWINDER_FRAME_POINTER=y case? (That final frame address looks wrong, but is potentially yet another bug; perhaps for clang we need to manually store the previous frame's pc at a different offset before jumping to __entry_text_start).
Also, I'm curious about CONFIG_THUMB2_KERNEL (forces CONFIG_UNWINDER_ARM=y).
before: # cat /proc/self/stack [<0>] proc_single_show+0x31/0x86 [<0>] seq_read_iter+0xff/0x326 [<0>] seq_read+0xd7/0xf2 [<0>] vfs_read+0x93/0x20e [<0>] ksys_read+0x53/0x92 [<0>] ret_fast_syscall+0x1/0x52 [<0>] 0xbe9a9cc0
after: # cat /proc/self/stack [<0>] proc_single_show+0x31/0x86 [<0>] seq_read_iter+0xff/0x326 [<0>] seq_read+0xd7/0xf2 [<0>] vfs_read+0x93/0x20e [<0>] ksys_read+0x53/0x92 [<0>] ret_fast_syscall+0x1/0x52 [<0>] 0xbec08cc0
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
so likely this fixes/improves CONFIG_UNWINDER_FRAME_POINTER=y? Is that correct?
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > --- > arch/arm/kernel/stacktrace.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c > index 76ea4178a55c..db798eac7431 100644 > --- a/arch/arm/kernel/stacktrace.c > +++ b/arch/arm/kernel/stacktrace.c > @@ -54,8 +54,7 @@ int notrace unwind_frame(struct stackframe *frame) > > frame->sp = frame->fp; > frame->fp = *(unsigned long *)(fp); > - frame->pc = frame->lr; > - frame->lr = *(unsigned long *)(fp + 4); > + frame->pc = *(unsigned long *)(fp + 4); > #else > /* check current frame pointer is within bounds */ > if (fp < low + 12 || fp > high - 4) >
-- Thanks, ~Nick Desaulniers
| |