| Message ID | 20200624112253.1602786-1-jean-philippe@linaro.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2d21889f8b5c50f65f5162bc972b0b1626b97be2 |
| Headers | show |
| Series | arm64: Don't insert a BTI instruction at inner labels | expand |
On Wed, Jun 24, 2020 at 01:22:54PM +0200, Jean-Philippe Brucker wrote: > It turns out we don't currently need the BTI landing pads inserted by > SYM_INNER_LABEL: > * ftrace_call and ftrace_graph_call are only used for runtime patching > of the active tracer. The patched code is not reached from a branch. > * install_el2_stub is reached from a CBZ instruction, which doesn't > change PSTATE.BTYPE. > * __guest_exit is reached from B instructions in the hyp-entry vectors, > which aren't subject to BTI checks either. > Remove the BTI annotation from SYM_INNER_LABEL. This fixes things for now but it feels like it's going to be fragile in the long run since inner labels are a bit of a wild west in terms of how they're going to be referenced - I can't think of a better solution at this level though :( Reviewed-by: Mark Brown <broonie@kernel.org>
On Wed, Jun 24, 2020 at 02:21:14PM +0100, Mark Brown wrote: > On Wed, Jun 24, 2020 at 01:22:54PM +0200, Jean-Philippe Brucker wrote: > > > It turns out we don't currently need the BTI landing pads inserted by > > SYM_INNER_LABEL: > > > * ftrace_call and ftrace_graph_call are only used for runtime patching > > of the active tracer. The patched code is not reached from a branch. > > * install_el2_stub is reached from a CBZ instruction, which doesn't > > change PSTATE.BTYPE. > > * __guest_exit is reached from B instructions in the hyp-entry vectors, > > which aren't subject to BTI checks either. > > > Remove the BTI annotation from SYM_INNER_LABEL. > > This fixes things for now but it feels like it's going to be fragile in > the long run since inner labels are a bit of a wild west in terms of how > they're going to be referenced - I can't think of a better solution at > this level though :( > > Reviewed-by: Mark Brown <broonie@kernel.org> Since inner labels requiring landing pads are going to be the exception rather than the rule, perhaps we can introduce a different macro for this. It feels arch-specific to me (indeed, inner labels are kind of arch-specific, since they're inevitably in the middle of some asm that is unlikely to be handled by core code). Do we know of any code that requires landing pads on inner labels? The uaccess fault stuff probably doesn't, because the error path is reached via ERET. I wondered about the suspend/resume code in sleep.S, but I don't see any inner labels in there. Cheers ---Dave
On Wed, 24 Jun 2020 13:22:54 +0200, Jean-Philippe Brucker wrote: > Some ftrace features are broken since commit 714a8d02ca4d ("arm64: asm: > Override SYM_FUNC_START when building the kernel with BTI"). For example > the function_graph tracer: > > $ echo function_graph > /sys/kernel/debug/tracing/current_tracer > [ 36.107016] WARNING: CPU: 0 PID: 115 at kernel/trace/ftrace.c:2691 ftrace_modify_all_code+0xc8/0x14c > > [...] Applied to arm64 (for-next/fixes), thanks! [1/1] arm64: Don't insert a BTI instruction at inner labels https://git.kernel.org/arm64/c/2d21889f8b5c Cheers,
On Wed, Jun 24, 2020 at 02:44:25PM +0100, Dave Martin wrote: > Since inner labels requiring landing pads are going to be the exception > rather than the rule, perhaps we can introduce a different macro for > this. It feels arch-specific to me (indeed, inner labels are kind of > arch-specific, since they're inevitably in the middle of some asm that > is unlikely to be handled by core code). Yes, we'd need a different macro (or argument to it or something). > Do we know of any code that requires landing pads on inner labels? There aren't any, that was what Jean-Philippe's analysis in the changelog covered.
diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h index 81fefd2a1d023..ba89a9af820ab 100644 --- a/arch/arm64/include/asm/linkage.h +++ b/arch/arm64/include/asm/linkage.h @@ -12,7 +12,6 @@ * instead. */ #define BTI_C hint 34 ; -#define BTI_J hint 36 ; /* * When using in-kernel BTI we need to ensure that PCS-conformant assembly @@ -43,11 +42,6 @@ SYM_START(name, SYM_L_WEAK, SYM_A_NONE) \ BTI_C -#define SYM_INNER_LABEL(name, linkage) \ - .type name SYM_T_NONE ASM_NL \ - SYM_ENTRY(name, linkage, SYM_A_NONE) \ - BTI_J - #endif /*
Some ftrace features are broken since commit 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI"). For example the function_graph tracer: $ echo function_graph > /sys/kernel/debug/tracing/current_tracer [ 36.107016] WARNING: CPU: 0 PID: 115 at kernel/trace/ftrace.c:2691 ftrace_modify_all_code+0xc8/0x14c When ftrace_modify_graph_caller() attempts to write a branch at ftrace_graph_call, it finds the "BTI J" instruction inserted by SYM_INNER_LABEL() instead of a NOP, and aborts. It turns out we don't currently need the BTI landing pads inserted by SYM_INNER_LABEL: * ftrace_call and ftrace_graph_call are only used for runtime patching of the active tracer. The patched code is not reached from a branch. * install_el2_stub is reached from a CBZ instruction, which doesn't change PSTATE.BTYPE. * __guest_exit is reached from B instructions in the hyp-entry vectors, which aren't subject to BTI checks either. Remove the BTI annotation from SYM_INNER_LABEL. Fixes: 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI") Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> --- Tested on QEMU with and without BTI, but only ftrace not KVM. --- arch/arm64/include/asm/linkage.h | 6 ------ 1 file changed, 6 deletions(-)