From: Dave Martin <Dave.Martin@arm•com>
To: Will Deacon <will@kernel•org>
Cc: Tamas Zsoldos <tamas.zsoldos@arm•com>,
Mark Brown <broonie@kernel•org>,
kernel-team@android•com, linux-arm-kernel@lists•infradead.org,
Daniel Kiss <daniel.kiss@arm•com>
Subject: Re: [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Date: Wed, 20 May 2020 10:33:55 +0100 [thread overview]
Message-ID: <20200520093354.GJ5031@arm.com> (raw)
In-Reply-To: <20200519162821.16857-2-will@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>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-05-20 9:34 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200520093354.GJ5031@arm.com \
--to=dave.martin@arm$(echo .)com \
--cc=broonie@kernel$(echo .)org \
--cc=daniel.kiss@arm$(echo .)com \
--cc=kernel-team@android$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=tamas.zsoldos@arm$(echo .)com \
--cc=will@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox