Skip to contentSkip to conversation

arm64: add an MTE support check to the top of mte_thread_switch()

PATCH In review Peter Collingbourne 2 participants 5 messages Sep 15, 2021 – Sep 24, 2021

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v3 1/2 arm64: add an MTE support check to the top of mte_thread_switch()

This lets us avoid doing unnecessary work on hardware that does not support MTE, and will allow us to freely use MTE instructions in the code called by mte_thread_switch().

Since this would mean that we do a redundant check in mte_check_tfsr_el1(), remove it and add two checks now required in its callers.

---
v3:
- remove check from mte_check_tfsr_el1()

 arch/arm64/include/asm/mte.h |  6 ++++++
 arch/arm64/kernel/mte.c      | 10 ++++------
 2 files changed, 10 insertions(+), 6 deletions(-)
arch/arm64/include/asm/mte.h +6 -0
index 3f93b9e0b339..02511650cffe 100644
@@ -99,11 +99,17 @@ void mte_check_tfsr_el1(void);
 
 static inline void mte_check_tfsr_entry(void)
 {
+	if (!system_supports_mte())
+		return;
+
 	mte_check_tfsr_el1();
 }
 
 static inline void mte_check_tfsr_exit(void)
 {
+	if (!system_supports_mte())
+		return;
+
 	/*
 	 * The asynchronous faults are sync'ed automatically with
 	 * TFSR_EL1 on kernel entry but for exit an explicit dsb()
arch/arm64/kernel/mte.c +4 -6
index 9d314a3bad3b..e5e801bc5312 100644
@@ -142,12 +142,7 @@ void mte_enable_kernel_async(void)
 #ifdef CONFIG_KASAN_HW_TAGS
 void mte_check_tfsr_el1(void)
 {
-	u64 tfsr_el1;
-
-	if (!system_supports_mte())
-		return;
-
-	tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
+	u64 tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
 
 	if (unlikely(tfsr_el1 & SYS_TFSR_EL1_TF1)) {
 		/*
@@ -199,6 +194,9 @@ void mte_thread_init_user(void)
 
 void mte_thread_switch(struct task_struct *next)
 {
+	if (!system_supports_mte())
+		return;
+
 	mte_update_sctlr_user(next);
 
 	/*
Signature · 7 lines
2.33.0.309.g3052b89438-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#1 lore raw

PATCH v3 2/2 arm64: kasan: mte: move GCR_EL1 switch to task switch when KASAN disabled

It is not necessary to write to GCR_EL1 on every kernel entry and exit when HW tag-based KASAN is disabled because the kernel will not execute any IRG instructions in that mode. Since accessing GCR_EL1 can be expensive on some microarchitectures, avoid doing so by moving the access to task switch when HW tag-based KASAN is disabled.

---
v2:
- add prerequisite patch to fix booting on non-MTE hardware

 arch/arm64/kernel/entry.S | 10 +++++-----
 arch/arm64/kernel/mte.c   | 24 +++++++++++++++++++++---
 include/linux/kasan.h     |  9 +++++++--
 3 files changed, 33 insertions(+), 10 deletions(-)
arch/arm64/kernel/entry.S +5 -5
index bc6d5a970a13..2f69ae43941d 100644
@@ -168,9 +168,9 @@ alternative_else_nop_endif
 
 	.macro mte_set_kernel_gcr, tmp, tmp2
 #ifdef CONFIG_KASAN_HW_TAGS
-alternative_if_not ARM64_MTE
+alternative_cb	kasan_hw_tags_enable
 	b	1f
-alternative_else_nop_endif
+alternative_cb_end
 	mov	\tmp, KERNEL_GCR_EL1
 	msr_s	SYS_GCR_EL1, \tmp
 1:
@@ -178,10 +178,10 @@ alternative_else_nop_endif
 	.endm
 
 	.macro mte_set_user_gcr, tsk, tmp, tmp2
-#ifdef CONFIG_ARM64_MTE
-alternative_if_not ARM64_MTE
+#ifdef CONFIG_KASAN_HW_TAGS
+alternative_cb	kasan_hw_tags_enable
 	b	1f
-alternative_else_nop_endif
+alternative_cb_end
 	ldr	\tmp, [\tsk, #THREAD_MTE_CTRL]
 
 	mte_set_gcr \tmp, \tmp2
arch/arm64/kernel/mte.c +21 -3
index e5e801bc5312..bb8204d4c5d3 100644
@@ -157,7 +157,7 @@ void mte_check_tfsr_el1(void)
 }
 #endif
 
-static void mte_update_sctlr_user(struct task_struct *task)
+static void mte_update_sctlr_user_and_gcr_excl(struct task_struct *task)
 {
 	/*
 	 * This must be called with preemption disabled and can only be called
@@ -177,6 +177,24 @@ static void mte_update_sctlr_user(struct task_struct *task)
 	else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
 		sctlr |= SCTLR_EL1_TCF0_SYNC;
 	task->thread.sctlr_user = sctlr;
+
+	/*
+	 * SYS_GCR_EL1 will be set to current->thread.mte_ctrl value by
+	 * mte_set_user_gcr() in kernel_exit, but only if KASAN is enabled.
+	 */
+	if (!kasan_hw_tags_enabled())
+		write_sysreg_s(((mte_ctrl >> MTE_CTRL_GCR_USER_EXCL_SHIFT) &
+				SYS_GCR_EL1_EXCL_MASK) | SYS_GCR_EL1_RRND,
+			       SYS_GCR_EL1);
+}
+
+void __init kasan_hw_tags_enable(struct alt_instr *alt, __le32 *origptr,
+				 __le32 *updptr, int nr_inst)
+{
+	BUG_ON(nr_inst != 1); /* Branch -> NOP */
+
+	if (kasan_hw_tags_enabled())
+		*updptr = cpu_to_le32(aarch64_insn_gen_nop());
 }
 
 void mte_thread_init_user(void)
