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

[Nick Desaulniers <ndesaulniers@google.com>]

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