[PATCH] arm64: mte: avoid clearing PSTATE.TCO on entry unless necessary

[Peter Collingbourne]

On Tue, Oct 5, 2021 at 9:46 AM Catalin Marinas <catalin.marinas at arm.com> wrote:
>
> On Wed, Sep 29, 2021 at 12:45:24PM -0700, Peter Collingbourne wrote:
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index 2f69ae43941d..85ead6bbb38e 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -269,7 +269,28 @@ alternative_else_nop_endif
> >       .else
> >       add     x21, sp, #PT_REGS_SIZE
> >       get_current_task tsk
> > +     ldr     x0, [tsk, THREAD_SCTLR_USER]
> >       .endif /* \el == 0 */
> > +
> > +     /*
> > +      * Re-enable tag checking (TCO set on exception entry). This is only
> > +      * necessary if MTE is enabled in either the kernel or the userspace
> > +      * task in synchronous mode. With MTE disabled in the kernel and
> > +      * disabled or asynchronous in userspace, tag check faults (including in
> > +      * uaccesses) are not reported, therefore there is no need to re-enable
> > +      * checking. This is beneficial on microarchitectures where re-enabling
> > +      * TCO is expensive.
> > +      */
> > +#ifdef CONFIG_ARM64_MTE
> > +alternative_cb       kasan_hw_tags_enable
> > +     tbz     x0, #SCTLR_EL1_TCF0_SHIFT, 1f
> > +alternative_cb_end
> > +alternative_if ARM64_MTE
> > +     SET_PSTATE_TCO(0)
> > +alternative_else_nop_endif
> > +1:
> > +#endif
>
> I think we can get here from an interrupt as well. Can we guarantee that
> the sctlr_user is valid? We are not always in a user process context.

Looking through the code in entry.S carefully it doesn't appear that
the tsk pointer is ever used when taking an exception from EL1. The
last user appears to have been removed in commit 3d2403fd10a1 ("arm64:
uaccess: remove set_fs()"). I just did a quick boot test on a couple
of platforms and things seem to work without the "get_current_task
tsk" line.

So I can't be confident that tsk points to a valid task, but at least
it looks like it was the case prior to that commit.

> Maybe only do the above checks if \el == 0, otherwise just bracket it
> with kasan_hw_tags_enable.

Is it possible for us to do a uaccess inside the EL1 -> EL1 exception
handler? That was the one thing I was unsure about and it's why I
opted to do the same check regardless of EL.

If we don't think these uaccesses can happen, or we don't think it
matters that they will either fail or not depending on whether KASAN
is enabled, then I think we can make it dependent on only
kasan_hw_tags_enable.

Peter