@@ -197,7 +215,7 @@ void mte_thread_switch(struct task_struct *next)
 	if (!system_supports_mte())
 		return;
 
-	mte_update_sctlr_user(next);
+	mte_update_sctlr_user_and_gcr_excl(next);
 
 	/*
 	 * Check if an async tag exception occurred at EL1.
@@ -242,7 +260,7 @@ long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 	task->thread.mte_ctrl = mte_ctrl;
 	if (task == current) {
 		preempt_disable();
-		mte_update_sctlr_user(task);
+		mte_update_sctlr_user_and_gcr_excl(task);
 		update_sctlr_el1(task->thread.sctlr_user);
 		preempt_enable();
 	}
include/linux/kasan.h +7 -2
index dd874a1ee862..de5f5913374d 100644
@@ -89,7 +89,7 @@ static __always_inline bool kasan_enabled(void)
 	return static_branch_likely(&kasan_flag_enabled);
 }
 
-static inline bool kasan_has_integrated_init(void)
+static inline bool kasan_hw_tags_enabled(void)
 {
 	return kasan_enabled();
 }
@@ -104,7 +104,7 @@ static inline bool kasan_enabled(void)
 	return IS_ENABLED(CONFIG_KASAN);
 }
 
-static inline bool kasan_has_integrated_init(void)
+static inline bool kasan_hw_tags_enabled(void)
 {
 	return false;
 }
@@ -125,6 +125,11 @@ static __always_inline void kasan_free_pages(struct page *page,
 
 #endif /* CONFIG_KASAN_HW_TAGS */
 
