[PATCH v3 0/4] arm64: ARMv8.5-A: MTE: Add async mode support

[PATCH v3 0/4] arm64: ARMv8.5-A: MTE: Add async mode support

[Vincenzo Frascino]

This patchset implements the asynchronous mode support for ARMv8.5-A
Memory Tagging Extension (MTE), which is a debugging feature that allows
to detect with the help of the architecture the C and C++ programmatic
memory errors like buffer overflow, use-after-free, use-after-return, etc.

MTE is built on top of the AArch64 v8.0 virtual address tagging TBI
(Top Byte Ignore) feature and allows a task to set a 4 bit tag on any
subset of its address space that is multiple of a 16 bytes granule. MTE
is based on a lock-key mechanism where the lock is the tag associated to
the physical memory and the key is the tag associated to the virtual
address.
When MTE is enabled and tags are set for ranges of address space of a task,
the PE will compare the tag related to the physical memory with the tag
related to the virtual address (tag check operation). Access to the memory
is granted only if the two tags match. In case of mismatch the PE will raise
an exception.

The exception can be handled synchronously or asynchronously. When the
asynchronous mode is enabled:
  - Upon fault the PE updates the TFSR_EL1 register.
  - The kernel detects the change during one of the following:
    - Context switching
    - Return to user/EL0
    - Kernel entry from EL1
    - Kernel exit to EL1
  - If the register has been updated by the PE the kernel clears it and
    reports the error.

The series contains as well an optimization to mte_assign_mem_tag_range().

The series is based on linux 5.11-rc3.

To simplify the testing a tree with the new patches on top has been made
available at [1].

[1] https://git.gitlab.arm.com/linux-arm/linux-vf.git mte/v10.async

Changes:
--------
v3:
  - Exposed kasan_hw_tags_mode to convert the internal
    KASAN represenetation.
  - Added dsb() for kernel exit paths in arm64.
  - Addressed review comments.
v2:
  - Fixed a compilation issue reported by krobot.
  - General cleanup.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Marco Elver <elver@google.com>
Cc: Evgenii Stepanov <eugenis@google.com>
Cc: Branislav Rankov <Branislav.Rankov@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Vincenzo Frascino (4):
  kasan, arm64: Add KASAN light mode
  arm64: mte: Add asynchronous mode support
  arm64: mte: Enable async tag check fault
  arm64: mte: Optimize mte_assign_mem_tag_range()

 arch/arm64/include/asm/memory.h    |  2 +-
 arch/arm64/include/asm/mte-kasan.h |  5 ++-
 arch/arm64/include/asm/mte.h       | 47 +++++++++++++++++++++-
 arch/arm64/kernel/entry-common.c   | 11 ++++++
 arch/arm64/kernel/mte.c            | 63 ++++++++++++++++++++++++++++--
 arch/arm64/lib/mte.S               | 15 -------
 include/linux/kasan.h              |  1 +
 include/linux/kasan_def.h          | 10 +++++
 mm/kasan/hw_tags.c                 | 19 ++++++++-
 mm/kasan/kasan.h                   |  2 +-
 10 files changed, 151 insertions(+), 24 deletions(-)
 create mode 100644 include/linux/kasan_def.h

-- 
2.30.0

[PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Vincenzo Frascino]

Architectures supported by KASAN HW can provide a light mode of
execution. On an MTE enabled arm64 hw for example this can be identified
with the asynch mode of execution.
In this mode, if a tag check fault occurs, the TFSR_EL1 register is
updated asynchronously. The kernel checks the corresponding bits
periodically.

KASAN requires a specific mode of execution to make use of this hw feature.

Add KASAN HW light execution mode.

Note: This patch adds the KASAN_ARG_MODE_LIGHT config option and the
"light" kernel command line option to enable the described feature.
This patch introduces the kasan_def.h header to make easier to propagate
the relevant enumerations to the architectural code.

Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/memory.h    |  2 +-
 arch/arm64/include/asm/mte-kasan.h |  5 +++--
 arch/arm64/kernel/mte.c            |  2 +-
 include/linux/kasan.h              |  1 +
 include/linux/kasan_def.h          | 10 ++++++++++
 mm/kasan/hw_tags.c                 | 19 ++++++++++++++++++-
 mm/kasan/kasan.h                   |  2 +-
 7 files changed, 35 insertions(+), 6 deletions(-)
 create mode 100644 include/linux/kasan_def.h

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 18fce223b67b..3a7c5beb7096 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -231,7 +231,7 @@ static inline const void *__tag_set(const void *addr, u8 tag)
 }
 
 #ifdef CONFIG_KASAN_HW_TAGS
-#define arch_enable_tagging()			mte_enable_kernel()
+#define arch_enable_tagging(mode)		mte_enable_kernel(mode)
 #define arch_init_tags(max_tag)			mte_init_tags(max_tag)
 #define arch_get_random_tag()			mte_get_random_tag()
 #define arch_get_mem_tag(addr)			mte_get_mem_tag(addr)
diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h
index 26349a4b5e2e..5402f4c8e88d 100644
--- a/arch/arm64/include/asm/mte-kasan.h
+++ b/arch/arm64/include/asm/mte-kasan.h
@@ -9,6 +9,7 @@
 
 #ifndef __ASSEMBLY__
 
+#include <linux/kasan_def.h>
 #include <linux/types.h>
 
 /*
@@ -29,7 +30,7 @@ u8 mte_get_mem_tag(void *addr);
 u8 mte_get_random_tag(void);
 void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag);
 
-void mte_enable_kernel(void);
+void mte_enable_kernel(enum kasan_hw_tags_mode mode);
 void mte_init_tags(u64 max_tag);
 
 #else /* CONFIG_ARM64_MTE */
@@ -52,7 +53,7 @@ static inline void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
 	return addr;
 }
 
-static inline void mte_enable_kernel(void)
+static inline void mte_enable_kernel(enum kasan_hw_tags_mode mode)
 {
 }
 
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index dc9ada64feed..53a6d734e29b 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -151,7 +151,7 @@ void mte_init_tags(u64 max_tag)
 	write_sysreg_s(SYS_GCR_EL1_RRND | gcr_kernel_excl, SYS_GCR_EL1);
 }
 
-void mte_enable_kernel(void)
+void mte_enable_kernel(enum kasan_hw_tags_mode mode)
 {
 	/* Enable MTE Sync Mode for EL1. */
 	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 5e0655fb2a6f..026031444217 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_KASAN_H
 #define _LINUX_KASAN_H
 
+#include <linux/kasan_def.h>
 #include <linux/static_key.h>
 #include <linux/types.h>
 
diff --git a/include/linux/kasan_def.h b/include/linux/kasan_def.h
new file mode 100644
index 000000000000..0a55400809c9
--- /dev/null
+++ b/include/linux/kasan_def.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_KASAN_DEF_H
+#define _LINUX_KASAN_DEF_H
+
+enum kasan_hw_tags_mode {
+	KASAN_HW_TAGS_SYNC,
+	KASAN_HW_TAGS_ASYNC,
+};
+
+#endif /* _LINUX_KASAN_DEF_H */
diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
index 55bd6f09c70f..6c3b0742f639 100644
--- a/mm/kasan/hw_tags.c
+++ b/mm/kasan/hw_tags.c
@@ -22,6 +22,7 @@
 enum kasan_arg_mode {
 	KASAN_ARG_MODE_DEFAULT,
 	KASAN_ARG_MODE_OFF,
+	KASAN_ARG_MODE_LIGHT,
 	KASAN_ARG_MODE_PROD,
 	KASAN_ARG_MODE_FULL,
 };
@@ -60,6 +61,8 @@ static int __init early_kasan_mode(char *arg)
 
 	if (!strcmp(arg, "off"))
 		kasan_arg_mode = KASAN_ARG_MODE_OFF;
+	else if (!strcmp(arg, "light"))
+		kasan_arg_mode = KASAN_ARG_MODE_LIGHT;
 	else if (!strcmp(arg, "prod"))
 		kasan_arg_mode = KASAN_ARG_MODE_PROD;
 	else if (!strcmp(arg, "full"))
@@ -105,9 +108,21 @@ static int __init early_kasan_fault(char *arg)
 }
 early_param("kasan.fault", early_kasan_fault);
 
