* [PATCH v2] ARM: unwind: set frame.pc correctly for current-thread unwinding
@ 2022-03-09 12:19 Russell King (Oracle)
2022-03-09 12:29 ` Ard Biesheuvel
0 siblings, 1 reply; 2+ messages in thread
From: Russell King (Oracle) @ 2022-03-09 12:19 UTC (permalink / raw)
To: linux-arm-kernel, Ard Biesheuvel
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@armlinux.org.uk>
---
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] ARM: unwind: set frame.pc correctly for current-thread unwinding
2022-03-09 12:19 [PATCH v2] ARM: unwind: set frame.pc correctly for current-thread unwinding Russell King (Oracle)
@ 2022-03-09 12:29 ` Ard Biesheuvel
0 siblings, 0 replies; 2+ messages in thread
From: Ard Biesheuvel @ 2022-03-09 12:29 UTC (permalink / raw)
To: Russell King (Oracle); +Cc: Linux ARM
On Wed, 9 Mar 2022 at 13:19, Russell King (Oracle)
<rmk+kernel@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@armlinux.org.uk>
Reviewed-by: Ard Biesheuvel <ardb@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
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-03-09 12:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-09 12:19 [PATCH v2] ARM: unwind: set frame.pc correctly for current-thread unwinding Russell King (Oracle)
2022-03-09 12:29 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).