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

Masami Hiramatsu
2021-10-12T14:18:46Z

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>