+static inline int hw_init_mode(enum kasan_arg_mode mode)
+{
+	switch (mode) {
+	case KASAN_ARG_MODE_LIGHT:
+		return KASAN_HW_TAGS_ASYNC;
+	default:
+		return KASAN_HW_TAGS_SYNC;
+	}
+}
+
 /* kasan_init_hw_tags_cpu() is called for each CPU. */
 void kasan_init_hw_tags_cpu(void)
 {
+	enum kasan_hw_tags_mode hw_mode;
+
 	/*
 	 * There's no need to check that the hardware is MTE-capable here,
 	 * as this function is only called for MTE-capable hardware.
@@ -118,7 +133,8 @@ void kasan_init_hw_tags_cpu(void)
 		return;
 
 	hw_init_tags(KASAN_TAG_MAX);
-	hw_enable_tagging();
+	hw_mode = hw_init_mode(kasan_arg_mode);
+	hw_enable_tagging(hw_mode);
 }
 
 /* kasan_init_hw_tags() is called once on boot CPU. */
@@ -145,6 +161,7 @@ void __init kasan_init_hw_tags(void)
 	case KASAN_ARG_MODE_OFF:
 		/* If KASAN is disabled, do nothing. */
 		return;
+	case KASAN_ARG_MODE_LIGHT:
 	case KASAN_ARG_MODE_PROD:
 		static_branch_enable(&kasan_flag_enabled);
 		break;
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index cc4d9e1d49b1..78c09279327e 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -284,7 +284,7 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag)
 #define arch_set_mem_tag_range(addr, size, tag) ((void *)(addr))
 #endif
 
-#define hw_enable_tagging()			arch_enable_tagging()
+#define hw_enable_tagging(mode)			arch_enable_tagging(mode)
 #define hw_init_tags(max_tag)			arch_init_tags(max_tag)
 #define hw_get_random_tag()			arch_get_random_tag()
 #define hw_get_mem_tag(addr)			arch_get_mem_tag(addr)
-- 
2.30.0

[PATCH v3 2/4] arm64: mte: Add asynchronous mode support

[Vincenzo Frascino]

MTE provides an asynchronous mode for detecting tag exceptions. In
particular instead of triggering a fault the arm64 core updates a
register which is checked by the kernel after the asynchronous tag
check fault has occurred.

Add support for MTE asynchronous mode.

The exception handling mechanism will be added with a future patch.

Note: KASAN HW activates async mode via kasan.mode kernel parameter.
The default mode is set to synchronous.
The code that verifies the status of TFSR_EL1 will be added with a
future patch.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/kernel/mte.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 53a6d734e29b..df7a1ae26d7c 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -153,8 +153,30 @@ void mte_init_tags(u64 max_tag)
 
 void mte_enable_kernel(enum kasan_hw_tags_mode mode)
 {
-	/* Enable MTE Sync Mode for EL1. */
-	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
+	const char *m;
+
+	/* Preset parameter values based on the mode. */
+	switch (mode) {
+	case KASAN_HW_TAGS_ASYNC:
+		/* Enable MTE Async Mode for EL1. */
+		sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC);
+		m = "asynchronous";
+		break;
+	case KASAN_HW_TAGS_SYNC:
+		sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
+		m = "synchronous";
+		break;
+	default:
+		/*
+		 * kasan mode should be always set hence we should
+		 * not reach this condition.
+		 */
+		WARN_ON_ONCE(1);
+		return;
+	}
+
+	pr_info_once("MTE: enabled in %s mode at EL1\n", m);
+
 	isb();
 }
 
-- 
2.30.0

[PATCH v3 3/4] arm64: mte: Enable async tag check fault

[Vincenzo Frascino]

MTE provides a mode that asynchronously updates the TFSR_EL1 register
when a tag check exception is detected.

To take advantage of this mode the kernel has to verify the status of
the register at:
  1. Context switching
  2. Return to user/EL0 (Not required in entry from EL0 since the kernel
  did not run)
  3. Kernel entry from EL1
  4. Kernel exit to EL1

If the register is non-zero a trace is reported.

Add the required features for EL1 detection and reporting.

Note: ITFSB bit is set in the SCTLR_EL1 register hence it guaranties that
the indirect writes to TFSR_EL1 are synchronized at exception entry to
EL1. On the context switch path the synchronization is guarantied by the
dsb() in __switch_to().

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h     | 21 +++++++++++++++++++
 arch/arm64/kernel/entry-common.c | 11 ++++++++++
 arch/arm64/kernel/mte.c          | 35 ++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index d02aff9f493d..1a715963d909 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -92,5 +92,26 @@ static inline void mte_assign_mem_tag_range(void *addr, size_t size)
 
 #endif /* CONFIG_ARM64_MTE */
 
+#ifdef CONFIG_KASAN_HW_TAGS
+void mte_check_tfsr_el1_no_sync(void);
+static inline void mte_check_tfsr_el1(void)
+{
+	mte_check_tfsr_el1_no_sync();
+	/*
+	 * The asynchronous faults are synch'ed automatically with
+	 * TFSR_EL1 on kernel entry but for exit an explicit dsb()
+	 * is required.
+	 */
+	dsb(ish);
+}
+#else
+static inline void mte_check_tfsr_el1_no_sync(void)
+{
+}
+static inline void mte_check_tfsr_el1(void)
+{
+}
+#endif /* CONFIG_KASAN_HW_TAGS */
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_MTE_H  */
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 5346953e4382..c6dfe8a525b0 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -37,6 +37,8 @@ static void noinstr enter_from_kernel_mode(struct pt_regs *regs)
 	lockdep_hardirqs_off(CALLER_ADDR0);
 	rcu_irq_enter_check_tick();
 	trace_hardirqs_off_finish();
