Re: [PATCH v9 2/4] arm64: mte: change ASYNC and SYNC TCF settings into bitfields

From: Will Deacon <will@kernel.org>
To: Peter Collingbourne <pcc@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Evgenii Stepanov <eugenis@google.com>,
	Szabolcs Nagy <szabolcs.nagy@arm.com>,
	Tejas Belagod <Tejas.Belagod@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v9 2/4] arm64: mte: change ASYNC and SYNC TCF settings into bitfields
Date: Tue, 13 Jul 2021 18:27:49 +0100	[thread overview]
Message-ID: <20210713172748.GB30304@willie-the-truck> (raw)
In-Reply-To: <CAMn1gO4gJ=8w5CEYcivMf2Nn4PRzRp_pBi_2ArRJ0p7ats5wig@mail.gmail.com>

On Mon, Jul 12, 2021 at 12:04:39PM -0700, Peter Collingbourne wrote:
> On Wed, Jul 7, 2021 at 4:11 AM Will Deacon <will@kernel.org> wrote:
> > On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:
> > >  long set_mte_ctrl(struct task_struct *task, unsigned long arg)
> > >  {
> > > -     u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
> > >       u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
> > >                       SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
> > >
> > >       if (!system_supports_mte())
> > >               return 0;
> > >
> > > -     switch (arg & PR_MTE_TCF_MASK) {
> > > -     case PR_MTE_TCF_NONE:
> > > -             sctlr |= SCTLR_EL1_TCF0_NONE;
> > > -             break;
> > > -     case PR_MTE_TCF_SYNC:
> > > -             sctlr |= SCTLR_EL1_TCF0_SYNC;
> > > -             break;
> > > -     case PR_MTE_TCF_ASYNC:
> > > -             sctlr |= SCTLR_EL1_TCF0_ASYNC;
> > > -             break;
> > > -     default:
> > > -             return -EINVAL;
> > > -     }
> > > +     if (arg & PR_MTE_TCF_ASYNC)
> > > +             mte_ctrl |= MTE_CTRL_TCF_ASYNC;
> > > +     if (arg & PR_MTE_TCF_SYNC)
> > > +             mte_ctrl |= MTE_CTRL_TCF_SYNC;
> > >
> > > -     if (task != current) {
> > > -             task->thread.sctlr_user = sctlr;
> > > -             task->thread.mte_ctrl = mte_ctrl;
> > > -     } else {
> > > -             set_task_sctlr_el1(sctlr);
> > > -             set_gcr_el1_excl(mte_ctrl);
> > > +     task->thread.mte_ctrl = mte_ctrl;
> > > +     if (task == current) {
> > > +             mte_update_sctlr_user(task);
> >
> > In conjunction with the next patch, what happens if we migrate at this
> > point? I worry that we can install a stale sctlr_user value.
> >
> > > +             set_task_sctlr_el1(task->thread.sctlr_user);
> 
> In this case, we will call mte_update_sctlr_user when scheduled onto
> the new CPU as a result of the change to mte_thread_switch, and both
> the scheduler and prctl will set SCTLR_EL1 to the new (correct) value
> for the current CPU.

Doesn't that rely on task->thread.sctlr_user being explicitly read on the
new CPU? For example, the following rough sequence is what I'm worried
about:

CPU x (prefer ASYNC)
set_mte_ctrl(ASYNC | SYNC)
	current->thread.mte_ctrl = ASYNC | SYNC;
	mte_update_sctlr_user
		current->thread.sctlr_user = ASYNC;
	Register Xn = current->thread.sctlr_user; // ASYNC
	<migration to CPU y>

CPU y (prefer SYNC)
mte_thread_switch
	mte_update_sctlr_user
		next->thread.sctlr_user = SYNC;
	update_sctlr_el1
		SCTLR_EL1 = SYNC;

	<resume next back in set_mte_ctrl>
	set_task_sctlr_el1(Xn); // ASYNC
	current->thread.sctlr_user = Xn; // ASYNC  XXX: also superfluous?
	SCTLR_EL1 = ASYNC;

Does that make sense?

I'm thinking set_mte_ctrl() should be using update_sctlr_el1() and disabling
preemption around the whole thing, which would make it a lot closer to the
context-switch path.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel