[PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[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

Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Mark Rutland]

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

Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Ard Biesheuvel]

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

Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Mark Rutland]

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

Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Mark Brown]

[-- 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

Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Mark Rutland]

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

Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Mark Brown]

[-- 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