+
+	mte_check_tfsr_el1_no_sync();
 }
 
 /*
@@ -47,6 +49,13 @@ static void noinstr exit_to_kernel_mode(struct pt_regs *regs)
 {
 	lockdep_assert_irqs_disabled();
 
+	/*
+	 * The dsb() in mte_check_tfsr_el1() is required to relate
+	 * the asynchronous tag check fault to the context in which
+	 * it happens.
+	 */
+	mte_check_tfsr_el1();
+
 	if (interrupts_enabled(regs)) {
 		if (regs->exit_rcu) {
 			trace_hardirqs_on_prepare();
@@ -243,6 +252,8 @@ asmlinkage void noinstr enter_from_user_mode(void)
 
 asmlinkage void noinstr exit_to_user_mode(void)
 {
+	mte_check_tfsr_el1();
+
 	trace_hardirqs_on_prepare();
 	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
 	user_enter_irqoff();
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index df7a1ae26d7c..6cb92e9d6ad1 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -180,6 +180,32 @@ void mte_enable_kernel(enum kasan_hw_tags_mode mode)
 	isb();
 }
 
+#ifdef CONFIG_KASAN_HW_TAGS
+void mte_check_tfsr_el1_no_sync(void)
+{
+	u64 tfsr_el1;
+
+	if (!system_supports_mte())
+		return;
+
+	tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
+
+	/*
+	 * The kernel should never hit the condition TF0 == 1
+	 * at this point because for the futex code we set
+	 * PSTATE.TCO.
+	 */
+	WARN_ON(tfsr_el1 & SYS_TFSR_EL1_TF0);
+
+	if (tfsr_el1 & SYS_TFSR_EL1_TF1) {
+		write_sysreg_s(0, SYS_TFSR_EL1);
+		isb();
+
+		pr_err("MTE: Asynchronous tag exception detected!");
+	}
+}
+#endif
+
 static void update_sctlr_el1_tcf0(u64 tcf0)
 {
 	/* ISB required for the kernel uaccess routines */
@@ -245,6 +271,15 @@ void mte_thread_switch(struct task_struct *next)
 	/* avoid expensive SCTLR_EL1 accesses if no change */
 	if (current->thread.sctlr_tcf0 != next->thread.sctlr_tcf0)
 		update_sctlr_el1_tcf0(next->thread.sctlr_tcf0);
+
+	/*
+	 * Check if an async tag exception occurred at EL1.
+	 *
+	 * Note: On the context switch path we rely on the dsb() present
+	 * in __switch_to() to guarantee that the indirect writes to TFSR_EL1
+	 * are synchronized before this point.
+	 */
+	mte_check_tfsr_el1_no_sync();
 }
 
 void mte_suspend_exit(void)
-- 
2.30.0

[PATCH v3 4/4] arm64: mte: Optimize mte_assign_mem_tag_range()

[Vincenzo Frascino]

mte_assign_mem_tag_range() is called on production KASAN HW hot
paths. It makes sense to optimize it in an attempt to reduce the
overhead.

Optimize mte_assign_mem_tag_range() based on the indications provided at
[1].

[1] https://lore.kernel.org/r/CAAeHK+wCO+J7D1_T89DG+jJrPLk3X9RsGFKxJGd0ZcUFjQT-9Q@mail.gmail.com/

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 arch/arm64/include/asm/mte.h | 26 +++++++++++++++++++++++++-
 arch/arm64/lib/mte.S         | 15 ---------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
index 1a715963d909..9730f2b07b79 100644
--- a/arch/arm64/include/asm/mte.h
+++ b/arch/arm64/include/asm/mte.h
@@ -49,7 +49,31 @@ long get_mte_ctrl(struct task_struct *task);
 int mte_ptrace_copy_tags(struct task_struct *child, long request,
 			 unsigned long addr, unsigned long data);
 
-void mte_assign_mem_tag_range(void *addr, size_t size);
+static inline void mte_assign_mem_tag_range(void *addr, size_t size)
+{
+	u64 _addr = (u64)addr;
+	u64 _end = _addr + size;
+
+	/*
+	 * This function must be invoked from an MTE enabled context.
+	 *
+	 * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
+	 * size must be non-zero and MTE_GRANULE_SIZE aligned.
+	 */
+	do {
+		/*
+		 * 'asm volatile' is required to prevent the compiler to move
+		 * the statement outside of the loop.
+		 */
+		asm volatile(__MTE_PREAMBLE "stg %0, [%0]"
+			     :
+			     : "r" (_addr)
+			     : "memory");
+
+		_addr += MTE_GRANULE_SIZE;
+	} while (_addr < _end);
+}
+
 
 #else /* CONFIG_ARM64_MTE */
 
diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S
index 9e1a12e10053..a0a650451510 100644
--- a/arch/arm64/lib/mte.S
+++ b/arch/arm64/lib/mte.S
@@ -150,18 +150,3 @@ SYM_FUNC_START(mte_restore_page_tags)
 	ret
 SYM_FUNC_END(mte_restore_page_tags)
 
-/*
- * Assign allocation tags for a region of memory based on the pointer tag
- *   x0 - source pointer
- *   x1 - size
- *
- * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
- * size must be non-zero and MTE_GRANULE_SIZE aligned.
- */
-SYM_FUNC_START(mte_assign_mem_tag_range)
-1:	stg	x0, [x0]
-	add	x0, x0, #MTE_GRANULE_SIZE
-	subs	x1, x1, #MTE_GRANULE_SIZE
-	b.gt	1b
-	ret
-SYM_FUNC_END(mte_assign_mem_tag_range)
-- 
2.30.0

Re: [PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Mark Rutland]

On Fri, Jan 15, 2021 at 12:00:40PM +0000, Vincenzo Frascino wrote:
> Architectures supported by KASAN HW can provide a light mode of
> execution. On an MTE enabled arm64 hw for example this can be identified
> with the asynch mode of execution.
> In this mode, if a tag check fault occurs, the TFSR_EL1 register is
> updated asynchronously. The kernel checks the corresponding bits
> periodically.

What's the expected usage of this relative to prod, given that this has
to be chosen at boot time? When/where is this expected to be used
relative to prod mode?

> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 18fce223b67b..3a7c5beb7096 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -231,7 +231,7 @@ static inline const void *__tag_set(const void *addr, u8 tag)
>  }
>  
>  #ifdef CONFIG_KASAN_HW_TAGS
> -#define arch_enable_tagging()			mte_enable_kernel()
> +#define arch_enable_tagging(mode)		mte_enable_kernel(mode)

Rather than passing a mode in, I think it'd be better to have:

* arch_enable_tagging_prod()
* arch_enable_tagging_light()

... that we can map in the arch code to separate:

* mte_enable_kernel_sync()
* mte_enable_kernel_async()

... as by construction that avoids calls with an unhandled mode, and we
wouldn't need the mode enum kasan_hw_tags_mode...

> +static inline int hw_init_mode(enum kasan_arg_mode mode)
> +{
> +	switch (mode) {
> +	case KASAN_ARG_MODE_LIGHT:
> +		return KASAN_HW_TAGS_ASYNC;
> +	default:
> +		return KASAN_HW_TAGS_SYNC;
> +	}
> +}

... and we can just have a wrapper like this to call either of the two functions directly, i.e.

static inline void hw_enable_tagging_mode(enum kasan_arg_mode mode)
{
	if (mode == KASAN_ARG_MODE_LIGHT)
		arch_enable_tagging_mode_light();
	else
		arch_enable_tagging_mode_prod();
}

Thanks,
Mark.

Re: [PATCH v3 2/4] arm64: mte: Add asynchronous mode support

[Mark Rutland]

On Fri, Jan 15, 2021 at 12:00:41PM +0000, Vincenzo Frascino wrote:
> MTE provides an asynchronous mode for detecting tag exceptions. In
> particular instead of triggering a fault the arm64 core updates a
> register which is checked by the kernel after the asynchronous tag
> check fault has occurred.
> 
> Add support for MTE asynchronous mode.
> 
> The exception handling mechanism will be added with a future patch.
> 
> Note: KASAN HW activates async mode via kasan.mode kernel parameter.
> The default mode is set to synchronous.
> The code that verifies the status of TFSR_EL1 will be added with a
> future patch.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/kernel/mte.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 53a6d734e29b..df7a1ae26d7c 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -153,8 +153,30 @@ void mte_init_tags(u64 max_tag)
>  
>  void mte_enable_kernel(enum kasan_hw_tags_mode mode)
>  {
> -	/* Enable MTE Sync Mode for EL1. */
> -	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> +	const char *m;
> +
> +	/* Preset parameter values based on the mode. */
> +	switch (mode) {
> +	case KASAN_HW_TAGS_ASYNC:
> +		/* Enable MTE Async Mode for EL1. */
> +		sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC);
> +		m = "asynchronous";
> +		break;
> +	case KASAN_HW_TAGS_SYNC:
> +		sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> +		m = "synchronous";
> +		break;
> +	default:
> +		/*
> +		 * kasan mode should be always set hence we should
> +		 * not reach this condition.
> +		 */
> +		WARN_ON_ONCE(1);
> +		return;
> +	}
> +
> +	pr_info_once("MTE: enabled in %s mode at EL1\n", m);
> +
>  	isb();
>  }

For clarity, we should have that ISB before the pr_info_once().

As with my comment on patch 1, I think with separate functions this
would be much clearer and simpler:

static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
{
	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, tcf);
	isb();

	pr_info_once("MTE: enabled in %s mode at EL1\n", mode);
}

void mte_enable_kernel_sync(void)
{
	__mte_enable_kernel("synchronous", SCTLR_ELx_TCF_SYNC);
}

void mte_enable_kernel_async(void)
{
	__mte_enable_kernel("asynchronous", SCTLR_ELx_TCF_ASYNC);
}

Thanks,
Mark.

Re: [PATCH v3 3/4] arm64: mte: Enable async tag check fault

[Mark Rutland]

On Fri, Jan 15, 2021 at 12:00:42PM +0000, Vincenzo Frascino wrote:
> MTE provides a mode that asynchronously updates the TFSR_EL1 register
> when a tag check exception is detected.
> 
> To take advantage of this mode the kernel has to verify the status of
> the register at:
>   1. Context switching
>   2. Return to user/EL0 (Not required in entry from EL0 since the kernel
>   did not run)
>   3. Kernel entry from EL1
>   4. Kernel exit to EL1
> 
> If the register is non-zero a trace is reported.
> 
> Add the required features for EL1 detection and reporting.
> 
> Note: ITFSB bit is set in the SCTLR_EL1 register hence it guaranties that
> the indirect writes to TFSR_EL1 are synchronized at exception entry to
> EL1. On the context switch path the synchronization is guarantied by the
> dsb() in __switch_to().
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/include/asm/mte.h     | 21 +++++++++++++++++++
>  arch/arm64/kernel/entry-common.c | 11 ++++++++++
>  arch/arm64/kernel/mte.c          | 35 ++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index d02aff9f493d..1a715963d909 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -92,5 +92,26 @@ static inline void mte_assign_mem_tag_range(void *addr, size_t size)
>  
>  #endif /* CONFIG_ARM64_MTE */
>  
> +#ifdef CONFIG_KASAN_HW_TAGS
> +void mte_check_tfsr_el1_no_sync(void);
> +static inline void mte_check_tfsr_el1(void)
> +{
> +	mte_check_tfsr_el1_no_sync();
> +	/*
> +	 * The asynchronous faults are synch'ed automatically with

Nit: can we please use "sync" rather than "synch", to match what we do
elsewhere, e.g. mte_check_tfsr_el1_no_sync immediately above. The
inconsistency is unfortunate and distracting.

> +	 * TFSR_EL1 on kernel entry but for exit an explicit dsb()
> +	 * is required.
> +	 */
> +	dsb(ish);
> +}

