| Message ID | 20200519162821.16857-2-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64 sigreturn unwinding fixes | expand |
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>
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>
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
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)
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(-)