* [PATCH v2 0/2] arm64 sigreturn unwinding fixes
@ 2020-05-19 16:28 Will Deacon
2020-05-19 16:28 ` [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon
2020-05-19 16:28 ` [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon
0 siblings, 2 replies; 13+ messages in thread
From: Will Deacon @ 2020-05-19 16:28 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin,
Daniel Kiss
Hi again,
Hot off the press, here's v2 of the sigreturn unwinding fixes I sent out
early on today:
https://lore.kernel.org/r/20200519121818.14511-1-will@kernel.org
Changes since v1 include:
* Retain all CFI directives, as they are needed for unwinding after all
* Add tonnes of comments
* Squash the last two patches
* Use SYM_CODE_{START,END} instead of open-coding it
* Update Fixes: tag
Please have a look, and give it a spin with your favourite unwinder.
Ta,
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/kernel/vdso/sigreturn.S | 47 +++++++++++++++++++++++-----
arch/arm64/kernel/vdso32/sigreturn.S | 16 +++++-----
2 files changed, 48 insertions(+), 15 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] 13+ messages in thread* [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 16:28 [PATCH v2 0/2] arm64 sigreturn unwinding fixes Will Deacon @ 2020-05-19 16:28 ` Will Deacon 2020-05-19 16:33 ` Mark Brown 2020-05-20 9:33 ` Dave Martin 2020-05-19 16:28 ` [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon 1 sibling, 2 replies; 13+ messages in thread From: Will Deacon @ 2020-05-19 16:28 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: Dave Martin <dave.martin@arm.com> Cc: Mark Brown <broonie@kernel.org> Cc: Daniel Kiss <daniel.kiss@arm.com> Cc: Tamas Zsoldos <tamas.zsoldos@arm.com> Fixes: c91db232da48 ("arm64: vdso: Convert to modern assembler annotations") Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/kernel/vdso/sigreturn.S | 11 +++++++++-- arch/arm64/kernel/vdso32/sigreturn.S | 16 ++++++++-------- 2 files changed, 17 insertions(+), 10 deletions(-) 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] 13+ messages in thread
* Re: [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 16:28 ` [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon @ 2020-05-19 16:33 ` Mark Brown 2020-05-20 9:33 ` Dave Martin 1 sibling, 0 replies; 13+ messages in thread From: Mark Brown @ 2020-05-19 16:33 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, kernel-team, Dave Martin, linux-arm-kernel, Daniel Kiss [-- Attachment #1.1: Type: text/plain, Size: 572 bytes --] On Tue, May 19, 2020 at 05:28:20PM +0100, Will Deacon wrote: > 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 Reviwed-by: Mark Brown <broonie@kernel.org> [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ 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] 13+ messages in thread
* Re: [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 16:28 ` [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon 2020-05-19 16:33 ` Mark Brown @ 2020-05-20 9:33 ` Dave Martin 2020-05-20 9:53 ` Will Deacon 1 sibling, 1 reply; 13+ messages in thread From: Dave Martin @ 2020-05-20 9:33 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 05:28:20PM +0100, Will Deacon wrote: > 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: Dave Martin <dave.martin@arm.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Daniel Kiss <daniel.kiss@arm.com> > Cc: Tamas Zsoldos <tamas.zsoldos@arm.com> > Fixes: c91db232da48 ("arm64: vdso: Convert to modern assembler annotations") > Signed-off-by: Will Deacon <will@kernel.org> > --- > arch/arm64/kernel/vdso/sigreturn.S | 11 +++++++++-- > arch/arm64/kernel/vdso32/sigreturn.S | 16 ++++++++-------- > 2 files changed, 17 insertions(+), 10 deletions(-) > > 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. > + */ Can we cross-reference or duplicate (perhaps abridged) this comment for vdso32? Can we also fix the comment by the definition of SYM_FUNC_START()? SYM_FUNC_START() supersedes ENTRY only for PCS-conformant function entry points. Any code with a wacky special-case interface should not not be using this. [...] > +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) ...although do we actually need this? 32-bit doesn't have BTI. But for the reasons given above, this is not a "function" and so SYM_FUNC_START() is trap for future maintenance even if it makes no difference now. [...] Either way, Reviewed-by: Dave Martin <Dave.Martin@arm.com> _______________________________________________ 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] 13+ messages in thread
* Re: [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-20 9:33 ` Dave Martin @ 2020-05-20 9:53 ` Will Deacon 0 siblings, 0 replies; 13+ messages in thread From: Will Deacon @ 2020-05-20 9:53 UTC (permalink / raw) To: Dave Martin Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Wed, May 20, 2020 at 10:33:55AM +0100, Dave Martin wrote: > On Tue, May 19, 2020 at 05:28:20PM +0100, Will Deacon wrote: > > 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: Dave Martin <dave.martin@arm.com> > > Cc: Mark Brown <broonie@kernel.org> > > Cc: Daniel Kiss <daniel.kiss@arm.com> > > Cc: Tamas Zsoldos <tamas.zsoldos@arm.com> > > Fixes: c91db232da48 ("arm64: vdso: Convert to modern assembler annotations") > > Signed-off-by: Will Deacon <will@kernel.org> > > --- > > arch/arm64/kernel/vdso/sigreturn.S | 11 +++++++++-- > > arch/arm64/kernel/vdso32/sigreturn.S | 16 ++++++++-------- > > 2 files changed, 17 insertions(+), 10 deletions(-) > > > > 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. > > + */ > > Can we cross-reference or duplicate (perhaps abridged) this comment for > vdso32? Yes, that's not a bad idea. I'll add a comment to the top of that file. > Can we also fix the comment by the definition of SYM_FUNC_START()? I'll tweak it slightly for v3. > > 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) > > ...although do we actually need this? 32-bit doesn't have BTI. > > But for the reasons given above, this is not a "function" and so > SYM_FUNC_START() is trap for future maintenance even if it makes no > difference now. Right, it's just done for consistency on the 32-bit side. > Either way, > > Reviewed-by: Dave Martin <Dave.Martin@arm.com> Thanks! Will _______________________________________________ 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] 13+ messages in thread
* [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 16:28 [PATCH v2 0/2] arm64 sigreturn unwinding fixes Will Deacon 2020-05-19 16:28 ` [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon @ 2020-05-19 16:28 ` Will Deacon 2020-05-20 9:42 ` Dave Martin 2020-05-20 10:48 ` Will Deacon 1 sibling, 2 replies; 13+ messages in thread From: Will Deacon @ 2020-05-19 16:28 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 | 40 ++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 7 deletions(-) diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S index 0c921130002a..cb47dfb3bd5a 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,34 @@ .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. + */ + .cfi_def_cfa x29, 0 + .cfi_offset x29, 0 * 8 + .cfi_offset x29, 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 +54,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 -- 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] 13+ messages in thread
* Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 16:28 ` [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon @ 2020-05-20 9:42 ` Dave Martin 2020-05-20 9:50 ` Will Deacon 2020-05-20 10:48 ` Will Deacon 1 sibling, 1 reply; 13+ messages in thread From: Dave Martin @ 2020-05-20 9:42 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote: > 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 | 40 ++++++++++++++++++++++++------ > 1 file changed, 33 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > index 0c921130002a..cb47dfb3bd5a 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,34 @@ > > .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. > + */ > + .cfi_def_cfa x29, 0 > + .cfi_offset x29, 0 * 8 > + .cfi_offset x29, 1 * 8 We should also give rationale for why we don't describe how to recover other regs here. At a signal, every reg is potentially live with data essential to the backtrace, so custom unwind entries further up the stack may unwind badly after trying to unwind out of the signal handler. Otherwise, looks reasonable -- it should be easier now to understand next time! [...] Cheers ---Dave _______________________________________________ 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] 13+ messages in thread
* Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-20 9:42 ` Dave Martin @ 2020-05-20 9:50 ` Will Deacon 2020-05-20 10:27 ` Dave Martin 0 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2020-05-20 9:50 UTC (permalink / raw) To: Dave Martin Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Wed, May 20, 2020 at 10:42:13AM +0100, Dave Martin wrote: > On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote: > > @@ -14,7 +18,34 @@ > > > > .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. > > + */ > > + .cfi_def_cfa x29, 0 > > + .cfi_offset x29, 0 * 8 > > + .cfi_offset x29, 1 * 8 > > We should also give rationale for why we don't describe how to recover > other regs here. At a signal, every reg is potentially live with data > essential to the backtrace, so custom unwind entries further up the > stack may unwind badly after trying to unwind out of the signal handler. Hmm, I'm not sure I get what you're asking for. We can't recover the other registers even if we tried, can we? I think the only way to get a reliable backtrace here is not to clobber the framepointer. Will _______________________________________________ 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] 13+ messages in thread
* Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-20 9:50 ` Will Deacon @ 2020-05-20 10:27 ` Dave Martin 2020-05-20 10:36 ` Will Deacon 0 siblings, 1 reply; 13+ messages in thread From: Dave Martin @ 2020-05-20 10:27 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Wed, May 20, 2020 at 10:50:28AM +0100, Will Deacon wrote: > On Wed, May 20, 2020 at 10:42:13AM +0100, Dave Martin wrote: > > On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote: > > > @@ -14,7 +18,34 @@ > > > > > > .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. > > > + */ > > > + .cfi_def_cfa x29, 0 > > > + .cfi_offset x29, 0 * 8 > > > + .cfi_offset x29, 1 * 8 > > > > We should also give rationale for why we don't describe how to recover > > other regs here. At a signal, every reg is potentially live with data > > essential to the backtrace, so custom unwind entries further up the > > stack may unwind badly after trying to unwind out of the signal handler. > > Hmm, I'm not sure I get what you're asking for. We can't recover the other > registers even if we tried, can we? I think the only way to get a reliable > backtrace here is not to clobber the framepointer. A caller somewhere up the stack could have stashed stuff in nonstandard places, with a custom unwind entry that doesn't use x29 in the usual way. If x29 and x30 were stashed in x8 and x9, say, then the unwinder needs to restore x8 and x9 correctly before that frame is reached. Dwarf unwind tables are expressive enough to describe how to unwind such a frames: the directives work on all the registers, not just x29, lr. For this kind of unwinding scenario to wokr, the userspace environment would need to provide correct unwind info for _everything_ rather than relying on the frame chain on the stack alone, so this scenario isn't applicable to C. I'm not saying we should try to support this, but a comment to indicate what we are and are not trying to do might be a good idea. How about something along these lines: /* * Don't try to provide unwind into for the other regs of the * interrupted context here. C/C++ based runtimes don't rely on * this for unwinding in practice. Debuggers need more, but they * already have baked-in knowledge about how to unwind out of * signals. */ Cheers ---Dave _______________________________________________ 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] 13+ messages in thread
* Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-20 10:27 ` Dave Martin @ 2020-05-20 10:36 ` Will Deacon 2020-05-20 11:03 ` Dave Martin 0 siblings, 1 reply; 13+ messages in thread From: Will Deacon @ 2020-05-20 10:36 UTC (permalink / raw) To: Dave Martin Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Wed, May 20, 2020 at 11:27:47AM +0100, Dave Martin wrote: > On Wed, May 20, 2020 at 10:50:28AM +0100, Will Deacon wrote: > > On Wed, May 20, 2020 at 10:42:13AM +0100, Dave Martin wrote: > > > On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote: > > > > @@ -14,7 +18,34 @@ > > > > > > > > .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. > > > > + */ > > > > + .cfi_def_cfa x29, 0 > > > > + .cfi_offset x29, 0 * 8 > > > > + .cfi_offset x29, 1 * 8 > > > > > > We should also give rationale for why we don't describe how to recover > > > other regs here. At a signal, every reg is potentially live with data > > > essential to the backtrace, so custom unwind entries further up the > > > stack may unwind badly after trying to unwind out of the signal handler. > > > > Hmm, I'm not sure I get what you're asking for. We can't recover the other > > registers even if we tried, can we? I think the only way to get a reliable > > backtrace here is not to clobber the framepointer. > > A caller somewhere up the stack could have stashed stuff in nonstandard > places, with a custom unwind entry that doesn't use x29 in the usual way. > > If x29 and x30 were stashed in x8 and x9, say, then the unwinder needs > to restore x8 and x9 correctly before that frame is reached. Dwarf > unwind tables are expressive enough to describe how to unwind such a > frames: the directives work on all the registers, not just x29, lr. Understood, I just can't figure out how we could support that even if we wanted to. The only evidence we have of those registers is in the sigcontext, but that may have been modified by the time we end up in the return trampoline. Would we need to push the registers twice (i.e. expand the frame record to include the GPRs)? Not saying we should do this, just wondering what it would take. > For this kind of unwinding scenario to wokr, the userspace environment > would need to provide correct unwind info for _everything_ rather than > relying on the frame chain on the stack alone, so this scenario isn't > applicable to C. > > I'm not saying we should try to support this, but a comment to indicate > what we are and are not trying to do might be a good idea. > > How about something along these lines: > > /* > * Don't try to provide unwind into for the other regs of the > * interrupted context here. C/C++ based runtimes don't rely on > * this for unwinding in practice. Debuggers need more, but they > * already have baked-in knowledge about how to unwind out of > * signals. > */ I'll fold that in, thanks. Will _______________________________________________ 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] 13+ messages in thread
* Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-20 10:36 ` Will Deacon @ 2020-05-20 11:03 ` Dave Martin 0 siblings, 0 replies; 13+ messages in thread From: Dave Martin @ 2020-05-20 11:03 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Wed, May 20, 2020 at 11:36:40AM +0100, Will Deacon wrote: > On Wed, May 20, 2020 at 11:27:47AM +0100, Dave Martin wrote: > > On Wed, May 20, 2020 at 10:50:28AM +0100, Will Deacon wrote: > > > On Wed, May 20, 2020 at 10:42:13AM +0100, Dave Martin wrote: > > > > On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote: > > > > > @@ -14,7 +18,34 @@ > > > > > > > > > > .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. > > > > > + */ > > > > > + .cfi_def_cfa x29, 0 > > > > > + .cfi_offset x29, 0 * 8 > > > > > + .cfi_offset x29, 1 * 8 > > > > > > > > We should also give rationale for why we don't describe how to recover > > > > other regs here. At a signal, every reg is potentially live with data > > > > essential to the backtrace, so custom unwind entries further up the > > > > stack may unwind badly after trying to unwind out of the signal handler. > > > > > > Hmm, I'm not sure I get what you're asking for. We can't recover the other > > > registers even if we tried, can we? I think the only way to get a reliable > > > backtrace here is not to clobber the framepointer. > > > > A caller somewhere up the stack could have stashed stuff in nonstandard > > places, with a custom unwind entry that doesn't use x29 in the usual way. > > > > If x29 and x30 were stashed in x8 and x9, say, then the unwinder needs > > to restore x8 and x9 correctly before that frame is reached. Dwarf > > unwind tables are expressive enough to describe how to unwind such a > > frames: the directives work on all the registers, not just x29, lr. > > Understood, I just can't figure out how we could support that even if we > wanted to. The only evidence we have of those registers is in the > sigcontext, but that may have been modified by the time we end up in the > return trampoline. Would we need to push the registers twice (i.e. expand > the frame record to include the GPRs)? Not saying we should do this, just > wondering what it would take. No, it's inevitably best effort. If the signal handler doesn't intend to return, than the backtrace may be nonsense anyway. The signal might result from the regs being garbage anyway, or the signal might be deliberate suicide by the "caller". If the signal handler does intend to return normally, then it is responsible for manipulating the sigcontext in a way that doesn't break the interrupted code -- which implies that the backtrace will be valid, and also means that invasive non-atomic changes to the sigcontext are unlikely. Because we can't know the intent of the handler, no amount of pushing duplicates etc. can work 100% of the time. The overwhelmingly common case of is that the signal handler doesn't mess with sigcontext at all, though. So we could probably restore the integer regs correctly for the common case. Nonetheless, there are limitations. Dwarf unwind can't describe how to unwind the FPSIMD/SVE regs etc. We're really into debugger territory if we start to care about that stuff. > > For this kind of unwinding scenario to wokr, the userspace environment > > would need to provide correct unwind info for _everything_ rather than > > relying on the frame chain on the stack alone, so this scenario isn't > > applicable to C. > > > > I'm not saying we should try to support this, but a comment to indicate > > what we are and are not trying to do might be a good idea. > > > > How about something along these lines: > > > > /* > > * Don't try to provide unwind into for the other regs of the > > * interrupted context here. C/C++ based runtimes don't rely on > > * this for unwinding in practice. Debuggers need more, but they > > * already have baked-in knowledge about how to unwind out of > > * signals. > > */ > > I'll fold that in, thanks. Thanks. This just avoids having to ask the question again or go back over all the messy rationale above. If someone _needs_ this to be extended in future, we can revisit it. But I hope not! Cheers ---Dave _______________________________________________ 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] 13+ messages in thread
* Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 16:28 ` [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon 2020-05-20 9:42 ` Dave Martin @ 2020-05-20 10:48 ` Will Deacon 2020-05-20 11:06 ` Dave Martin 1 sibling, 1 reply; 13+ messages in thread From: Will Deacon @ 2020-05-20 10:48 UTC (permalink / raw) To: linux-arm-kernel Cc: Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin, Daniel Kiss On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote: > 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 | 40 ++++++++++++++++++++++++------ > 1 file changed, 33 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > index 0c921130002a..cb47dfb3bd5a 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,34 @@ > > .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. > + */ > + .cfi_def_cfa x29, 0 > + .cfi_offset x29, 0 * 8 > + .cfi_offset x29, 1 * 8 Oops, just spotted this bug: second entry should be x30. Will _______________________________________________ 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] 13+ messages in thread
* Re: [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-20 10:48 ` Will Deacon @ 2020-05-20 11:06 ` Dave Martin 0 siblings, 0 replies; 13+ messages in thread From: Dave Martin @ 2020-05-20 11:06 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Wed, May 20, 2020 at 11:48:03AM +0100, Will Deacon wrote: > On Tue, May 19, 2020 at 05:28:21PM +0100, Will Deacon wrote: > > 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 | 40 ++++++++++++++++++++++++------ > > 1 file changed, 33 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > > index 0c921130002a..cb47dfb3bd5a 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,34 @@ > > > > .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. > > + */ > > + .cfi_def_cfa x29, 0 > > + .cfi_offset x29, 0 * 8 > > + .cfi_offset x29, 1 * 8 > > Oops, just spotted this bug: second entry should be x30. Dang, didn't spot that. Yes. Must have been momory corruption in your editor... Cheers ---Dave _______________________________________________ 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] 13+ messages in thread
end of thread, other threads:[~2020-05-20 11:06 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-19 16:28 [PATCH v2 0/2] arm64 sigreturn unwinding fixes Will Deacon 2020-05-19 16:28 ` [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon 2020-05-19 16:33 ` Mark Brown 2020-05-20 9:33 ` Dave Martin 2020-05-20 9:53 ` Will Deacon 2020-05-19 16:28 ` [PATCH v2 2/2] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon 2020-05-20 9:42 ` Dave Martin 2020-05-20 9:50 ` Will Deacon 2020-05-20 10:27 ` Dave Martin 2020-05-20 10:36 ` Will Deacon 2020-05-20 11:03 ` Dave Martin 2020-05-20 10:48 ` Will Deacon 2020-05-20 11:06 ` Dave Martin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.