+static inline bool kasan_has_integrated_init(void)
+{
+	return kasan_hw_tags_enabled();
+}
+
 #ifdef CONFIG_KASAN
 
 struct kasan_cache {
Signature · 7 lines
2.33.0.309.g3052b89438-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
maintainer Reviewed-by on v3 2/2
#2 lore raw

Re: [PATCH v3 2/2] arm64: kasan: mte: move GCR_EL1 switch to task switch when KASAN disabled

Quoted 9 lines · Peter Collingbourne
arch/arm64/kernel/mte.c +1 -1
-static void mte_update_sctlr_user(struct task_struct *task)
+static void mte_update_sctlr_user_and_gcr_excl(struct task_struct *task)
 {
 	/*
 	 * This must be called with preemption disabled and can only be called
@@ -177,6 +177,24 @@ static void mte_update_sctlr_user(struct task_struct *task)
 	else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
 		sctlr |= SCTLR_EL1_TCF0_SYNC;
 	task->thread.sctlr_user = sctlr;

arch/arm64/kernel/mte.c static void mte_update_sctlr_user(struct task_struct *task) · line 189

Quoted 10 lines
arch/arm64/kernel/mte.c +10 -0
+
+	/*
+	 * SYS_GCR_EL1 will be set to current->thread.mte_ctrl value by
+	 * mte_set_user_gcr() in kernel_exit, but only if KASAN is enabled.
+	 */
+	if (!kasan_hw_tags_enabled())
+		write_sysreg_s(((mte_ctrl >> MTE_CTRL_GCR_USER_EXCL_SHIFT) &
+				SYS_GCR_EL1_EXCL_MASK) | SYS_GCR_EL1_RRND,
+			       SYS_GCR_EL1);
+}

The only nitpick I have is that I'd like to keep the GCR_EL1 updating in a separate function, mte_update_gcr_user() or something (it can be static inline) that you'd explicitly call from mte_thread_switch() and set_mte_ctrl(). The original mte_update_sctlr_user() was not writing any system registers.

Maybe a better alternative would be to move the update_sctlr_el1() call in mte_update_sctlr_user() as well (and rename it more generically to mte_update_user_ctrl()) but with a new bool update_sctlr argument that's false on the thread switch path.

Either way:

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#3 lore raw

Re: [PATCH v3 2/2] arm64: kasan: mte: move GCR_EL1 switch to task switch when KASAN disabled

Quoted 19 lines · Catalin Marinas
Quoted 17 lines · Peter Collingbourne
arch/arm64/kernel/mte.c +9 -1
-static void mte_update_sctlr_user(struct task_struct *task)
+static void mte_update_sctlr_user_and_gcr_excl(struct task_struct *task)
 {
      /*
       * This must be called with preemption disabled and can only be called
@@ -177,6 +177,24 @@ static void mte_update_sctlr_user(struct task_struct *task)
      else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
              sctlr |= SCTLR_EL1_TCF0_SYNC;
      task->thread.sctlr_user = sctlr;
+
+     /*
+      * SYS_GCR_EL1 will be set to current->thread.mte_ctrl value by
+      * mte_set_user_gcr() in kernel_exit, but only if KASAN is enabled.
+      */
+     if (!kasan_hw_tags_enabled())
+             write_sysreg_s(((mte_ctrl >> MTE_CTRL_GCR_USER_EXCL_SHIFT) &
+                             SYS_GCR_EL1_EXCL_MASK) | SYS_GCR_EL1_RRND,
Quoted 8 lines

arch/arm64/kernel/mte.c static void mte_update_sctlr_user(struct task_struct *task) · line 189

Quoted 2 lines
arch/arm64/kernel/mte.c +2 -0
+                            SYS_GCR_EL1);
+}

The only nitpick I have is that I'd like to keep the GCR_EL1 updating in a separate function, mte_update_gcr_user() or something (it can be static inline) that you'd explicitly call from mte_thread_switch() and set_mte_ctrl(). The original mte_update_sctlr_user() was not writing any system registers.

Done in v4. (I mildly prefer the way I originally wrote it as I think it's less error prone, but I don't mind that much.) Sorry, I forgot to add your Reviewed-by.

Peter

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

maintainer
#4 lore raw

Re: (subset) [PATCH v3 1/2] arm64: add an MTE support check to the top of mte_thread_switch()

In reply to Peter Collingbourne
Quoted 7 lines · Peter Collingbourne

This lets us avoid doing unnecessary work on hardware that does not support MTE, and will allow us to freely use MTE instructions in the code called by mte_thread_switch().

Since this would mean that we do a redundant check in mte_check_tfsr_el1(), remove it and add two checks now required in its callers.

I applied the first patch to arm64 (for-next/fixes), it should appear in 5.15-rc3. It looks to me more like a performance regression with the additional dsb+isb on the syscall path for hw not supporting MTE (and I added a Fixes tag). I'll leave the second patch to Will for 5.16.

Thanks!

[1/2] arm64: add an MTE support check to the top of mte_thread_switch()

      https://git.kernel.org/arm64/c/8c8a3b5bd960
Signature · 7 lines
Catalin


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel