Skip to contentSkip to conversation

arm64 sigreturn unwinding fixes

PATCH In review Will Deacon 2 participants 4 messages May 20, 2020 – May 21, 2020 arm64

lore
Share & tools
lore

Will Deacon

author maintainer
#0 lore raw

v3 cover arm64 sigreturn unwinding fixes

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/[email protected]
v2: https://lore.kernel.org/r/[email protected]

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 <[email protected]>
Cc Tamas Zsoldos <[email protected]>
Cc Daniel Kiss <[email protected]>
Cc Mark Brown <[email protected]>

--->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(-)
Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author maintainer
#1 lore raw

PATCH v3 1/2 arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

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 <[email protected]>
Cc Tamas Zsoldos <[email protected]>
Fixes c91db232da48 ("arm64: vdso: Convert to modern assembler annotations")
---
 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(-)
arch/arm64/include/asm/linkage.h +3 -3
index b5a7998a6b2a..81fefd2a1d02 100644
@@ -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)	\
arch/arm64/kernel/vdso/sigreturn.S +9 -2
index 3fb13b81f780..0c921130002a 100644
@@ -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
arch/arm64/kernel/vdso32/sigreturn.S +8 -8
index 620524969696..b36d4e2267a3 100644
@@ -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)
Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author maintainer
#2 lore raw

PATCH v3 2/2 arm64: vdso: Fix CFI directives in sigreturn trampoline

In reply to Will Deacon

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.

---
 arch/arm64/kernel/vdso/sigreturn.S   | 44 +++++++++++++++++++++++-----
 arch/arm64/kernel/vdso32/sigreturn.S |  3 ++
 2 files changed, 40 insertions(+), 7 deletions(-)
arch/arm64/kernel/vdso/sigreturn.S +37 -7
index 0c921130002a..620a3ef837b7 100644
@@ -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
arch/arm64/kernel/vdso32/sigreturn.S +3 -0
index b36d4e2267a3..b0091064c3d6 100644
@@ -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
  */
 
Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#3 lore raw
In reply to Will Deacon

Hi Will,

Works okay, LGTM.

Thanks for the patch.

Cheers,
Daniel

Quoted 35 lines · Will Deacon

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/[email protected]
v2: https://lore.kernel.org/r/[email protected]

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 <[email protected]>
Cc Tamas Zsoldos <[email protected]>
Cc Daniel Kiss <[email protected]>
Cc Mark Brown <[email protected]>

--->8

Will Deacon (2):
arm64: vdso: Don't prefix sigreturn trampoline with a BTI C
instruction
arm64: vdso: Fix CFI directives in sigreturn trampoline

Quoted 8 lines
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(-)
Signature · 1 line
2.26.2.761.g0e0b3e54be-goog

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel