[PATCH v2] ARM: unwind: set frame.pc correctly for current-thread unwinding

[Ard Biesheuvel]

On Wed, 9 Mar 2022 at 13:19, Russell King (Oracle)
<rmk+kernel at armlinux.org.uk> wrote:
>
> When e.g. a WARN_ON() is encountered, we attempt to unwind the current
> thread. To do this, we set frame.pc to unwind_backtrace, which means it
> points at the beginning of the function. However, the rest of the state
> is initialised from within the function, which means the function
> prologue has already been run.
>
> This can be confusing, and with a recent patch from Ard, can result in
> the unwinder misbehaving if we want to be strict about the PC value.
>
> If we correctly initialise the state so it is self-consistent (in other
> words, set frame.pc to the location we are initialising it) then we
> eliminate this confusion, and avoid possible future issues.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel at armlinux.org.uk>

Reviewed-by: Ard Biesheuvel <ardb at kernel.org>

> ---
>  arch/arm/kernel/return_address.c | 3 ++-
>  arch/arm/kernel/stacktrace.c     | 3 ++-
>  arch/arm/kernel/unwind.c         | 7 ++++++-
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
> index 00c11579406c..8aac1e10b117 100644
> --- a/arch/arm/kernel/return_address.c
> +++ b/arch/arm/kernel/return_address.c
> @@ -41,7 +41,8 @@ void *return_address(unsigned int level)
>         frame.fp = (unsigned long)__builtin_frame_address(0);
>         frame.sp = current_stack_pointer;
>         frame.lr = (unsigned long)__builtin_return_address(0);
> -       frame.pc = (unsigned long)return_address;
> +here:
> +       frame.pc = (unsigned long)&&here;
>  #ifdef CONFIG_KRETPROBES
>         frame.kr_cur = NULL;
>         frame.tsk = current;
> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> index 75e905508f27..b5efecb3d730 100644
> --- a/arch/arm/kernel/stacktrace.c
> +++ b/arch/arm/kernel/stacktrace.c
> @@ -160,7 +160,8 @@ static noinline void __save_stack_trace(struct task_struct *tsk,
>                 frame.fp = (unsigned long)__builtin_frame_address(0);
>                 frame.sp = current_stack_pointer;
>                 frame.lr = (unsigned long)__builtin_return_address(0);
> -               frame.pc = (unsigned long)__save_stack_trace;
> +here:
> +               frame.pc = (unsigned long)&&here;
>         }
>  #ifdef CONFIG_KRETPROBES
>         frame.kr_cur = NULL;
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index a007af0f0209..0e2244e26f37 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -487,7 +487,12 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>                 frame.fp = (unsigned long)__builtin_frame_address(0);
>                 frame.sp = current_stack_pointer;
>                 frame.lr = (unsigned long)__builtin_return_address(0);
> -               frame.pc = (unsigned long)unwind_backtrace;
> +               /* We are saving the stack and execution state at this
> +                * point, so we should ensure that frame.pc is within
> +                * this block of code.
> +                */
> +here:
> +               frame.pc = (unsigned long)&&here;
>         } else {
>                 /* task blocked in __switch_to */
>                 frame.fp = thread_saved_fp(tsk);
> --
> 2.30.2
>