| Message ID | 20210115120043.50023-4-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ARMv8.5-A: MTE: Add async mode support | expand |
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 >
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()).
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.
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.
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. >
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.
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.
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)
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(+)