Re: [PATCH v3 03/17] iommu: Make __iommu_group_set_domain() handle error unwind

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 13, 2023 at 05:47:14PM +0100, Robin Murphy wrote:
> On 06/04/2023 8:09 pm, Jason Gunthorpe wrote:
> > On Thu, Apr 06, 2023 at 07:08:59PM +0100, Robin Murphy wrote:
> > 
> > > > +		if (ret) {
> > > > +			result = ret;
> > > > +			/*
> > > > +			 * Keep trying the other devices in the group. If a
> > > > +			 * driver fails attach to an otherwise good domain, and
> > > > +			 * does not support blocking domains, it should at least
> > > > +			 * drop its reference on the current domain so we don't
> > > > +			 * UAF.
> > > > +			 */
> > > > +			if (flags & IOMMU_SET_DOMAIN_MUST_SUCCEED)
> > > > +				continue;
> > > 
> > > I know it's called out above, but my brain truly can't cope with a flag
> > > named "MUST_SUCCEED" which actually means "can fail, but you can't reason
> > > about the state after failure". As before, I think the actual flag itself
> > > can go away, but the behaviour itself will still be no nicer for being
> > > implicit :/
> > 
> > What I saw is that a bunch of drivers tend to implement attach_dev
> > with first some kind of detach, which often cannot fail, and then can
> > fail the new attach.
> > 
> > So, this is intended to go along with that design pattern. We call
> > every device's attach and hope that the outcome is either to release
> > the current iommu_domain or to attach to the new one.
> 
> It's that design pattern which is most of the problem, though.

Yes, this is definately true

> It's a left-over from the old pre-default-domain era (quite
> literally, in most cases, where drivers were just minimally bodged
> to simulate the previous core call of ops->detach_dev). In the
> modern always-attached-to-something paradigm, drivers should
> absolutely be able to figure out if an attach will fail *before*
> they throw away any existing device state.

That is fine

I think I muddled the description I gave last time.

The loop is doing this:

	for_each_group_device(group, gdev) {
		ret = __iommu_device_set_domain(group, gdev->dev, new_domain,
						flags);
		if (ret) {
			result = ret;
			/*
			 * Keep trying the other devices in the group. If a
			 * driver fails attach to an otherwise good domain, and
			 * does not support blocking domains, it should at least
			 * drop its reference on the current domain so we don't
			 * UAF.
			 */
			if (flags & IOMMU_SET_DOMAIN_MUST_SUCCEED)
				continue;
			goto err_revert;

And then in __iommu_device_set_domain:

	ret = __iommu_attach_device(new_domain, dev);
	if (ret) {
		/*
		 * If we have a blocking domain then try to attach that in hopes
		 * of avoiding a UAF. Modern drivers should implement blocking
		 * domains as global statics that cannot fail.
		 */
		if ((flags & IOMMU_SET_DOMAIN_MUST_SUCCEED) &&
		    group->blocking_domain &&
		    group->blocking_domain != new_domain)
			__iommu_attach_device(group->blocking_domain, dev);

So, for "old" drivers the ops->attach_dev will disconnect the domain while
it fails.

"new" drivers will leave the domain attached and the above will then
go on an explicitly use the blocking_domain.

So the purpose of the loop is to go over every device and detach the
domain. Either with the sketchy old driver implicit detach or the new
driver explicit blocking domain attach.

The blocking domain part isn't 100% finished yet, as
group->blocking_domain is often NULL, and might be an UNMANAGED
domain, but from a failure management perspecitive this is what I am
proposing.

> If we reach the point of adding core code that only serves to help drivers
> emulate the new API using the old API because we'd rather undermine the new
> API design than fix the drivers, then frankly that seems like an argument
> for just reverting back to the old API.

I don't see it as undermining, what a new high quality driver should
implement is:

 - Do all attach checks before changing any state. Once state changing
   happens it should succeed

 - Thus leave the old domain in place on attach failure

 - Provide a blocking domain with an attach_dev that cannot fail

 - If attach must fail and cannot go back to the old domain, it must
   go to a blocking domain

 - Re-attach of a previously attached domain should succeed

 - [missing] provide the blocking domain pointer through the device
   ops so it is always available.

 - Do not fail when attaching the blocking domain

Effectively the blocking domain is filling the error handling role
left by detach_dev. What we are doing is unmuddling the mixture of
detach_dev behaviors where some drivers set IDENTITY and some drivers
set BLOCKING.

> > The primary purpose of the MUST_SUCCEED flag is to prevent the code
> > from trying to go back to the old domain. This really should only
> > depend on the caller's context and ability to handle errors, so I
> > don't see what would make it implicit?
> 
> It could be implicit *because* it's contextual; the domain types already
> carry as much information as the flag would. Attaching to a non-core
> (unmanaged or SVA) domain type is the one case where core code has clear
> reason to unwind if an attach fails mid-group. 

It is not universal, eg the "change default domain" flow looks like this:

	/* We must set default_domain early for __iommu_device_set_domain */
	group->default_domain = dom;
	if (!group->domain) {
          ..
	} else {
		ret = __iommu_group_set_domain(group, dom);
		if (ret) {

In this case even though we are attaching the (new) default domain
what we want is to unroll back to group->domain and leave the old
default_domain in place.

I find it much clearer to directly specify the error unwind expecation
of the caller based on the caller's logic and ability to handle the
returned error than to try an infer it.

> Attaching from non-core to core should represent a repeat of something which
> previously succeeded for the given device+domain combination, so whether we
> permit that to fail-to-limbo or mandate that drivers must be able to repeat
> a previous success mostly depends on what we think is a reasonable
> expectation of how to design a driver

I think most drivers are OK to repeat, but some like s390 have FW
interfaces that are not reliable, so repeat can fail there. I would
say a drivers should strive for a design that repeat should work.

The repeat safe drivers never hit these error cases anyhow..

When thinking about this error handling I was looking at cases like
s390. For their flow the first thing attach does is a FW call to set a
blocking behavior, if they have a failure from that point they can
leave the device blocking. When the core code asks for blocking during
the error handling it is NOP.

Jason