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

[Catalin Marinas <catalin.marinas@arm.com>]

On Mon, Jun 28, 2021 at 11:14:49AM +0100, Will Deacon wrote:
> On Fri, Jun 25, 2021 at 02:53:50PM +0100, Catalin Marinas wrote:
> > On Fri, Jun 25, 2021 at 01:39:59PM +0100, Will Deacon wrote:
> > > On Fri, Jun 25, 2021 at 01:01:37PM +0100, Catalin Marinas wrote:
> > > > So we can document that the mode requested by the app is an indication,
> > > > the system may change it to another value (and back-port documentation
> > > > to 5.10). If we get a request from developers to honour a specific mode,
> > > > we can add a new PR_MTE_TCF_EXACT bit or something but it's not
> > > > essential we do it now.
> > > > 
> > > > So if we allow the kernel to change the user requested mode (via sysfs),
> > > > I think we still have two more issues to clarify:
> > > > 
> > > > 1. Do we allow only "upgrade" (for some meaning of this) or sysfs can
> > > >    downgrade to a less strict mode. I'd go for upgrade here to a
> > > >    stricter check as in Peter's patch.
> > > > 
> > > > 2. Should the sysfs upgrade the PR_MTE_TCF_NONE? _MTAG_ENABLE does that,
> > > >    so I'd say yes.
> > > > 
> > > > Any other thoughts are welcome.
> > > 
> > > As I mentioned before, I think the sysfs interface should offer:
> > > 
> > > 	"task"	: Honour whatever the task has asked for (default)
> > > 	"async" : Force async on this CPU
> > > 	"sync"  : Force sync on this CPU
> > > 
> > > I don't think we should upgrade PR_MTE_TCF_NONE unless we also have a "none"
> > > option in here. I originally suggested that, but in hindsight it feels like
> > > a bad idea because a task could SIGILL on migration. So what we're saying is
> > > that PR_MTE_TCF_SYNC and PR_MTE_TCF_ASYNC will always enable MTE on success,
> > > but the reporting mode is a hint.
> > > 
> > > I don't think upgrade/downgrade makes a lot of sense given that the sysfs
> > > controls can be changed at any point in time. It should just be an override.
> > 
> > The problem with sysfs is that it's global, so it assumes that any
> > process has the same needs. The _MTAG_ENABLE glibc tunable at least can
> > be set per process.
> 
> Again, this seems to be an argument against doing this at all. We already
> have a per-task interface to change the checking mode, but there is a need
> to override this on a per-cpu basis to achieve acceptable performance.
> Applications can't possibly be aware of that and so their "needs" cannot be
> taken into account here.

That's because we have two conflicting needs: (a) more precise checking
for debugging or increased security and (b) better performance. The
sysfs interface is primarily meant for (b) but that overrides any
application choice for (a).

> > > This means that we can force async for CPUs where sync mode is horribly
> > > slow, whilst honouring the task's request on CPUs which are better
> > > implemented.
> > 
> > This may hamper debugging on, for example, a system where the root
> > configured the modes for CPUs and a normal user wants to use MTE to
> > identify access bugs. Another case is some service that wants tightened
> > security from MTE irrespective of the performance.
> 
> I suppose the way I see this is similar to how I see CPU errata: essentially
> sync mode is unusably slow on some CPUs, so we disable it (drop to async) on
> a per-cpu basis. The only difference is that we provide the switch to
> userspace, since there isn't a functional problem. However, when we
> inevitably hit real errata, we could even force the mode in the kernel
> rather than disable the feature entirely.

I think everyone assumes that sync is slow, so they'd only set it for
debugging or some increased security. It's quite hard to quantify what a
"too slow" sync is.

> > The slight downside of the "upgrade" mode assumes that the user is aware
> > that async is the fastest and asks for this unless it has specific
> > needs. Of course, we can also extend the interface to "sync-force" or
> > "sync-upgrade" etc. but I think it's over-engineered.
> 
> Another reason I dislike "upgrade" is because it means that the kernel embeds
> an ordering of which mode upgrades to another mode and, as new modes get
> added by the architecture in future, this feels more like policy to me and
> would be better off handled in userspace.

Yeah, I don't like this aspect of the "upgrade" either but I'd accept it
as a compromise.

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

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