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

From: Catalin Marinas <catalin.marinas@arm.com>
To: Will Deacon <will@kernel.org>
Cc: Peter Collingbourne <pcc@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Evgenii Stepanov <eugenis@google.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5] arm64: mte: allow async MTE to be upgraded to sync on a per-CPU basis
Date: Fri, 18 Jun 2021 16:09:55 +0100	[thread overview]
Message-ID: <20210618150953.GH16116@arm.com> (raw)
In-Reply-To: <20210617215829.GD25403@willie-the-truck>

On Thu, Jun 17, 2021 at 10:58:30PM +0100, Will Deacon wrote:
> On Tue, Jun 15, 2021 at 01:38:07PM -0700, Peter Collingbourne wrote:
> > +Upgrading to stricter tag checking modes
> > +----------------------------------------
> > +
> > +On some CPUs the performance of MTE in stricter tag checking modes
> > +is similar to that of less strict tag checking modes. This makes it
> > +worthwhile to enable stricter checks on those CPUs when a less strict
> > +checking mode is requested, in order to gain the error detection
> > +benefits of the stricter checks without the performance downsides. To
> > +opt into upgrading to a stricter checking mode on those CPUs, the user
> > +can set the ``PR_MTE_DYNAMIC_TCF`` flag bit in the ``flags`` argument
> > +to the ``prctl(PR_SET_TAGGED_ADDR_CTRL, flags, 0, 0, 0)`` system call.
> > +
> > +This feature is currently only supported for upgrading from
> > +asynchronous mode. To configure a CPU to upgrade from asynchronous mode
> > +to synchronous mode, a privileged user may write the value ``1`` to
> > +``/sys/devices/system/cpu/cpu<N>/mte_upgrade_async``, and to disable
> > +upgrading they may write the value ``0``. By default the feature is
> > +disabled on all CPUs.
> > +
> >  Initial process state
> >  ---------------------
> >  
> > @@ -128,6 +147,7 @@ On ``execve()``, the new process has the following configuration:
> >  - ``PR_TAGGED_ADDR_ENABLE`` set to 0 (disabled)
> >  - Tag checking mode set to ``PR_MTE_TCF_NONE``
> >  - ``PR_MTE_TAG_MASK`` set to 0 (all tags excluded)
> > +- ``PR_MTE_DYNAMIC_TCF`` set to 0 (disabled)
> >  - ``PSTATE.TCO`` set to 0
> >  - ``PROT_MTE`` not set on any of the initial memory maps
> 
> Something about this doesn't sit right with me, as we're mixing a per-task
> interface with a per-cpu interface for selecting async/sync MTE and the
> priorities are somewhat confusing.
> 
> I think a better interface would be for the sysfs entry for each CPU to
> allow selection between:
> 
> 	task  : Honour the prctl() (current behaviour)
> 	async : Force async for tasks using MTE
> 	sync  : Force sync for tasks using MTE
> 	none  : MTE disabled
> 
> i.e. the per-cpu setting is an override.

As Peter mentioned, forcing it is a potential ABI break, so such feature
would need backporting to 5.10. There's also a minor use-case that came
up in the early discussions - an app may want to use async mode only for
reporting but forcing it to sync would break such application (since
sync mode prevents the faulty access from taking place).

So I'd rather leave it up to the user task to decide whether its choice
can be changed. Peter introduced a new PR_MTE_DYNAMIC_TCF for this
purpose (or a different name if you have a better suggestion).

I think the other important question is whether we go for an override
style or an upgrade one. Peter chose the latter, though I think an
override is simpler to understand.

BTW, I like the idea of using strings in the sysfs interface than
numbers.

-- 
Catalin

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