* [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE @ 2021-12-03 13:03 Mark Brown 2021-12-06 16:30 ` Mark Rutland 0 siblings, 1 reply; 7+ messages in thread From: Mark Brown @ 2021-12-03 13:03 UTC (permalink / raw) To: Catalin Marinas, Will Deacon; +Cc: Mark Rutland, linux-arm-kernel, Mark Brown A couple of SYM_CODE sections have added usage of BTI_C which is currently only defined when building for BTI. This means that the users have ugly ifdefs for the case where BTI is disabled so let's provide an empty definition in that case and remove the ifdefs. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/include/asm/linkage.h | 4 ++++ arch/arm64/kernel/entry-ftrace.S | 4 ---- arch/arm64/lib/kasan_sw_tags.S | 2 -- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h index 9906541a6861..9c70136d7f94 100644 --- a/arch/arm64/include/asm/linkage.h +++ b/arch/arm64/include/asm/linkage.h @@ -42,6 +42,10 @@ SYM_START(name, SYM_L_WEAK, SYM_A_NONE) \ BTI_C +#else + +#define BTI_C + #endif /* diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S index 8cf970d219f5..46a2de864794 100644 --- a/arch/arm64/kernel/entry-ftrace.S +++ b/arch/arm64/kernel/entry-ftrace.S @@ -77,17 +77,13 @@ .endm SYM_CODE_START(ftrace_regs_caller) -#ifdef BTI_C BTI_C -#endif ftrace_regs_entry 1 b ftrace_common SYM_CODE_END(ftrace_regs_caller) SYM_CODE_START(ftrace_caller) -#ifdef BTI_C BTI_C -#endif ftrace_regs_entry 0 b ftrace_common SYM_CODE_END(ftrace_caller) diff --git a/arch/arm64/lib/kasan_sw_tags.S b/arch/arm64/lib/kasan_sw_tags.S index 5b04464c045e..a6d6fa2f761e 100644 --- a/arch/arm64/lib/kasan_sw_tags.S +++ b/arch/arm64/lib/kasan_sw_tags.S @@ -38,9 +38,7 @@ * incremented by 256 prior to return). */ SYM_CODE_START(__hwasan_tag_mismatch) -#ifdef BTI_C BTI_C -#endif add x29, sp, #232 stp x2, x3, [sp, #8 * 2] stp x4, x5, [sp, #8 * 4] -- 2.30.2 _______________________________________________ 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] 7+ messages in thread
* Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE 2021-12-03 13:03 [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE Mark Brown @ 2021-12-06 16:30 ` Mark Rutland 2021-12-06 16:52 ` Ard Biesheuvel 2021-12-06 18:05 ` Mark Brown 0 siblings, 2 replies; 7+ messages in thread From: Mark Rutland @ 2021-12-06 16:30 UTC (permalink / raw) To: Mark Brown; +Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, Ard Biesheuvel On Fri, Dec 03, 2021 at 01:03:35PM +0000, Mark Brown wrote: > A couple of SYM_CODE sections have added usage of BTI_C which is > currently only defined when building for BTI. This means that the > users have ugly ifdefs for the case where BTI is disabled so let's > provide an empty definition in that case and remove the ifdefs. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/include/asm/linkage.h | 4 ++++ > arch/arm64/kernel/entry-ftrace.S | 4 ---- > arch/arm64/lib/kasan_sw_tags.S | 2 -- > 3 files changed, 4 insertions(+), 6 deletions(-) Looking around, there are other places that open-code `hint 34`, e.g. arch/arm64/crypto/aes-modes.S. Those are unconditional, so we should probably figure out whether we want those to be conditional (or if we're happy to make the other cases similarly unconditional). I'd argue we should probably place BTIs in assembly unconditionally, on the assumption that they shouldn't have an measureable performance impact in practice (as we're already assuming that when CONFIG_ARM64_BTI_KERNEL is selected anyhow). Thoughts? Regardless, I reckon we should probably define a `bti` macro centrally in arch/arm64/include/asm/assembler.h, something like: | /* | * Branch Target Identifier | */ | .macro bti, targets | .equ .L__bti_targets_c, 1 | .equ .L__bti_targets_j, 2 | .equ .L__bti_targets_jc,3 | .if .L__bti_targets_\targets == .L__bti_targets_c | hint #34 | .elseif .L__bti_targets_\targets == .L__bti_targets_j | hint #36 | .elseif .L__bti_targets_\targets == .L__bti_targets_jc | hint #38 | .else | .error "Unsupported BTI targets '\targets\()'" | .endif | .endm | The `.equ` gunk is because there's no `.elseifc`, and without that we have to have a chain of `.endif` for each `.ifc`, and can't produce a warning reliably. That seems to do the right thing: | [mark@lakrids:~/src/linux]% git diff -- arch/arm64/kernel/head.S | diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S | index 6a98f1a38c29..f6968563bab8 100644 | --- a/arch/arm64/kernel/head.S | +++ b/arch/arm64/kernel/head.S | @@ -89,6 +89,10 @@ | * x24 __primary_switch() .. relocate_kernel() current RELR displacement | */ | SYM_CODE_START(primary_entry) | + bti c | + bti j | + bti jc | + bti no | bl preserve_boot_args | bl init_kernel_el // w0=cpu_boot_mode | adrp x23, __PHYS_OFFSET | [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- arch/arm64/kernel/head.o | Makefile:1811: warning: overriding recipe for target 'arch/arm64/kernel/head.o' | Makefile:1167: warning: ignoring old recipe for target 'arch/arm64/kernel/head.o' | CALL scripts/checksyscalls.sh | CALL scripts/atomic/check-atomics.sh | AS arch/arm64/kernel/head.o | arch/arm64/kernel/head.S: Assembler messages: | arch/arm64/kernel/head.S:95: Error: Unsupported BTI targets 'no' | make[2]: *** [scripts/Makefile.build:388: arch/arm64/kernel/head.o] Error 1 | make[1]: *** [scripts/Makefile.build:549: arch/arm64/kernel] Error 2 | make: *** [Makefile:1846: arch/arm64] Error 2 > diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h > index 9906541a6861..9c70136d7f94 100644 > --- a/arch/arm64/include/asm/linkage.h > +++ b/arch/arm64/include/asm/linkage.h > @@ -42,6 +42,10 @@ > SYM_START(name, SYM_L_WEAK, SYM_A_NONE) \ > BTI_C > > +#else > + > +#define BTI_C > + > #endif Can we simplify the ifdeffery so that we only conditionally define BTI_C, and unconditionally define the sites using it? That way if there's a problem with any of the users that will consistently show up in testing, regardless of whether CONFIG_ARM64_BTI_KERNEL is selected. e.g. we can make this: | #ifdef CONFIG_ARM64_BTI_KERNEL | #define BTI_C hint #34 ; | #else | #define BTI_C | #endif | | #define SYM_FUNC_START(name) \ | SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) \ | BTI_C | | ... ... or if we define an unconditional `bti` macro: | #define SYM_FUNC_START(name) \ | SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) \ | bti c | | ... Thanks, Mark. > /* > diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S > index 8cf970d219f5..46a2de864794 100644 > --- a/arch/arm64/kernel/entry-ftrace.S > +++ b/arch/arm64/kernel/entry-ftrace.S > @@ -77,17 +77,13 @@ > .endm > > SYM_CODE_START(ftrace_regs_caller) > -#ifdef BTI_C > BTI_C > -#endif > ftrace_regs_entry 1 > b ftrace_common > SYM_CODE_END(ftrace_regs_caller) > > SYM_CODE_START(ftrace_caller) > -#ifdef BTI_C > BTI_C > -#endif > ftrace_regs_entry 0 > b ftrace_common > SYM_CODE_END(ftrace_caller) > diff --git a/arch/arm64/lib/kasan_sw_tags.S b/arch/arm64/lib/kasan_sw_tags.S > index 5b04464c045e..a6d6fa2f761e 100644 > --- a/arch/arm64/lib/kasan_sw_tags.S > +++ b/arch/arm64/lib/kasan_sw_tags.S > @@ -38,9 +38,7 @@ > * incremented by 256 prior to return). > */ > SYM_CODE_START(__hwasan_tag_mismatch) > -#ifdef BTI_C > BTI_C > -#endif > add x29, sp, #232 > stp x2, x3, [sp, #8 * 2] > stp x4, x5, [sp, #8 * 4] > -- > 2.30.2 > _______________________________________________ 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] 7+ messages in thread
* Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE 2021-12-06 16:30 ` Mark Rutland @ 2021-12-06 16:52 ` Ard Biesheuvel 2021-12-06 17:11 ` Mark Rutland 2021-12-06 18:05 ` Mark Brown 1 sibling, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2021-12-06 16:52 UTC (permalink / raw) To: Mark Rutland; +Cc: Mark Brown, Catalin Marinas, Will Deacon, Linux ARM On Mon, 6 Dec 2021 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote: > > On Fri, Dec 03, 2021 at 01:03:35PM +0000, Mark Brown wrote: > > A couple of SYM_CODE sections have added usage of BTI_C which is > > currently only defined when building for BTI. This means that the > > users have ugly ifdefs for the case where BTI is disabled so let's > > provide an empty definition in that case and remove the ifdefs. > > > > Signed-off-by: Mark Brown <broonie@kernel.org> > > --- > > arch/arm64/include/asm/linkage.h | 4 ++++ > > arch/arm64/kernel/entry-ftrace.S | 4 ---- > > arch/arm64/lib/kasan_sw_tags.S | 2 -- > > 3 files changed, 4 insertions(+), 6 deletions(-) > > Looking around, there are other places that open-code `hint 34`, e.g. > arch/arm64/crypto/aes-modes.S. Those are unconditional, so we should probably > figure out whether we want those to be conditional (or if we're happy to make > the other cases similarly unconditional). > > I'd argue we should probably place BTIs in assembly unconditionally, on the > assumption that they shouldn't have an measureable performance impact in > practice (as we're already assuming that when CONFIG_ARM64_BTI_KERNEL is > selected anyhow). Thoughts? > From the top of my head, I can't say for sure but there are some computed gotos in the crypto code where removing an instruction may throw off the calculation. So keeping the hint unconditionally makes sense to me, and by the same reasoning, it would be better not to introduce macros that shadow existing instructions if they may resolve to a sequence of a different size. > Regardless, I reckon we should probably define a `bti` macro centrally in > arch/arm64/include/asm/assembler.h, something like: > > | /* > | * Branch Target Identifier > | */ > | .macro bti, targets > | .equ .L__bti_targets_c, 1 > | .equ .L__bti_targets_j, 2 > | .equ .L__bti_targets_jc,3 > | .if .L__bti_targets_\targets == .L__bti_targets_c > | hint #34 > | .elseif .L__bti_targets_\targets == .L__bti_targets_j > | hint #36 > | .elseif .L__bti_targets_\targets == .L__bti_targets_jc > | hint #38 > | .else > | .error "Unsupported BTI targets '\targets\()'" > | .endif > | .endm > | > > The `.equ` gunk is because there's no `.elseifc`, and without that we have to > have a chain of `.endif` for each `.ifc`, and can't produce a warning reliably. > > That seems to do the right thing: > > | [mark@lakrids:~/src/linux]% git diff -- arch/arm64/kernel/head.S > | diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > | index 6a98f1a38c29..f6968563bab8 100644 > | --- a/arch/arm64/kernel/head.S > | +++ b/arch/arm64/kernel/head.S > | @@ -89,6 +89,10 @@ > | * x24 __primary_switch() .. relocate_kernel() current RELR displacement > | */ > | SYM_CODE_START(primary_entry) > | + bti c > | + bti j > | + bti jc > | + bti no > | bl preserve_boot_args > | bl init_kernel_el // w0=cpu_boot_mode > | adrp x23, __PHYS_OFFSET > | [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- arch/arm64/kernel/head.o > | Makefile:1811: warning: overriding recipe for target 'arch/arm64/kernel/head.o' > | Makefile:1167: warning: ignoring old recipe for target 'arch/arm64/kernel/head.o' > | CALL scripts/checksyscalls.sh > | CALL scripts/atomic/check-atomics.sh > | AS arch/arm64/kernel/head.o > | arch/arm64/kernel/head.S: Assembler messages: > | arch/arm64/kernel/head.S:95: Error: Unsupported BTI targets 'no' > | make[2]: *** [scripts/Makefile.build:388: arch/arm64/kernel/head.o] Error 1 > | make[1]: *** [scripts/Makefile.build:549: arch/arm64/kernel] Error 2 > | make: *** [Makefile:1846: arch/arm64] Error 2 > > > diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h > > index 9906541a6861..9c70136d7f94 100644 > > --- a/arch/arm64/include/asm/linkage.h > > +++ b/arch/arm64/include/asm/linkage.h > > @@ -42,6 +42,10 @@ > > SYM_START(name, SYM_L_WEAK, SYM_A_NONE) \ > > BTI_C > > > > +#else > > + > > +#define BTI_C > > + > > #endif > > Can we simplify the ifdeffery so that we only conditionally define BTI_C, and > unconditionally define the sites using it? That way if there's a problem with > any of the users that will consistently show up in testing, regardless of > whether CONFIG_ARM64_BTI_KERNEL is selected. > > e.g. we can make this: > > | #ifdef CONFIG_ARM64_BTI_KERNEL > | #define BTI_C hint #34 ; > | #else > | #define BTI_C > | #endif > | > | #define SYM_FUNC_START(name) \ > | SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) \ > | BTI_C > | > | ... > > ... or if we define an unconditional `bti` macro: > > | #define SYM_FUNC_START(name) \ > | SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN) \ > | bti c > | > | ... > > Thanks, > Mark. > > > /* > > diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S > > index 8cf970d219f5..46a2de864794 100644 > > --- a/arch/arm64/kernel/entry-ftrace.S > > +++ b/arch/arm64/kernel/entry-ftrace.S > > @@ -77,17 +77,13 @@ > > .endm > > > > SYM_CODE_START(ftrace_regs_caller) > > -#ifdef BTI_C > > BTI_C > > -#endif > > ftrace_regs_entry 1 > > b ftrace_common > > SYM_CODE_END(ftrace_regs_caller) > > > > SYM_CODE_START(ftrace_caller) > > -#ifdef BTI_C > > BTI_C > > -#endif > > ftrace_regs_entry 0 > > b ftrace_common > > SYM_CODE_END(ftrace_caller) > > diff --git a/arch/arm64/lib/kasan_sw_tags.S b/arch/arm64/lib/kasan_sw_tags.S > > index 5b04464c045e..a6d6fa2f761e 100644 > > --- a/arch/arm64/lib/kasan_sw_tags.S > > +++ b/arch/arm64/lib/kasan_sw_tags.S > > @@ -38,9 +38,7 @@ > > * incremented by 256 prior to return). > > */ > > SYM_CODE_START(__hwasan_tag_mismatch) > > -#ifdef BTI_C > > BTI_C > > -#endif > > add x29, sp, #232 > > stp x2, x3, [sp, #8 * 2] > > stp x4, x5, [sp, #8 * 4] > > -- > > 2.30.2 > > _______________________________________________ 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] 7+ messages in thread
* Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE 2021-12-06 16:52 ` Ard Biesheuvel @ 2021-12-06 17:11 ` Mark Rutland 0 siblings, 0 replies; 7+ messages in thread From: Mark Rutland @ 2021-12-06 17:11 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Mark Brown, Catalin Marinas, Will Deacon, Linux ARM On Mon, Dec 06, 2021 at 05:52:16PM +0100, Ard Biesheuvel wrote: > On Mon, 6 Dec 2021 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote: > > > > On Fri, Dec 03, 2021 at 01:03:35PM +0000, Mark Brown wrote: > > > A couple of SYM_CODE sections have added usage of BTI_C which is > > > currently only defined when building for BTI. This means that the > > > users have ugly ifdefs for the case where BTI is disabled so let's > > > provide an empty definition in that case and remove the ifdefs. > > > > > > Signed-off-by: Mark Brown <broonie@kernel.org> > > > --- > > > arch/arm64/include/asm/linkage.h | 4 ++++ > > > arch/arm64/kernel/entry-ftrace.S | 4 ---- > > > arch/arm64/lib/kasan_sw_tags.S | 2 -- > > > 3 files changed, 4 insertions(+), 6 deletions(-) > > > > Looking around, there are other places that open-code `hint 34`, e.g. > > arch/arm64/crypto/aes-modes.S. Those are unconditional, so we should probably > > figure out whether we want those to be conditional (or if we're happy to make > > the other cases similarly unconditional). > > > > I'd argue we should probably place BTIs in assembly unconditionally, on the > > assumption that they shouldn't have an measureable performance impact in > > practice (as we're already assuming that when CONFIG_ARM64_BTI_KERNEL is > > selected anyhow). Thoughts? > > From the top of my head, I can't say for sure but there are some > computed gotos in the crypto code where removing an instruction may > throw off the calculation. So keeping the hint unconditionally makes > sense to me, and by the same reasoning, it would be better not to > introduce macros that shadow existing instructions if they may resolve > to a sequence of a different size. As an aside, while that code works, it's using `BTI C` in an odd way, since the those are internal labels rather than callable function entry points (but the `BR` works because of the X16/X17 exemption that makes PLTs work). For consistency, we might want to make that use `BTI J`, and an X register other than X16/X17, which is what the compiler should generate for cases like this. That would minimize the set of targets the `BR` can legitimately hit (though AFAICT this cannot be gadgetized anyway, so this'd just be for consistency/lack-of-surprise). Thanks, Mark. _______________________________________________ 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] 7+ messages in thread
* Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE 2021-12-06 16:30 ` Mark Rutland 2021-12-06 16:52 ` Ard Biesheuvel @ 2021-12-06 18:05 ` Mark Brown 2021-12-06 18:38 ` Mark Rutland 1 sibling, 1 reply; 7+ messages in thread From: Mark Brown @ 2021-12-06 18:05 UTC (permalink / raw) To: Mark Rutland Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, Ard Biesheuvel [-- Attachment #1.1: Type: text/plain, Size: 2810 bytes --] On Mon, Dec 06, 2021 at 04:30:41PM +0000, Mark Rutland wrote: > Looking around, there are other places that open-code `hint 34`, e.g. > arch/arm64/crypto/aes-modes.S. Those are unconditional, so we should probably > figure out whether we want those to be conditional (or if we're happy to make > the other cases similarly unconditional). It seems to *just* be aes-modes.S AFAICT and that does rely on having the BTIs actually emitted (well, and kselftest but that can't use any kernel internal helpers so it's not relevant here). > I'd argue we should probably place BTIs in assembly unconditionally, on the > assumption that they shouldn't have an measureable performance impact in > practice (as we're already assuming that when CONFIG_ARM64_BTI_KERNEL is > selected anyhow). Thoughts? I'm not sure what you mean here? We do already add BTIs unconditionally to SYM_FUNC when BTI is enabled for the kernel, it's only SYM_CODE where we don't. For SYM_CODE it seems unwise to add the BTIs since there's things like the vectors that get covered and it's not really idiomatic for SYM_CODE's whole "I know exactly what I'm doing" thing. I also don't know if we end up with SYM_CODEs for anything particularly space constrained other than the vectors, I can't think of anything. If you mean that the macro should unconditionally emit BTI when the macro is used then the main reason I didn't do that was for consistency with C code - that will only have BTIs added if we're building with BTI enabled. There is at least one implementation out there which implements unknown HINTs with a performance overhead compared to NOPs so that is one of the reasons why the kernel might be built with BTI disabled. > Regardless, I reckon we should probably define a `bti` macro centrally in > arch/arm64/include/asm/assembler.h, something like: > That seems to do the right thing: Yes, even seems to do the right thing without complaint if the assembler is configured v8.5 and so understands v8.5. My main worry would be the potential confusion caused when people see what looks like it should be a v8.5 instruction in code which is supposed to build configured for v8.0, it looks wrong to see something that looks like a more modern instruction in code that should be buildable with a toolchain that doesn't understand it. > Can we simplify the ifdeffery so that we only conditionally define BTI_C, and > unconditionally define the sites using it? That way if there's a problem with > any of the users that will consistently show up in testing, regardless of > whether CONFIG_ARM64_BTI_KERNEL is selected. > > e.g. we can make this: > > | #ifdef CONFIG_ARM64_BTI_KERNEL > | #define BTI_C hint #34 ; > | #else > | #define BTI_C > | #endif Should work. [-- 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] 7+ messages in thread
* Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE 2021-12-06 18:05 ` Mark Brown @ 2021-12-06 18:38 ` Mark Rutland 2021-12-06 19:31 ` Mark Brown 0 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2021-12-06 18:38 UTC (permalink / raw) To: Mark Brown; +Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, Ard Biesheuvel On Mon, Dec 06, 2021 at 06:05:44PM +0000, Mark Brown wrote: > On Mon, Dec 06, 2021 at 04:30:41PM +0000, Mark Rutland wrote: > > > Looking around, there are other places that open-code `hint 34`, e.g. > > arch/arm64/crypto/aes-modes.S. Those are unconditional, so we should probably > > figure out whether we want those to be conditional (or if we're happy to make > > the other cases similarly unconditional). > > It seems to *just* be aes-modes.S AFAICT and that does rely on having > the BTIs actually emitted (well, and kselftest but that can't use any > kernel internal helpers so it's not relevant here). > > > I'd argue we should probably place BTIs in assembly unconditionally, on the > > assumption that they shouldn't have an measureable performance impact in > > practice (as we're already assuming that when CONFIG_ARM64_BTI_KERNEL is > > selected anyhow). Thoughts? > > I'm not sure what you mean here? We do already add BTIs unconditionally > to SYM_FUNC when BTI is enabled for the kernel, it's only SYM_CODE where > we don't. I mean that regardless of whether BTI is enabled for the kernel, all SYM_FUNC_*() code should get an actual BTI instruction, and if we have a macro (be it `BTI_C`, `bti`, or otherwise), that should output an actual BTI instruction. Where explicitly use the macro in SYM_CODE_*() code, that would have an actual BTI, but we wouldn't insert it automatically into SYM_CODE_*(). > If you mean that the macro should unconditionally emit BTI when the > macro is used then the main reason I didn't do that was for consistency > with C code - that will only have BTIs added if we're building with BTI > enabled. There is at least one implementation out there which implements > unknown HINTs with a performance overhead compared to NOPs so that is > one of the reasons why the kernel might be built with BTI disabled. I appreciate that rationale; I'm saying we should re-evaluate that. I expect distros will (eventually) enable BTI for usersapce (where perf impact likely matters more), defconfig has BTI enabled (so people will be using that by default), and the general expectation is that HINT space instructions should be fast when they're NOPs. I suspect that even if a particular CPU has expensive HINTs, since asm functions are called more rarely than C functions, it's likely in the noise anyway. If this does actually matter in practice, then I'm happy to make it conditional, I just think we should err on the side of simplicity otherwise. Especially as we already have on case where we add the instructions unconditionally, in what is arguably fast-path code! > > Regardless, I reckon we should probably define a `bti` macro centrally in > > arch/arm64/include/asm/assembler.h, something like: > > > That seems to do the right thing: > > Yes, even seems to do the right thing without complaint if the assembler > is configured v8.5 and so understands v8.5. My main worry would be the > potential confusion caused when people see what looks like it should be > a v8.5 instruction in code which is supposed to build configured for > v8.0, it looks wrong to see something that looks like a more modern > instruction in code that should be buildable with a toolchain that > doesn't understand it. Sure, but that's already the case for SB and ESB, so I'm not sure that matters. > > Can we simplify the ifdeffery so that we only conditionally define BTI_C, and > > unconditionally define the sites using it? That way if there's a problem with > > any of the users that will consistently show up in testing, regardless of > > whether CONFIG_ARM64_BTI_KERNEL is selected. > > > > e.g. we can make this: > > > > | #ifdef CONFIG_ARM64_BTI_KERNEL > > | #define BTI_C hint #34 ; > > | #else > > | #define BTI_C > > | #endif > > Should work. Cool. Thanks, Mark. _______________________________________________ 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] 7+ messages in thread
* Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE 2021-12-06 18:38 ` Mark Rutland @ 2021-12-06 19:31 ` Mark Brown 0 siblings, 0 replies; 7+ messages in thread From: Mark Brown @ 2021-12-06 19:31 UTC (permalink / raw) To: Mark Rutland Cc: Catalin Marinas, Will Deacon, linux-arm-kernel, Ard Biesheuvel [-- Attachment #1.1: Type: text/plain, Size: 2719 bytes --] On Mon, Dec 06, 2021 at 06:38:46PM +0000, Mark Rutland wrote: > On Mon, Dec 06, 2021 at 06:05:44PM +0000, Mark Brown wrote: > > On Mon, Dec 06, 2021 at 04:30:41PM +0000, Mark Rutland wrote: > > If you mean that the macro should unconditionally emit BTI when the > > macro is used then the main reason I didn't do that was for consistency > > with C code - that will only have BTIs added if we're building with BTI > > enabled. There is at least one implementation out there which implements > > unknown HINTs with a performance overhead compared to NOPs so that is > > one of the reasons why the kernel might be built with BTI disabled. > I appreciate that rationale; I'm saying we should re-evaluate that. ... > If this does actually matter in practice, then I'm happy to make it > conditional, I just think we should err on the side of simplicity otherwise. > Especially as we already have on case where we add the instructions > unconditionally, in what is arguably fast-path code! Hrm. I'm not sure I see that as a substantial win TBH - it does make the assembler output more consistent but it's also not what I'd expect to happen if we turn off BTI for the kernel. I guess it's just the usage in the AES code that's making you think of this - all the issues have been missing landing pads in SYM_CODE? I'm not exactly against it, I'm just unclear on the upside which makes me feel like I'm missing something. It feels like this comes from the current conflation of conditionally providing the macro with conditionally using it in sites where it can be omitted. My instinct is more to do something like define your BTI C macro unconditionally but still have a conditional thing like we have currently at least for SYM_FUNC, possibly renamed though. Actually now I think about it we may also want to think about pointer auth for SYM_FUNC, but that's definitely a separate and more substantial thing that needs much more consideration and is out of scope here. > > Yes, even seems to do the right thing without complaint if the assembler > > is configured v8.5 and so understands v8.5. My main worry would be the > > potential confusion caused when people see what looks like it should be > > a v8.5 instruction in code which is supposed to build configured for > > v8.0, it looks wrong to see something that looks like a more modern > > instruction in code that should be buildable with a toolchain that > > doesn't understand it. > Sure, but that's already the case for SB and ESB, so I'm not sure that matters. Oh, ick. I may perhaps be more sensitive to this than most due to working with the FP code which has more hand assembly than average. [-- 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] 7+ messages in thread
end of thread, other threads:[~2021-12-06 19:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-03 13:03 [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE Mark Brown 2021-12-06 16:30 ` Mark Rutland 2021-12-06 16:52 ` Ard Biesheuvel 2021-12-06 17:11 ` Mark Rutland 2021-12-06 18:05 ` Mark Brown 2021-12-06 18:38 ` Mark Rutland 2021-12-06 19:31 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox