[PATCH v1 4/4] arm64/mte: Add userspace interface for enabling asymmetric mode

[Catalin Marinas]

On Fri, Mar 04, 2022 at 01:09:00PM -0800, Peter Collingbourne wrote:
> On Thu, Mar 3, 2022 at 2:41 AM Catalin Marinas <catalin.marinas at arm.com> wrote:
> > On Wed, Mar 02, 2022 at 12:58:48PM -0800, Evgenii Stepanov wrote:
> > > On Wed, Mar 2, 2022 at 11:33 AM Mark Brown <broonie at kernel.org> wrote:
> > > > On Wed, Mar 02, 2022 at 10:44:31AM -0800, Evgenii Stepanov wrote:
> > > > > On Wed, Mar 2, 2022 at 5:10 AM Mark Brown <broonie at kernel.org> wrote:
> > > > > > On Wed, Mar 02, 2022 at 11:44:53AM +0000, Catalin Marinas wrote:
> > > > > > > On Tue, Mar 01, 2022 at 04:52:01PM -0800, Evgenii Stepanov wrote:
> > > >
> > > > > > > > Extending PR_MTE_TCF_MASK seems bad for backward compatibility. User
> > > > > > > > code may do "flags =& ~PR_MTE_TCF_MASK" to disable MTE; when compiled
> > > > > > > > against an old version of the header this would fail to remove the ASYMM
> > > > > > > > bit.
> > > >
> > > > > > > But if the app is compiled against an old version, it wouldn't set
> > > > > > > MTE_CTRL_TCF_ASYMM either as it doesn't have the definition.
> > > >
> > > > > Libraries within a single process can be built against different
> > > > > header versions. In our case, this is libc vs the app: we expect to
> > > > > set all 3 mode bits when an app asks for "async" to enable the
> > > > > mte_tcf_preferred logic. Even if the app is built against an older NDK
> > > > > and unaware of the Asymm mode existence!
> > > >
> > > > I can't see how we can resolve that case in the kernel except by adding
> > > > a specific call to disable all MTE modes which would obviously only be
> > > > useful for future proofing given that no existing applications would
> > > > support it.
> > >
> > > One option would be to introduce a new, future-proofed prctl with a
> > > wider mask, and throw in a few extra reserved bits just in case. Then
> > > have the legacy prctl always clear MTE_CTRL_TCF_ASYMM.
> >
> > If this problem is real, we can easily tweak the current proposal so the
> > that ASYMM can only be set *if* both ASYNC and SYNC are set. IOW, it's
> > not a specific mode an app requests on its own but rather something the
> > kernel may choose via the preferred mode if the app opted in. We still
> > introduce a new bit for ASYMM but not change the mask.
> 
> As discussed out-of-band, I've never really liked this API style of
> trying to cram everything into a single prctl(), not least because of
> these compatibility concerns.

I'd say the main problem is not necessarily the single prctl() which, at
least initially, followed the hw control closely. The two main issues I
see are: (1) unclear user-space ownership of such control and (2) the
ABI change from single mode request to a mask of supported application
modes which we did not foresee the need for when first adding MTE, nor
did we see the potential problems with the asymmetric mode later.

In Evgenii's use-case, it seems that it's not the libc owning the MTE
controls but any other piece of code that may not be recompiled to the
latest libc headers. We could argue that it's a user problem to solve
and not that different from apps that use a non-zero tag or some pointer
arithmetics and get confused by an MTE-aware allocator.

Can any libc versioning help with making sure old apps are not linked
against newer libc with such change? I guess this only works for symbols
but here it's a macro.

> Evgenii proposed a prctl() with a wider
> mask, but I think this sort of approach just kicks the can down the
> road (well, maybe for *this* feature we can't expect there to be too
> many new modes added, but can we say the same for everything else in
> tagged_addr_ctrl?).

Extending the mask and adding a few more bits doesn't solve the original
problem raised by Evgenii: applications compiled against current headers
but (dynamically) linked against future libc could be confused if they
manage the prctl() themselves.

> And an attempt to guess the user intent from which
> bits are set seems prone to failure and unnecessarily restrictive.

Only allowing asymmetric mode if both symmetric and async are supported
is not that confusing. The only downside is that one cannot ask for
asymmetric mode only but is this such a big problem? For testing one can
always set the sysfs preferred mode to asymm. It will be some time
before we see asymm in production.

> It
> seems far more preferable to have a separate prctl() control for the
> TCF bits and start working towards splitting out the other bits to
> their own prctl()s. Then any user code that manipulates these bits
> will "naturally" work. For compatibility the TCF bits would still be
> accessible via the existing prctl(), but an attempt to set TCF via
> this prctl() would clear the ASYMM bit.

In hindsight, this would have been better. Right now we have to make
sure we don't break the ABI and it's not only about setting the TCF but
also getting the controls (prctl(PR_GET_TAGGED_ADDR_CTRL), ptrace()).

Assuming we have a different prctl() for setting TCF, what would we
report in the current PR_GET_ for existing apps when only the asymm mode
was set? By the same logic as the TCF mask breaking current apps, this
doesn't work if somehow an app uses the information and, for example,
retry a faulting operation indefinitely.

I don't see MTE gaining more TCF modes in the future but it's not
excluded to get other controls.

> The only downside that I've seen mentioned is that we will end up
> issuing more syscalls on process startup which could harm performance.

I don't think that's the only downside. It's an ABI change as well
w.r.t. getting the current TCF setting and asymm being invisible to an
unaware app. The only way out I see is to make asymm a kernel choice
when both sync and async are selected (potentially with another bit to
opt it but without changing the TCF mask).

-- 
Catalin