| Message ID | 20210118183033.41764-5-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ARMv8.5-A: MTE: Add async mode support | expand |
On Mon, Jan 18, 2021 at 06:30:32PM +0000, Vincenzo Frascino wrote: > static void update_sctlr_el1_tcf0(u64 tcf0) > { > /* ISB required for the kernel uaccess routines */ > @@ -235,6 +273,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(); > } We need an isb() before mte_check_tfsr_el1() here as well, we only have a dsb() in __switch_to(). We do have an isb() in update_sctlr_el1_tcf0() but only if the check passed. Now, it's worth benchmarking how expensive update_sctlr_el1_tcf0() is (i.e. an SCTLR_EL1 access + isb with something like hackbench) and we could probably remove the check altogether. In the meantime, you can add an isb() on the "else" path of the above check.
On 1/19/21 2:34 PM, Catalin Marinas wrote: > On Mon, Jan 18, 2021 at 06:30:32PM +0000, Vincenzo Frascino wrote: >> static void update_sctlr_el1_tcf0(u64 tcf0) >> { >> /* ISB required for the kernel uaccess routines */ >> @@ -235,6 +273,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(); >> } > > We need an isb() before mte_check_tfsr_el1() here as well, we only have > a dsb() in __switch_to(). We do have an isb() in update_sctlr_el1_tcf0() > but only if the check passed. Now, it's worth benchmarking how expensive > update_sctlr_el1_tcf0() is (i.e. an SCTLR_EL1 access + isb with > something like hackbench) and we could probably remove the check > altogether. In the meantime, you can add an isb() on the "else" path of > the above check. > Good catch, I saw the isb() in update_sctlr_el1_tcf0() and for some reasons that it is not escaping me I thought it was sufficient, but clearly it is not. I am happy to benchmark what you are suggesting and provide some data after this series is merged (if it works for you) so that we can decide. In the meantime as you suggested I will fix the "else" for v5.
diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h index d02aff9f493d..237bb2f7309d 100644 --- a/arch/arm64/include/asm/mte.h +++ b/arch/arm64/include/asm/mte.h @@ -92,5 +92,37 @@ 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(void); + +static inline void mte_check_tfsr_entry(void) +{ + mte_check_tfsr_el1(); +} + +static inline void mte_check_tfsr_exit(void) +{ + /* + * The asynchronous faults are sync'ed automatically with + * TFSR_EL1 on kernel entry but for exit an explicit dsb() + * is required. + */ + dsb(nsh); + isb(); + + mte_check_tfsr_el1(); +} +#else +static inline void mte_check_tfsr_el1(void) +{ +} +static inline void mte_check_tfsr_entry(void) +{ +} +static inline void mte_check_tfsr_exit(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..31666511ba67 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_entry(); } /* @@ -47,6 +49,8 @@ static void noinstr exit_to_kernel_mode(struct pt_regs *regs) { lockdep_assert_irqs_disabled(); + mte_check_tfsr_exit(); + if (interrupts_enabled(regs)) { if (regs->exit_rcu) { trace_hardirqs_on_prepare(); @@ -243,6 +247,8 @@ asmlinkage void noinstr enter_from_user_mode(void) asmlinkage void noinstr exit_to_user_mode(void) { + mte_check_tfsr_exit(); + 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 78fc079a3b1e..0a9cc82a5301 100644 --- a/arch/arm64/kernel/mte.c +++ b/arch/arm64/kernel/mte.c @@ -170,6 +170,44 @@ void mte_enable_kernel_async(void) __mte_enable_kernel("asynchronous", SCTLR_ELx_TCF_ASYNC); } +#ifdef CONFIG_KASAN_HW_TAGS +static inline void mte_report_async(void) +{ + u64 pc = (u64)__builtin_return_address(0); + + kasan_report_async(0, 0, false, pc); +} + +void mte_check_tfsr_el1(void) +{ + u64 tfsr_el1; + + if (!system_supports_mte()) + return; + + tfsr_el1 = read_sysreg_s(SYS_TFSR_EL1); + + /* + * 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. + */ + WARN_ONCE(tfsr_el1 & SYS_TFSR_EL1_TF0, + "Kernel async tag fault on TTBR0 address"); + + if (unlikely(tfsr_el1 & SYS_TFSR_EL1_TF1)) { + /* + * Note: isb() is not required after this direct write + * because there is no indirect read subsequent to it + * (per ARM DDI 0487F.c table D13-1). + */ + write_sysreg_s(0, SYS_TFSR_EL1); + + mte_report_async(); + } +} +#endif + static void update_sctlr_el1_tcf0(u64 tcf0) { /* ISB required for the kernel uaccess routines */ @@ -235,6 +273,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(); } void mte_suspend_exit(void)
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(). The dsb(nsh) in mte_check_tfsr_exit() is provisional pending confirmation by the architects. 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 | 32 ++++++++++++++++++++++ arch/arm64/kernel/entry-common.c | 6 ++++ arch/arm64/kernel/mte.c | 47 ++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+)