[PATCH v5] arm64: mte: allow async MTE to be upgraded to sync on a per-CPU basis

[Peter Collingbourne]

On Wed, Jun 30, 2021 at 8:19 AM Catalin Marinas <catalin.marinas at arm.com> wrote:
>
> On Tue, Jun 29, 2021 at 12:11:17PM -0700, Peter Collingbourne wrote:
> > On Tue, Jun 29, 2021 at 3:46 AM Will Deacon <will at kernel.org> wrote:
> > > On Mon, Jun 28, 2021 at 04:20:24PM +0100, Catalin Marinas wrote:
> > > > Another option is a mapping table where async can be remapped to sync
> > > > and sync to async (or even to "none" for both). That's not far from one
> > > > of Peter's mte-upgrade-async proposal, we just add mte-map-async and
> > > > mte-map-sync options. Most likely we'll just use mte-map-async for now
> > > > to map it to sync on some CPUs but it wouldn't exclude other forced
> > > > settings.
> > >
> > > Catalin and I discussed this offline and ended up with another option:
> > > retrospectively change the prctl() ABI so that the 'flags' argument
> > > accepts a bitmask of modes that the application is willing to accept. This
> > > doesn't break any existing users, as we currently enforce that only one
> > > mode is specified, but it would allow things like:
> > >
> > >         prctl(PR_SET_TAGGED_ADDR_CTRL,
> > >               PR_MTE_TCF_SYNC | PR_MTE_TCF_ASYNC,
> > >               0, 0, 0);
> > >
> > > which is actually very similar to Peter's PR_MTE_DYNAMIC_TCF proposal, with
> > > the difference that I think this extends more naturally as new PR_MTR_TCF_*
> > > flags are introduced.
> > >
> > > Then we expose a per-cpu file in sysfs (say "cpuX/mte_tcf_preferred")
> > > which initially reads as "async". If the root user does, e.g.
> > >
> > >         # echo "sync" > cpu1/mte_tcf_preferred
> > >
> > > then a task which has successfully issued a PR_SET_TAGGED_ADDR_CTRL prctl()
> > > request for PR_MTE_TCF_SYNC | PR_MTE_TCF_ASYNC will run in sync mode on
> > > CPU1, but async mode on other CPUs (assuming they retain the default value).
> > >
> > > We'll need to special-case PR_MTE_TCF_NONE, as that's just a shorthand for
> > > "no flags" so doing PR_MTE_TCF_NONE | PR_MTE_TCF_SYNC is just the same as
> > > doing PR_MTE_TCF_SYNC (which I think is already the behaviour today). The
> > > only values which the sysfs files would accept today are "sync" and "async".
> > >
> > > When faced with a situation where the prctl() flags for a task do not
> > > intersect with the preferred mode for a CPU on which the task is going
> > > to run, the lowest bit number flag is chosen from the mask set by the
> > > prctl().
> > >
> > > Thoughts?
> >
> > This all sounds great and I'm glad you were able to come to an
> > agreement on this. I'll get started on implementing it.
> >
> > Once we have ASYM support I'm not sure if we can rely on bit numbering
> > for ordering, as we will want the ordering to be ASYNC < ASYM < SYNC
> > and ASYNC is bit-adjacent to SYNC. So I think we will need to make
> > ASYM a special case.
>
> The bit position based order - SYNC < ASYNC < ASYM - is indeed arbitrary
> but I think it's easier to follow. When we add ASYM, it will be the last
> one rather than squeezing it in the middle of the current order. Any
> other order somehow implies that one is better than the other but we
> don't have a clear definition for "better".

At least from my perspective "more strict" is a reasonable enough
definition for "better", at least by default, since it seems like what
most users would want. If users really want a different ordering,
perhaps it can be an opt-in behavior.

> > This would also allow NONE to be upgraded by allocating a bit position
> > for NONE, but if we change the value of NONE it may break applications
> > built against new headers running on old kernels, so maybe it should
> > be made a separate constant. This doesn't need to be done immediately,
> > though.
>
> I think we should leave NONE as non-upgradable, the software doesn't
> expect any fault when accessing with the wrong tag. We also can't change
> the definition now as it's already in upstream glibc.

Right, I was thinking that if we made this upgradeable we would
introduce a constant with a different name, like NONE2 or
NONE_UPGRADEABLE or something.

Peter