Did you mean to have the barrier /before/ checking the TFSR? I'm
confused as to why it's after the check if the point of it is to ensure
that TFSR has been updated.

I don't understand this difference between the entry/exit paths; are you
relying on a prior DSB in the entry path?

Is the DSB alone sufficient to update the TFSR (i.e. is an indirect
write ordered before a direct read)? ... or do you need a DSB + ISB
here?

It's probably worth a comment as to why the ISH domain is correct here
rather than NSH or SY. I'm not entirely certain if ISH is necessary or
sufficient, but it depends on the completion rules.

[...]

> >  
>  /*
> @@ -47,6 +49,13 @@ static void noinstr exit_to_kernel_mode(struct pt_regs *regs)
>  {
>  	lockdep_assert_irqs_disabled();
>  
> +	/*
> +	 * The dsb() in mte_check_tfsr_el1() is required to relate
> +	 * the asynchronous tag check fault to the context in which
> +	 * it happens.
> +	 */
> +	mte_check_tfsr_el1();

I think this comment is misplaced, given that mte_check_tfsr_el1() isn't
even in the same file.

If you need to do different things upon entry/exit, I'd rather we had
separate functions, e.g.

* mte_check_tfsr_entry();
* mte_check_tfsr_exit();

... since then it's immediately obvious in context as to whether we're
using the right function, and then we can have a comment within each of
the functions explaining what we need to do in that specific case.

