Re: [PATCH 6/8] ARM: clang: Do not relay on lr register for stacktrace

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Nick,

On Mon, 11 Oct 2021 11:45:22 -0700
Nick Desaulniers <ndesaulniers@google.com> wrote:

> 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.)

Hmm, I found this bug while testing my kretprobe-stacktrace test
([2/8] in this series), so if you apply this series and revert
this patch and enable CONFIG_KPROBES_SANITY_TEST, you'll see that
the tests failures as below.

[    4.062056]     ok 4 - test_kretprobes
[    4.069944]     # test_stacktrace_on_kretprobe: EXPECTATION FAILED at kernel/test_kprobes.c:235
[    4.069944]     Expected i != ret, but
[    4.069944]         i == 10
[    4.069944]         ret == 10
[    4.072692]     not ok 5 - test_stacktrace_on_kretprobe
[    4.088171]     # test_stacktrace_on_nested_kretprobe: EXPECTATION FAILED at kernel/test_kprobes.c:235
[    4.088171]     Expected i != ret, but
[    4.088171]         i == 10
[    4.088171]         ret == 10
[    4.091265]     not ok 6 - test_stacktrace_on_nested_kretprobe

This means the test failed to find the correct return address from
the stacktrace.
Applying this patch,

[    4.283953]     ok 4 - test_kretprobes
[    4.290206]     ok 5 - test_stacktrace_on_kretprobe
[    4.300429]     ok 6 - test_stacktrace_on_nested_kretprobe
[    4.301743] # kprobes_test: pass:6 fail:0 skip:0 total:6


> 
> 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).

Ah, yes. I didn't touch the UNWINDER_ARM. As you may know the
unwind_frame()@arch/arm/kernel/stacktrace.c is compiled only
CONFIG_UNWINDER_FRAME_POINTER=y.

> 
> 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?

Yes, that is correct.

Thank you!

> 
> >
> > 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


-- 
Masami Hiramatsu <mhiramat@kernel.org>