* [PATCH v3 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
2020-05-20 17:55 [PATCH v3 0/2] arm64 sigreturn unwinding fixes Will Deacon
@ 2020-05-20 17:55 ` Will Deacon
2020-05-20 17:55 ` [PATCH v3 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon
2020-05-21 9:53 ` [PATCH v3 0/2] arm64 sigreturn unwinding fixes Daniel Kiss
2 siblings, 0 replies; 4+ messages in thread
From: Will Deacon @ 2020-05-20 17:55 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin,
Daniel Kiss
For better or worse, GDB relies on the exact instruction sequence in the
VDSO sigreturn trampoline in order to unwind from signals correctly.
Commit c91db232da48 ("arm64: vdso: Convert to modern assembler annotations")
unfortunately added a BTI C instruction to the start of __kernel_rt_sigreturn,
which breaks this check. Thankfully, it's also not required, since the
trampoline is called from a RET instruction when returning from the signal
handler
Remove the unnecessary BTI C instruction from __kernel_rt_sigreturn,
and do the same for the 32-bit VDSO as well for good measure.
Cc: Daniel Kiss <daniel.kiss@arm.com>
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
Reviewed-by: Dave Martin <dave.martin@arm.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Fixes: c91db232da48 ("arm64: vdso: Convert to modern assembler annotations")
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/include/asm/linkage.h | 6 +++---
arch/arm64/kernel/vdso/sigreturn.S | 11 +++++++++--
arch/arm64/kernel/vdso32/sigreturn.S | 16 ++++++++--------
3 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index b5a7998a6b2a..81fefd2a1d02 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -15,9 +15,9 @@
#define BTI_J hint 36 ;
/*
- * When using in-kernel BTI we need to ensure that assembly functions
- * have suitable annotations. Override SYM_FUNC_START to insert a BTI
- * landing pad at the start of everything.
+ * When using in-kernel BTI we need to ensure that PCS-conformant assembly
+ * functions have suitable annotations. Override SYM_FUNC_START to insert
+ * a BTI landing pad at the start of everything.
*/
#define SYM_FUNC_START(name) \
SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) \
diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
index 3fb13b81f780..0c921130002a 100644
--- a/arch/arm64/kernel/vdso/sigreturn.S
+++ b/arch/arm64/kernel/vdso/sigreturn.S
@@ -15,7 +15,14 @@
.text
nop
-SYM_FUNC_START(__kernel_rt_sigreturn)
+/*
+ * GDB relies on being able to identify the sigreturn instruction sequence to
+ * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START()
+ * here, as it will emit a BTI C instruction and break the unwinder. Thankfully,
+ * this function is only ever called from a RET and so omitting the landing pad
+ * is perfectly fine.
+ */
+SYM_CODE_START(__kernel_rt_sigreturn)
.cfi_startproc
.cfi_signal_frame
.cfi_def_cfa x29, 0
@@ -24,6 +31,6 @@ SYM_FUNC_START(__kernel_rt_sigreturn)
mov x8, #__NR_rt_sigreturn
svc #0
.cfi_endproc
-SYM_FUNC_END(__kernel_rt_sigreturn)
+SYM_CODE_END(__kernel_rt_sigreturn)
emit_aarch64_feature_1_and
diff --git a/arch/arm64/kernel/vdso32/sigreturn.S b/arch/arm64/kernel/vdso32/sigreturn.S
index 620524969696..b36d4e2267a3 100644
--- a/arch/arm64/kernel/vdso32/sigreturn.S
+++ b/arch/arm64/kernel/vdso32/sigreturn.S
@@ -17,39 +17,39 @@
.save {r0-r15}
.pad #COMPAT_SIGFRAME_REGS_OFFSET
nop
-SYM_FUNC_START(__kernel_sigreturn_arm)
+SYM_CODE_START(__kernel_sigreturn_arm)
mov r7, #__NR_compat_sigreturn
svc #0
.fnend
-SYM_FUNC_END(__kernel_sigreturn_arm)
+SYM_CODE_END(__kernel_sigreturn_arm)
.fnstart
.save {r0-r15}
.pad #COMPAT_RT_SIGFRAME_REGS_OFFSET
nop
-SYM_FUNC_START(__kernel_rt_sigreturn_arm)
+SYM_CODE_START(__kernel_rt_sigreturn_arm)
mov r7, #__NR_compat_rt_sigreturn
svc #0
.fnend
-SYM_FUNC_END(__kernel_rt_sigreturn_arm)
+SYM_CODE_END(__kernel_rt_sigreturn_arm)
.thumb
.fnstart
.save {r0-r15}
.pad #COMPAT_SIGFRAME_REGS_OFFSET
nop
-SYM_FUNC_START(__kernel_sigreturn_thumb)
+SYM_CODE_START(__kernel_sigreturn_thumb)
mov r7, #__NR_compat_sigreturn
svc #0
.fnend
-SYM_FUNC_END(__kernel_sigreturn_thumb)
+SYM_CODE_END(__kernel_sigreturn_thumb)
.fnstart
.save {r0-r15}
.pad #COMPAT_RT_SIGFRAME_REGS_OFFSET
nop
-SYM_FUNC_START(__kernel_rt_sigreturn_thumb)
+SYM_CODE_START(__kernel_rt_sigreturn_thumb)
mov r7, #__NR_compat_rt_sigreturn
svc #0
.fnend
-SYM_FUNC_END(__kernel_rt_sigreturn_thumb)
+SYM_CODE_END(__kernel_rt_sigreturn_thumb)
--
2.26.2.761.g0e0b3e54be-goog
_______________________________________________
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] 4+ messages in thread* [PATCH v3 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline
2020-05-20 17:55 [PATCH v3 0/2] arm64 sigreturn unwinding fixes Will Deacon
2020-05-20 17:55 ` [PATCH v3 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon
@ 2020-05-20 17:55 ` Will Deacon
2020-05-21 9:53 ` [PATCH v3 0/2] arm64 sigreturn unwinding fixes Daniel Kiss
2 siblings, 0 replies; 4+ messages in thread
From: Will Deacon @ 2020-05-20 17:55 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin,
Daniel Kiss
Daniel reports that the .cfi_startproc is misplaced for the sigreturn
trampoline, which causes LLVM's unwinder to misbehave:
| I run into this with LLVM’s unwinder.
| This combination was always broken.
This prompted Dave to question our use of CFI directives more generally,
and I ended up going down a rabbit hole trying to figure out how this
very poorly documented stuff gets used.
Move the CFI directives so that the "mysterious NOP" is included in
the .cfi_{start,end}proc block and add a bunch of comments so that I
can save myself another headache in future.
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
Reported-by: Dave Martin <dave.martin@arm.com>
Reported-by: Daniel Kiss <daniel.kiss@arm.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
arch/arm64/kernel/vdso/sigreturn.S | 44 +++++++++++++++++++++++-----
arch/arm64/kernel/vdso32/sigreturn.S | 3 ++
2 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
index 0c921130002a..620a3ef837b7 100644
--- a/arch/arm64/kernel/vdso/sigreturn.S
+++ b/arch/arm64/kernel/vdso/sigreturn.S
@@ -1,7 +1,11 @@
/* SPDX-License-Identifier: GPL-2.0-only */
/*
* Sigreturn trampoline for returning from a signal when the SA_RESTORER
- * flag is not set.
+ * flag is not set. It serves primarily as a hall of shame for crappy
+ * unwinders and features an exciting but mysterious NOP instruction.
+ *
+ * It's also fragile as hell, so please think twice before changing anything
+ * in here.
*
* Copyright (C) 2012 ARM Limited
*
@@ -14,7 +18,38 @@
.text
- nop
+/* Ensure that the mysterious NOP can be associated with a function. */
+ .cfi_startproc
+
+/*
+ * .cfi_signal_frame causes the corresponding Frame Description Entry in the
+ * .eh_frame section to be annotated as a signal frame. This allows DWARF
+ * unwinders (e.g. libstdc++) to implement _Unwind_GetIPInfo(), which permits
+ * unwinding out of the signal trampoline without the need for the mysterious
+ * NOP.
+ */
+ .cfi_signal_frame
+
+/*
+ * Tell the unwinder where to locate the frame record linking back to the
+ * interrupted context. We don't provide unwind info for registers other
+ * than the frame pointer and the link register here; in practice, this
+ * is sufficient for unwinding in C/C++ based runtimes and the values in
+ * the sigcontext may have been modified by this point anyway. Debuggers
+ * already have baked-in strategies for attempting to unwind out of signals.
+ */
+ .cfi_def_cfa x29, 0
+ .cfi_offset x29, 0 * 8
+ .cfi_offset x30, 1 * 8
+
+/*
+ * This mysterious NOP is required for some unwinders (e.g. libc++) that
+ * unconditionally subtract one from the result of _Unwind_GetIP() in order to
+ * identify the calling function.
+ * Hack borrowed from arch/powerpc/kernel/vdso64/sigtramp.S.
+ */
+ nop // Mysterious NOP
+
/*
* GDB relies on being able to identify the sigreturn instruction sequence to
* unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START()
@@ -23,11 +58,6 @@
* is perfectly fine.
*/
SYM_CODE_START(__kernel_rt_sigreturn)
- .cfi_startproc
- .cfi_signal_frame
- .cfi_def_cfa x29, 0
- .cfi_offset x29, 0 * 8
- .cfi_offset x30, 1 * 8
mov x8, #__NR_rt_sigreturn
svc #0
.cfi_endproc
diff --git a/arch/arm64/kernel/vdso32/sigreturn.S b/arch/arm64/kernel/vdso32/sigreturn.S
index b36d4e2267a3..b0091064c3d6 100644
--- a/arch/arm64/kernel/vdso32/sigreturn.S
+++ b/arch/arm64/kernel/vdso32/sigreturn.S
@@ -3,6 +3,9 @@
* This file provides both A32 and T32 versions, in accordance with the
* arm sigreturn code.
*
+ * Please read the comments in arch/arm64/kernel/vdso/sigreturn.S to
+ * understand some of the craziness in here.
+ *
* Copyright (C) 2018 ARM Limited
*/
--
2.26.2.761.g0e0b3e54be-goog
_______________________________________________
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] 4+ messages in thread* Re: [PATCH v3 0/2] arm64 sigreturn unwinding fixes
2020-05-20 17:55 [PATCH v3 0/2] arm64 sigreturn unwinding fixes Will Deacon
2020-05-20 17:55 ` [PATCH v3 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon
2020-05-20 17:55 ` [PATCH v3 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon
@ 2020-05-21 9:53 ` Daniel Kiss
2 siblings, 0 replies; 4+ messages in thread
From: Daniel Kiss @ 2020-05-21 9:53 UTC (permalink / raw)
To: Will Deacon
Cc: Tamas Zsoldos, Mark Brown, kernel-team@android.com, Dave P Martin,
linux-arm-kernel@lists.infradead.org
Hi Will,
Works okay, LGTM.
Thanks for the patch.
Cheers,
Daniel
> On 20 May 2020, at 19:55, Will Deacon <will@kernel.org> wrote:
>
> Hi folks,
>
> Here is v3 of my attempt at fixing the vdso sigreturn code for unwinders.
> Previous versions are available here:
>
> v1: https://lore.kernel.org/r/20200519121818.14511-1-will@kernel.org
> v2: https://lore.kernel.org/r/20200519162821.16857-1-will@kernel.org
>
> Changes since v2 include:
>
> * Fix the .cfi directives to identify the link register correctly
> * Even more comments
>
> Daniel, please can you give this a spin with the LLVM unwinder? It should work
> this time.
>
> Cheers,
>
> Will
>
> Cc: Dave Martin <dave.martin@arm.com>
> Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>
> Cc: Daniel Kiss <daniel.kiss@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: <kernel-team@android.com>
>
> --->8
>
> Will Deacon (2):
> arm64: vdso: Don't prefix sigreturn trampoline with a BTI C
> instruction
> arm64: vdso: Fix CFI directives in sigreturn trampoline
>
> arch/arm64/include/asm/linkage.h | 6 ++--
> arch/arm64/kernel/vdso/sigreturn.S | 51 ++++++++++++++++++++++++----
> arch/arm64/kernel/vdso32/sigreturn.S | 19 ++++++-----
> 3 files changed, 58 insertions(+), 18 deletions(-)
>
> --
> 2.26.2.761.g0e0b3e54be-goog
>
_______________________________________________
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] 4+ messages in thread