>  	if (interrupts_enabled(regs)) {
>  		if (regs->exit_rcu) {
>  			trace_hardirqs_on_prepare();
> @@ -243,6 +252,8 @@ asmlinkage void noinstr enter_from_user_mode(void)
>  
>  asmlinkage void noinstr exit_to_user_mode(void)
>  {
> +	mte_check_tfsr_el1();
> +
>  	trace_hardirqs_on_prepare();
>  	lockdep_hardirqs_on_prepare(CALLER_ADDR0);
>  	user_enter_irqoff();
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index df7a1ae26d7c..6cb92e9d6ad1 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -180,6 +180,32 @@ void mte_enable_kernel(enum kasan_hw_tags_mode mode)
>  	isb();
>  }
>  
> +#ifdef CONFIG_KASAN_HW_TAGS
> +void mte_check_tfsr_el1_no_sync(void)
> +{
> +	u64 tfsr_el1;
> +
> +	if (!system_supports_mte())
> +		return;
> +
> +	tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
> +
> +	/*
> +	 * The kernel should never hit the condition TF0 == 1
> +	 * at this point because for the futex code we set
> +	 * PSTATE.TCO.
> +	 */

I thing it's worth spelling out what TF0 == 1 means, e.g.

	/*
	 * The kernel should never trigger an asynchronous fault on a
	 * TTBR0 address, so we should never see TF0 set.
	 * For futexes we disable checks via PSTATE.TCO.
	 */

... what about regular uaccess using LDTR/STTR? What happens for those?

> +	WARN_ON(tfsr_el1 & SYS_TFSR_EL1_TF0);

It's probably worth giving this a message so that we can debug it more
easily, e.g.

	WARN(tfsr_el1 & SYS_TFSR_EL1_TF0,
	     "Kernel async tag fault on TTBR0 address");

> +	if (tfsr_el1 & SYS_TFSR_EL1_TF1) {

It might be worth wrapping this with an unlikely(), given we hope this
never happens.

Thanks,
Mark.

> +		write_sysreg_s(0, SYS_TFSR_EL1);
> +		isb();
> +
> +		pr_err("MTE: Asynchronous tag exception detected!");
> +	}
> +}
> +#endif
> +
>  static void update_sctlr_el1_tcf0(u64 tcf0)
>  {
>  	/* ISB required for the kernel uaccess routines */
> @@ -245,6 +271,15 @@ void mte_thread_switch(struct task_struct *next)
>  	/* avoid expensive SCTLR_EL1 accesses if no change */
>  	if (current->thread.sctlr_tcf0 != next->thread.sctlr_tcf0)
>  		update_sctlr_el1_tcf0(next->thread.sctlr_tcf0);
> +
> +	/*
> +	 * Check if an async tag exception occurred at EL1.
> +	 *
> +	 * Note: On the context switch path we rely on the dsb() present
> +	 * in __switch_to() to guarantee that the indirect writes to TFSR_EL1
> +	 * are synchronized before this point.
> +	 */
> +	mte_check_tfsr_el1_no_sync();
>  }
>  
>  void mte_suspend_exit(void)
> -- 
> 2.30.0
> 

Re: [PATCH v3 4/4] arm64: mte: Optimize mte_assign_mem_tag_range()

[Mark Rutland]

On Fri, Jan 15, 2021 at 12:00:43PM +0000, Vincenzo Frascino wrote:
> mte_assign_mem_tag_range() is called on production KASAN HW hot
> paths. It makes sense to optimize it in an attempt to reduce the
> overhead.
> 
> Optimize mte_assign_mem_tag_range() based on the indications provided at
> [1].

... what exactly is the optimization?

I /think/ you're just trying to have it inlined, but you should mention
that explicitly.

> 
> [1] https://lore.kernel.org/r/CAAeHK+wCO+J7D1_T89DG+jJrPLk3X9RsGFKxJGd0ZcUFjQT-9Q@mail.gmail.com/
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/include/asm/mte.h | 26 +++++++++++++++++++++++++-
>  arch/arm64/lib/mte.S         | 15 ---------------
>  2 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index 1a715963d909..9730f2b07b79 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -49,7 +49,31 @@ long get_mte_ctrl(struct task_struct *task);
>  int mte_ptrace_copy_tags(struct task_struct *child, long request,
>  			 unsigned long addr, unsigned long data);
>  
> -void mte_assign_mem_tag_range(void *addr, size_t size);
> +static inline void mte_assign_mem_tag_range(void *addr, size_t size)
> +{
> +	u64 _addr = (u64)addr;
> +	u64 _end = _addr + size;
> +
> +	/*
> +	 * This function must be invoked from an MTE enabled context.
> +	 *
> +	 * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
> +	 * size must be non-zero and MTE_GRANULE_SIZE aligned.
> +	 */
> +	do {
> +		/*
> +		 * 'asm volatile' is required to prevent the compiler to move
> +		 * the statement outside of the loop.
> +		 */
> +		asm volatile(__MTE_PREAMBLE "stg %0, [%0]"
> +			     :
> +			     : "r" (_addr)
> +			     : "memory");
> +
> +		_addr += MTE_GRANULE_SIZE;
> +	} while (_addr < _end);

Is there any chance that this can be used for the last bytes of the
virtual address space? This might need to change to `_addr == _end` if
that is possible, otherwise it'll terminate early in that case.

> +}

What does the code generation look like for this, relative to the
assembly version?

Thanks,
Mark.

> +
>  
>  #else /* CONFIG_ARM64_MTE */
>  
> diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S
> index 9e1a12e10053..a0a650451510 100644
> --- a/arch/arm64/lib/mte.S
> +++ b/arch/arm64/lib/mte.S
> @@ -150,18 +150,3 @@ SYM_FUNC_START(mte_restore_page_tags)
>  	ret
>  SYM_FUNC_END(mte_restore_page_tags)
>  
> -/*
> - * Assign allocation tags for a region of memory based on the pointer tag
> - *   x0 - source pointer
> - *   x1 - size
> - *
> - * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
> - * size must be non-zero and MTE_GRANULE_SIZE aligned.
> - */
> -SYM_FUNC_START(mte_assign_mem_tag_range)
> -1:	stg	x0, [x0]
> -	add	x0, x0, #MTE_GRANULE_SIZE
> -	subs	x1, x1, #MTE_GRANULE_SIZE
> -	b.gt	1b
> -	ret
> -SYM_FUNC_END(mte_assign_mem_tag_range)
> -- 
> 2.30.0
> 

Re: [PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Andrey Konovalov]

On Fri, Jan 15, 2021 at 1:00 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> Architectures supported by KASAN HW can provide a light mode of
> execution. On an MTE enabled arm64 hw for example this can be identified
> with the asynch mode of execution.
> In this mode, if a tag check fault occurs, the TFSR_EL1 register is
> updated asynchronously. The kernel checks the corresponding bits
> periodically.
>
> KASAN requires a specific mode of execution to make use of this hw feature.
>
> Add KASAN HW light execution mode.
>
> Note: This patch adds the KASAN_ARG_MODE_LIGHT config option and the
> "light" kernel command line option to enable the described feature.
> This patch introduces the kasan_def.h header to make easier to propagate
> the relevant enumerations to the architectural code.
>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  arch/arm64/include/asm/memory.h    |  2 +-
>  arch/arm64/include/asm/mte-kasan.h |  5 +++--
>  arch/arm64/kernel/mte.c            |  2 +-
>  include/linux/kasan.h              |  1 +
>  include/linux/kasan_def.h          | 10 ++++++++++
>  mm/kasan/hw_tags.c                 | 19 ++++++++++++++++++-
>  mm/kasan/kasan.h                   |  2 +-
>  7 files changed, 35 insertions(+), 6 deletions(-)
>  create mode 100644 include/linux/kasan_def.h
>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 18fce223b67b..3a7c5beb7096 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -231,7 +231,7 @@ static inline const void *__tag_set(const void *addr, u8 tag)
>  }
>
>  #ifdef CONFIG_KASAN_HW_TAGS
> -#define arch_enable_tagging()                  mte_enable_kernel()
> +#define arch_enable_tagging(mode)              mte_enable_kernel(mode)
>  #define arch_init_tags(max_tag)                        mte_init_tags(max_tag)
>  #define arch_get_random_tag()                  mte_get_random_tag()
>  #define arch_get_mem_tag(addr)                 mte_get_mem_tag(addr)
> diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h
> index 26349a4b5e2e..5402f4c8e88d 100644
> --- a/arch/arm64/include/asm/mte-kasan.h
> +++ b/arch/arm64/include/asm/mte-kasan.h
> @@ -9,6 +9,7 @@
>
>  #ifndef __ASSEMBLY__
>
> +#include <linux/kasan_def.h>
>  #include <linux/types.h>
>
>  /*
> @@ -29,7 +30,7 @@ u8 mte_get_mem_tag(void *addr);
>  u8 mte_get_random_tag(void);
>  void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag);
>
> -void mte_enable_kernel(void);
> +void mte_enable_kernel(enum kasan_hw_tags_mode mode);
>  void mte_init_tags(u64 max_tag);
>
>  #else /* CONFIG_ARM64_MTE */
> @@ -52,7 +53,7 @@ static inline void *mte_set_mem_tag_range(void *addr, size_t size, u8 tag)
>         return addr;
>  }
>
> -static inline void mte_enable_kernel(void)
> +static inline void mte_enable_kernel(enum kasan_hw_tags_mode mode)
>  {
>  }
>
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index dc9ada64feed..53a6d734e29b 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -151,7 +151,7 @@ void mte_init_tags(u64 max_tag)
>         write_sysreg_s(SYS_GCR_EL1_RRND | gcr_kernel_excl, SYS_GCR_EL1);
>  }
>
> -void mte_enable_kernel(void)
> +void mte_enable_kernel(enum kasan_hw_tags_mode mode)
>  {
>         /* Enable MTE Sync Mode for EL1. */
>         sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 5e0655fb2a6f..026031444217 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -2,6 +2,7 @@
>  #ifndef _LINUX_KASAN_H
>  #define _LINUX_KASAN_H
>
> +#include <linux/kasan_def.h>
>  #include <linux/static_key.h>
>  #include <linux/types.h>
>
> diff --git a/include/linux/kasan_def.h b/include/linux/kasan_def.h
> new file mode 100644
> index 000000000000..0a55400809c9
> --- /dev/null
> +++ b/include/linux/kasan_def.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_KASAN_DEF_H
> +#define _LINUX_KASAN_DEF_H
> +
> +enum kasan_hw_tags_mode {
> +       KASAN_HW_TAGS_SYNC,
> +       KASAN_HW_TAGS_ASYNC,
> +};
> +
> +#endif /* _LINUX_KASAN_DEF_H */
> diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> index 55bd6f09c70f..6c3b0742f639 100644
> --- a/mm/kasan/hw_tags.c
> +++ b/mm/kasan/hw_tags.c
> @@ -22,6 +22,7 @@
>  enum kasan_arg_mode {
>         KASAN_ARG_MODE_DEFAULT,
>         KASAN_ARG_MODE_OFF,
> +       KASAN_ARG_MODE_LIGHT,
>         KASAN_ARG_MODE_PROD,
>         KASAN_ARG_MODE_FULL,
>  };
> @@ -60,6 +61,8 @@ static int __init early_kasan_mode(char *arg)
>
>         if (!strcmp(arg, "off"))
>                 kasan_arg_mode = KASAN_ARG_MODE_OFF;
> +       else if (!strcmp(arg, "light"))
> +               kasan_arg_mode = KASAN_ARG_MODE_LIGHT;

Hi Vincenzo,

I've just mailed the change to KASAN parameters [1] as discussed, so
we should use a standalone parameter here (kasan.trap?).

Thanks!

[1] https://lkml.org/lkml/2021/1/15/1242

Re: [PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Vincenzo Frascino]

Hi Andrey,

On 1/15/21 6:59 PM, Andrey Konovalov wrote:
> On Fri, Jan 15, 2021 at 1:00 PM Vincenzo Frascino
> <vincenzo.frascino@arm.com> wrote:
>>

[...]
>> @@ -60,6 +61,8 @@ static int __init early_kasan_mode(char *arg)
>>
>>         if (!strcmp(arg, "off"))
>>                 kasan_arg_mode = KASAN_ARG_MODE_OFF;
>> +       else if (!strcmp(arg, "light"))
>> +               kasan_arg_mode = KASAN_ARG_MODE_LIGHT;
> 
> Hi Vincenzo,
> 
> I've just mailed the change to KASAN parameters [1] as discussed, so
> we should use a standalone parameter here (kasan.trap?).
> 
> Thanks!
> 
> [1] https://lkml.org/lkml/2021/1/15/1242
> 

Thanks for this. I will have a look into it today. In the meantime, could you
please elaborate a bit more on kasan.trap?

-- 
Regards,
Vincenzo

Re: [PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Vincenzo Frascino]

Hi Mark,

On 1/15/21 3:08 PM, Mark Rutland wrote:
> On Fri, Jan 15, 2021 at 12:00:40PM +0000, Vincenzo Frascino wrote:
>> Architectures supported by KASAN HW can provide a light mode of
>> execution. On an MTE enabled arm64 hw for example this can be identified
>> with the asynch mode of execution.
>> In this mode, if a tag check fault occurs, the TFSR_EL1 register is
>> updated asynchronously. The kernel checks the corresponding bits
>> periodically.
> 
> What's the expected usage of this relative to prod, given that this has
> to be chosen at boot time? When/where is this expected to be used
> relative to prod mode?
> 

IIUC the light mode is meant for low spec devices. I let Andrey comment a bit
more on this topic.

>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>> index 18fce223b67b..3a7c5beb7096 100644
>> --- a/arch/arm64/include/asm/memory.h
>> +++ b/arch/arm64/include/asm/memory.h
>> @@ -231,7 +231,7 @@ static inline const void *__tag_set(const void *addr, u8 tag)
>>  }
>>  
>>  #ifdef CONFIG_KASAN_HW_TAGS
>> -#define arch_enable_tagging()			mte_enable_kernel()
>> +#define arch_enable_tagging(mode)		mte_enable_kernel(mode)
> 
> Rather than passing a mode in, I think it'd be better to have:
> 
> * arch_enable_tagging_prod()
> * arch_enable_tagging_light()
> 
> ... that we can map in the arch code to separate:
> 
> * mte_enable_kernel_sync()
> * mte_enable_kernel_async()
> 
> ... as by construction that avoids calls with an unhandled mode, and we
> wouldn't need the mode enum kasan_hw_tags_mode...
> 
>> +static inline int hw_init_mode(enum kasan_arg_mode mode)
>> +{
>> +	switch (mode) {
>> +	case KASAN_ARG_MODE_LIGHT:
>> +		return KASAN_HW_TAGS_ASYNC;
>> +	default:
>> +		return KASAN_HW_TAGS_SYNC;
>> +	}
>> +}
> 
> ... and we can just have a wrapper like this to call either of the two functions directly, i.e.
> 
> static inline void hw_enable_tagging_mode(enum kasan_arg_mode mode)
> {
> 	if (mode == KASAN_ARG_MODE_LIGHT)
> 		arch_enable_tagging_mode_light();
> 	else
> 		arch_enable_tagging_mode_prod();
> }
>

Fine by me, this would remove the need of adding a new enumeration as well and
reflect on the arch code. I would keep "arch_enable_tagging_mode_sync" and
"arch_enable_tagging_mode_async" though to give a clear indication in the KASAN
code of the mode we are setting. I will adapt my code accordingly for v4.

> Thanks,
> Mark.
> 

-- 
Regards,
Vincenzo

Re: [PATCH v3 2/4] arm64: mte: Add asynchronous mode support

[Vincenzo Frascino]

On 1/15/21 3:13 PM, Mark Rutland wrote:
> On Fri, Jan 15, 2021 at 12:00:41PM +0000, Vincenzo Frascino wrote:
>> MTE provides an asynchronous mode for detecting tag exceptions. In
>> particular instead of triggering a fault the arm64 core updates a
>> register which is checked by the kernel after the asynchronous tag
>> check fault has occurred.
>>
>> Add support for MTE asynchronous mode.
>>
>> The exception handling mechanism will be added with a future patch.
>>
>> Note: KASAN HW activates async mode via kasan.mode kernel parameter.
>> The default mode is set to synchronous.
>> The code that verifies the status of TFSR_EL1 will be added with a
>> future patch.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
>> ---
>>  arch/arm64/kernel/mte.c | 26 ++++++++++++++++++++++++--
>>  1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
>> index 53a6d734e29b..df7a1ae26d7c 100644
>> --- a/arch/arm64/kernel/mte.c
>> +++ b/arch/arm64/kernel/mte.c
>> @@ -153,8 +153,30 @@ void mte_init_tags(u64 max_tag)
>>  
>>  void mte_enable_kernel(enum kasan_hw_tags_mode mode)
>>  {
>> -	/* Enable MTE Sync Mode for EL1. */
>> -	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
>> +	const char *m;
>> +
>> +	/* Preset parameter values based on the mode. */
>> +	switch (mode) {
>> +	case KASAN_HW_TAGS_ASYNC:
>> +		/* Enable MTE Async Mode for EL1. */
>> +		sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC);
>> +		m = "asynchronous";
>> +		break;
>> +	case KASAN_HW_TAGS_SYNC:
>> +		sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
>> +		m = "synchronous";
>> +		break;
>> +	default:
>> +		/*
>> +		 * kasan mode should be always set hence we should
>> +		 * not reach this condition.
>> +		 */
>> +		WARN_ON_ONCE(1);
>> +		return;
>> +	}
>> +
>> +	pr_info_once("MTE: enabled in %s mode at EL1\n", m);
>> +
>>  	isb();
>>  }
> 
> For clarity, we should have that ISB before the pr_info_once().
>

Good point, I will fix it in v4.

> As with my comment on patch 1, I think with separate functions this
> would be much clearer and simpler:
> 
> static inline void __mte_enable_kernel(const char *mode, unsigned long tcf)
> {
> 	sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, tcf);
> 	isb();
> 
> 	pr_info_once("MTE: enabled in %s mode at EL1\n", mode);
> }
> 
> void mte_enable_kernel_sync(void)
> {
> 	__mte_enable_kernel("synchronous", SCTLR_ELx_TCF_SYNC);
> }
> 
> void mte_enable_kernel_async(void)
> {
> 	__mte_enable_kernel("asynchronous", SCTLR_ELx_TCF_ASYNC);
> }
> 

Ok, seems cleaner like this, will adapt my code accordingly.

> Thanks,
> Mark.
> 

-- 
Regards,
Vincenzo

Re: [PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Andrey Konovalov]

On Sat, Jan 16, 2021 at 2:37 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> > [1] https://lkml.org/lkml/2021/1/15/1242
> >
>
> Thanks for this. I will have a look into it today. In the meantime, could you
> please elaborate a bit more on kasan.trap?

That's what I call the boot parameter that allows switching between
sync and async. We'll need one as we're dropping
kasan.mode=off/prod/light/full.

Feel free to name it differently. Perhaps, as kasan.mode is now
unused, we can use that for sync/async.

Re: [PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Vincenzo Frascino]

On 1/16/21 1:59 PM, Andrey Konovalov wrote:
> On Sat, Jan 16, 2021 at 2:37 PM Vincenzo Frascino
> <vincenzo.frascino@arm.com> wrote:
>>
>>> [1] https://lkml.org/lkml/2021/1/15/1242
>>>
>>
>> Thanks for this. I will have a look into it today. In the meantime, could you
>> please elaborate a bit more on kasan.trap?
> 
> That's what I call the boot parameter that allows switching between
> sync and async. We'll need one as we're dropping
> kasan.mode=off/prod/light/full.
> 
> Feel free to name it differently. Perhaps, as kasan.mode is now
> unused, we can use that for sync/async.
> 

I see, thanks for the explanation. "mode" or "trap" would work for me.

-- 
Regards,
Vincenzo

Re: [PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Andrey Konovalov]

On Sat, Jan 16, 2021 at 2:43 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
> On 1/15/21 3:08 PM, Mark Rutland wrote:
> > On Fri, Jan 15, 2021 at 12:00:40PM +0000, Vincenzo Frascino wrote:
> >> Architectures supported by KASAN HW can provide a light mode of
> >> execution. On an MTE enabled arm64 hw for example this can be identified
> >> with the asynch mode of execution.
> >> In this mode, if a tag check fault occurs, the TFSR_EL1 register is
> >> updated asynchronously. The kernel checks the corresponding bits
> >> periodically.
> >
> > What's the expected usage of this relative to prod, given that this has
> > to be chosen at boot time? When/where is this expected to be used
> > relative to prod mode?

Hi Mark,

Sync + no panic (what is called prod right now) + logging is for the
initial MTE integration stage as causing panics is risky. There's no
way to know how often MTE-detected bugs will happen during normal
usage as the kernel is buggy.

Eventually, we're hoping to switch to sync + panic to allow MTE to act
as a security mitigation. For devices where the slowdown caused by
sync is untolerable, there'll be an option to use async, which is
significantly faster. The exact perf numbers are yet to be measured
properly, I'll share them with one of the future patches.

Thanks!

Re: [PATCH v3 4/4] arm64: mte: Optimize mte_assign_mem_tag_range()

[Vincenzo Frascino]

Hi Mark,

On 1/15/21 3:45 PM, Mark Rutland wrote:
> On Fri, Jan 15, 2021 at 12:00:43PM +0000, Vincenzo Frascino wrote:
>> mte_assign_mem_tag_range() is called on production KASAN HW hot
>> paths. It makes sense to optimize it in an attempt to reduce the
>> overhead.
>>
>> Optimize mte_assign_mem_tag_range() based on the indications provided at
>> [1].
> 
> ... what exactly is the optimization?
> 
> I /think/ you're just trying to have it inlined, but you should mention
> that explicitly.
> 

Good point, I will change it in the next version. I used "Optimize" as a
continuation of the topic in the previous thread but you are right it is not
immediately obvious.

>>
>> [1] https://lore.kernel.org/r/CAAeHK+wCO+J7D1_T89DG+jJrPLk3X9RsGFKxJGd0ZcUFjQT-9Q@mail.gmail.com/
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
>> ---
>>  arch/arm64/include/asm/mte.h | 26 +++++++++++++++++++++++++-
>>  arch/arm64/lib/mte.S         | 15 ---------------
>>  2 files changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
>> index 1a715963d909..9730f2b07b79 100644
>> --- a/arch/arm64/include/asm/mte.h
>> +++ b/arch/arm64/include/asm/mte.h
>> @@ -49,7 +49,31 @@ long get_mte_ctrl(struct task_struct *task);
>>  int mte_ptrace_copy_tags(struct task_struct *child, long request,
>>  			 unsigned long addr, unsigned long data);
>>  
>> -void mte_assign_mem_tag_range(void *addr, size_t size);
>> +static inline void mte_assign_mem_tag_range(void *addr, size_t size)
>> +{
>> +	u64 _addr = (u64)addr;
>> +	u64 _end = _addr + size;
>> +
>> +	/*
>> +	 * This function must be invoked from an MTE enabled context.
>> +	 *
>> +	 * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
>> +	 * size must be non-zero and MTE_GRANULE_SIZE aligned.
>> +	 */
>> +	do {
>> +		/*
>> +		 * 'asm volatile' is required to prevent the compiler to move
>> +		 * the statement outside of the loop.
>> +		 */
>> +		asm volatile(__MTE_PREAMBLE "stg %0, [%0]"
>> +			     :
>> +			     : "r" (_addr)
>> +			     : "memory");
>> +
>> +		_addr += MTE_GRANULE_SIZE;
>> +	} while (_addr < _end);
> 
> Is there any chance that this can be used for the last bytes of the
> virtual address space? This might need to change to `_addr == _end` if
> that is possible, otherwise it'll terminate early in that case.
> 

Theoretically it is a possibility. I will change the condition and add a note
for that.

>> +}
> 
> What does the code generation look like for this, relative to the
> assembly version?
> 

The assembly looks like this:

 390:   8b000022        add     x2, x1, x0
 394:   aa0003e1        mov     x1, x0
 398:   d9200821        stg     x1, [x1]
 39c:   91004021        add     x1, x1, #0x10
 3a0:   eb01005f        cmp     x2, x1
 3a4:   54ffffa8        b.hi    398 <mte_set_mem_tag_range+0x48>

You can see the handcrafted one below.

> Thanks,
> Mark.
> 
>> +
>>  
>>  #else /* CONFIG_ARM64_MTE */
>>  
>> diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S
>> index 9e1a12e10053..a0a650451510 100644
>> --- a/arch/arm64/lib/mte.S
>> +++ b/arch/arm64/lib/mte.S
>> @@ -150,18 +150,3 @@ SYM_FUNC_START(mte_restore_page_tags)
>>  	ret
>>  SYM_FUNC_END(mte_restore_page_tags)
>>  
>> -/*
>> - * Assign allocation tags for a region of memory based on the pointer tag
>> - *   x0 - source pointer
>> - *   x1 - size
>> - *
>> - * Note: The address must be non-NULL and MTE_GRANULE_SIZE aligned and
>> - * size must be non-zero and MTE_GRANULE_SIZE aligned.
>> - */
>> -SYM_FUNC_START(mte_assign_mem_tag_range)
>> -1:	stg	x0, [x0]
>> -	add	x0, x0, #MTE_GRANULE_SIZE
>> -	subs	x1, x1, #MTE_GRANULE_SIZE
>> -	b.gt	1b
>> -	ret
>> -SYM_FUNC_END(mte_assign_mem_tag_range)
>> -- 
>> 2.30.0
>>

-- 
Regards,
Vincenzo

Re: [PATCH v3 4/4] arm64: mte: Optimize mte_assign_mem_tag_range()

[Vincenzo Frascino]

Hi Mark,

On 1/16/21 2:22 PM, Vincenzo Frascino wrote:
>> Is there any chance that this can be used for the last bytes of the
>> virtual address space? This might need to change to `_addr == _end` if
>> that is possible, otherwise it'll terminate early in that case.
>>
> Theoretically it is a possibility. I will change the condition and add a note
> for that.
> 

I was thinking to the end of the virtual address space scenario and I forgot
that if I use a condition like `_addr == _end` the tagging operation overflows
to the first granule of the next allocation. This disrupts tagging accesses for
that memory area hence I think that `_addr < _end` is the way to go.

-- 
Regards,
Vincenzo

Re: [PATCH v3 1/4] kasan, arm64: Add KASAN light mode

[Mark Rutland]

On Sat, Jan 16, 2021 at 01:47:08PM +0000, Vincenzo Frascino wrote:
> On 1/15/21 3:08 PM, Mark Rutland wrote:
> > On Fri, Jan 15, 2021 at 12:00:40PM +0000, Vincenzo Frascino wrote:
> >>  #ifdef CONFIG_KASAN_HW_TAGS
> >> -#define arch_enable_tagging()			mte_enable_kernel()
> >> +#define arch_enable_tagging(mode)		mte_enable_kernel(mode)
> > 
> > Rather than passing a mode in, I think it'd be better to have:
> > 
> > * arch_enable_tagging_prod()
> > * arch_enable_tagging_light()
> > 
> > ... that we can map in the arch code to separate:
> > 
> > * mte_enable_kernel_sync()
> > * mte_enable_kernel_async()
> > 
> > ... as by construction that avoids calls with an unhandled mode, and we
> > wouldn't need the mode enum kasan_hw_tags_mode...
> > 
> >> +static inline int hw_init_mode(enum kasan_arg_mode mode)
> >> +{
> >> +	switch (mode) {
> >> +	case KASAN_ARG_MODE_LIGHT:
> >> +		return KASAN_HW_TAGS_ASYNC;
> >> +	default:
> >> +		return KASAN_HW_TAGS_SYNC;
> >> +	}
> >> +}
> > 
> > ... and we can just have a wrapper like this to call either of the two functions directly, i.e.
> > 
> > static inline void hw_enable_tagging_mode(enum kasan_arg_mode mode)
> > {
> > 	if (mode == KASAN_ARG_MODE_LIGHT)
> > 		arch_enable_tagging_mode_light();
> > 	else
> > 		arch_enable_tagging_mode_prod();
> > }
> >
> 
> Fine by me, this would remove the need of adding a new enumeration as well and
> reflect on the arch code. I would keep "arch_enable_tagging_mode_sync" and
> "arch_enable_tagging_mode_async" though to give a clear indication in the KASAN
> code of the mode we are setting. I will adapt my code accordingly for v4.

Thanks, that sounds great!

I completely agree on keeping the '_sync' and '_aync' suffixes in the
the core code.

Mark.

Re: [PATCH v3 4/4] arm64: mte: Optimize mte_assign_mem_tag_range()

[Mark Rutland]

On Sun, Jan 17, 2021 at 12:27:08PM +0000, Vincenzo Frascino wrote:
> Hi Mark,
> 
> On 1/16/21 2:22 PM, Vincenzo Frascino wrote:
> >> Is there any chance that this can be used for the last bytes of the
> >> virtual address space? This might need to change to `_addr == _end` if
> >> that is possible, otherwise it'll terminate early in that case.
> >>
> > Theoretically it is a possibility. I will change the condition and add a note
> > for that.
> > 
> 
> I was thinking to the end of the virtual address space scenario and I forgot
> that if I use a condition like `_addr == _end` the tagging operation overflows
> to the first granule of the next allocation. This disrupts tagging accesses for
> that memory area hence I think that `_addr < _end` is the way to go.

I think it implies `_addr != _end` is necessary. Otherwise, if `addr` is
PAGE_SIZE from the end of memory, and `size` is PAGE_SIZE, `_end` will
be 0, so using `_addr < _end` will mean the loop will terminate after a
single MTE tag granule rather than the whole page.

Generally, for some addr/increment/size combination (where all are
suitably aligned), you need a pattern like:

| do {
|       thing(addr);
|       addr += increment;
| } while (addr != end);

... or:

| for (addr = start; addr != end; addr += increment) {
|       thing(addr);
| }

... to correctly handle working at the very end of the VA space.

We do similar for page tables, e.g. when we use pmd_addr_end().

Thanks,
Mark.

Re: [PATCH v3 4/4] arm64: mte: Optimize mte_assign_mem_tag_range()

[Vincenzo Frascino]

On 1/18/21 10:41 AM, Mark Rutland wrote:
> On Sun, Jan 17, 2021 at 12:27:08PM +0000, Vincenzo Frascino wrote:
>> Hi Mark,
>>
>> On 1/16/21 2:22 PM, Vincenzo Frascino wrote:
>>>> Is there any chance that this can be used for the last bytes of the
>>>> virtual address space? This might need to change to `_addr == _end` if
>>>> that is possible, otherwise it'll terminate early in that case.
>>>>
>>> Theoretically it is a possibility. I will change the condition and add a note
>>> for that.
>>>
>>
>> I was thinking to the end of the virtual address space scenario and I forgot
>> that if I use a condition like `_addr == _end` the tagging operation overflows
>> to the first granule of the next allocation. This disrupts tagging accesses for
>> that memory area hence I think that `_addr < _end` is the way to go.
> 
> I think it implies `_addr != _end` is necessary. Otherwise, if `addr` is
> PAGE_SIZE from the end of memory, and `size` is PAGE_SIZE, `_end` will
> be 0, so using `_addr < _end` will mean the loop will terminate after a
> single MTE tag granule rather than the whole page.
> 
> Generally, for some addr/increment/size combination (where all are
> suitably aligned), you need a pattern like:
> 
> | do {
> |       thing(addr);
> |       addr += increment;
> | } while (addr != end);
> 
> ... or:
> 
> | for (addr = start; addr != end; addr += increment) {
> |       thing(addr);
> | }
> 
> ... to correctly handle working at the very end of the VA space.
> 
> We do similar for page tables, e.g. when we use pmd_addr_end().
>

Good point! I agree it wraps around otherwise. I will change it accordingly.

Thanks!

> Thanks,
> Mark.
> 

-- 
Regards,
Vincenzo

Re: [PATCH v3 3/4] arm64: mte: Enable async tag check fault

[Catalin Marinas]

On Fri, Jan 15, 2021 at 12:00:42PM +0000, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h
> index d02aff9f493d..1a715963d909 100644
> --- a/arch/arm64/include/asm/mte.h
> +++ b/arch/arm64/include/asm/mte.h
> @@ -92,5 +92,26 @@ static inline void mte_assign_mem_tag_range(void *addr, size_t size)
>  
>  #endif /* CONFIG_ARM64_MTE */
>  
> +#ifdef CONFIG_KASAN_HW_TAGS
> +void mte_check_tfsr_el1_no_sync(void);
> +static inline void mte_check_tfsr_el1(void)
> +{
> +	mte_check_tfsr_el1_no_sync();
> +	/*
> +	 * The asynchronous faults are synch'ed automatically with
> +	 * TFSR_EL1 on kernel entry but for exit an explicit dsb()
> +	 * is required.
> +	 */
> +	dsb(ish);
> +}

Mark commented already, the barrier should be above
mte_check_tfsr_el1_no_sync(). Regarding the ISB, we are waiting for
confirmation from the architects.

> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index df7a1ae26d7c..6cb92e9d6ad1 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -180,6 +180,32 @@ void mte_enable_kernel(enum kasan_hw_tags_mode mode)
>  	isb();
>  }
>  
> +#ifdef CONFIG_KASAN_HW_TAGS
> +void mte_check_tfsr_el1_no_sync(void)
> +{
> +	u64 tfsr_el1;
> +
> +	if (!system_supports_mte())
> +		return;
> +
> +	tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
> +
> +	/*
> +	 * The kernel should never hit the condition TF0 == 1
> +	 * at this point because for the futex code we set
> +	 * PSTATE.TCO.
> +	 */
> +	WARN_ON(tfsr_el1 & SYS_TFSR_EL1_TF0);

I'd change this to a WARN_ON_ONCE() in case we trip over this due to
model bugs etc. and it floods the log.

> +	if (tfsr_el1 & SYS_TFSR_EL1_TF1) {
> +		write_sysreg_s(0, SYS_TFSR_EL1);
> +		isb();

While in general we use ISB after a sysreg update, I haven't convinced
myself it's needed here. There's no side-effect to updating this reg and
a subsequent TFSR access should see the new value. If a speculated load
is allowed to update this reg, we'd probably need an ISB+DSB (I don't
think it does, something to check with the architects).

> +
> +		pr_err("MTE: Asynchronous tag exception detected!");

We discussed this already, I think we should replace this pr_err() with
a call to kasan_report(). In principle, kasan already knows the mode as
it asked for sync/async but we could make this explicit and expand the
kasan API to take some argument (or have separate function like
kasan_report_async()).

-- 
Catalin

Re: [PATCH v3 3/4] arm64: mte: Enable async tag check fault

[Vincenzo Frascino]

On 1/18/21 12:57 PM, Catalin Marinas wrote:
>> +#ifdef CONFIG_KASAN_HW_TAGS
>> +void mte_check_tfsr_el1_no_sync(void)
>> +{
>> +	u64 tfsr_el1;
>> +
>> +	if (!system_supports_mte())
>> +		return;
>> +
>> +	tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1);
>> +
>> +	/*
>> +	 * The kernel should never hit the condition TF0 == 1
>> +	 * at this point because for the futex code we set
>> +	 * PSTATE.TCO.
>> +	 */
>> +	WARN_ON(tfsr_el1 & SYS_TFSR_EL1_TF0);
> I'd change this to a WARN_ON_ONCE() in case we trip over this due to
> model bugs etc. and it floods the log.
> 

I will merge yours and Mark's comment using WARN_ONCE() here. Did not think of
potential bug in the model and you are completely right.

>> +	if (tfsr_el1 & SYS_TFSR_EL1_TF1) {
>> +		write_sysreg_s(0, SYS_TFSR_EL1);
>> +		isb();
> While in general we use ISB after a sysreg update, I haven't convinced
> myself it's needed here. There's no side-effect to updating this reg and
> a subsequent TFSR access should see the new value.

Why there is no side-effect?

> If a speculated load is allowed to update this reg, we'd probably need an
> ISB+DSB (I don't think it does, something to check with the architects).
> 

I will check this with the architects and let you know.

-- 
Regards,
Vincenzo

Re: [PATCH v3 3/4] arm64: mte: Enable async tag check fault

[Mark Rutland]

On Mon, Jan 18, 2021 at 01:37:35PM +0000, Vincenzo Frascino wrote:
> On 1/18/21 12:57 PM, Catalin Marinas wrote:

> >> +	if (tfsr_el1 & SYS_TFSR_EL1_TF1) {
> >> +		write_sysreg_s(0, SYS_TFSR_EL1);
> >> +		isb();
> > While in general we use ISB after a sysreg update, I haven't convinced
> > myself it's needed here. There's no side-effect to updating this reg and
> > a subsequent TFSR access should see the new value.
> 
> Why there is no side-effect?

Catalin's saying that the value of TFSR_EL1 doesn't affect anything
other than a read of TFSR_EL1, i.e. there are no indirect reads of
TFSR_EL1 where the value has an effect, so there are no side-effects.

Looking at the ARM ARM, no synchronization is requires from a direct
write to an indirect write (per ARM DDI 0487F.c table D13-1), so I agree
that we don't need the ISB here so long as there are no indirect reads.

Are you aware of cases where the TFSR_EL1 value is read other than by an
MRS? e.g. are there any cases where checks are elided if TF1 is set? If
so, we may need the ISB to order the direct write against subsequent
indirect reads.

Thanks,
Mark.

Re: [PATCH v3 3/4] arm64: mte: Enable async tag check fault

[Vincenzo Frascino]

Hi Mark,

On 1/18/21 2:14 PM, Mark Rutland wrote:
> On Mon, Jan 18, 2021 at 01:37:35PM +0000, Vincenzo Frascino wrote:
>> On 1/18/21 12:57 PM, Catalin Marinas wrote:
> 
>>>> +	if (tfsr_el1 & SYS_TFSR_EL1_TF1) {
>>>> +		write_sysreg_s(0, SYS_TFSR_EL1);
>>>> +		isb();
>>> While in general we use ISB after a sysreg update, I haven't convinced
>>> myself it's needed here. There's no side-effect to updating this reg and
>>> a subsequent TFSR access should see the new value.
>>
>> Why there is no side-effect?
> 
> Catalin's saying that the value of TFSR_EL1 doesn't affect anything
> other than a read of TFSR_EL1, i.e. there are no indirect reads of
> TFSR_EL1 where the value has an effect, so there are no side-effects.
> 
> Looking at the ARM ARM, no synchronization is requires from a direct
> write to an indirect write (per ARM DDI 0487F.c table D13-1), so I agree
> that we don't need the ISB here so long as there are no indirect reads.
> 
> Are you aware of cases where the TFSR_EL1 value is read other than by an
> MRS? e.g. are there any cases where checks are elided if TF1 is set? If
> so, we may need the ISB to order the direct write against subsequent
> indirect reads.
> 

Thank you for the explanation. I am not aware of any case in which TFSR_EL1 is
read other then by an MRS. Based on the ARM DDI 0487F.c (J1-7626) TF0/TF1 are
always set to '1' without being accessed before. I will check with the
architects for further clarification and if this is correct I will remove the
isb() in the next version.

> Thanks,
> Mark.
> 

-- 
Regards,
Vincenzo

Re: [PATCH v3 3/4] arm64: mte: Enable async tag check fault

[Vincenzo Frascino]

On 1/18/21 2:48 PM, Vincenzo Frascino wrote:
>> Are you aware of cases where the TFSR_EL1 value is read other than by an
>> MRS? e.g. are there any cases where checks are elided if TF1 is set? If
>> so, we may need the ISB to order the direct write against subsequent
>> indirect reads.
>>
> Thank you for the explanation. I am not aware of any case in which TFSR_EL1 is
> read other then by an MRS. Based on the ARM DDI 0487F.c (J1-7626) TF0/TF1 are
> always set to '1' without being accessed before. I will check with the
> architects for further clarification and if this is correct I will remove the
> isb() in the next version.
> 

I spoke to the architects and I confirm that the isb() can be removed.

-- 
Regards,
Vincenzo

Re: [PATCH v3 3/4] arm64: mte: Enable async tag check fault

[Vincenzo Frascino]

On 1/18/21 1:37 PM, Vincenzo Frascino wrote:
>> If a speculated load is allowed to update this reg, we'd probably need an
>> ISB+DSB (I don't think it does, something to check with the architects).
>>
> I will check this with the architects and let you know.

I spoke to the architects and no speculative load can update TFSR_EL1.

-- 
Regards,
Vincenzo