Re: [PATCH 4/4] iommu: Fix ordering of iommu_release_device()

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Sep 09, 2022 at 06:57:59PM +0100, Robin Murphy wrote:

> > > That then only leaves the issue that that domain may still become
> > > invalid at any point after the group mutex has been dropped.
> > 
> > So that is this race:
> > 
> >          CPU0                         CPU1
> >     iommu_release_device(a)
> >        __iommu_group_remove_device(a)
> > 			         iommu_device_use_default_domain(b)
> >                                   iommu_domain_free(domain)
> >                                   iommu_release_device(b)
> >                                        ops->release_device(b)
> >        ops->release_device(a)
> >          // Boom, a is still attached to domain :(
> > 
> > I can't think of how to solve this other than holding the group mutex
> > across release_device. See below.
> 
> I see a few possibilities:
> 
> - Backtrack slightly on its removal, and instead repurpose detach_dev
> into a specialised domain cleanup callback, called before or during
> iommu_group_remove_device(), with the group mutex held.

See below for why that is somewhat troublesome..
 
> - Drivers that hold any kind of internal per-device references to
> domains - which is generally the root of this issue in the first place -
> can implement proper reference counting, so even if a domain is "freed"
> with a device still attached as above, it doesn't actually go away until
> release_device(a) cleans up the final dangling reference. I suggested
> the core doing this generically, but on reflection I think it's actually
> a lot more straightforward as a driver-internal thing.

Isn't this every driver though? Like every single driver implementing
an UNMANAGED/DMA/DMA_FQ domain has a hidden reference to the
iommu_domain - minimally to point the HW to the IOPTEs it stores.

> - Drivers that basically just keep a list of devices in the domain and
> need to do a list_del() in release_device, can also list_del_init() any
> still-attached devices in domain_free, with a simple per-instance or
> global lock to serialise the two.

Compared to just locking ops->release_device() these all seem more
complicated?

IMHO the core code should try to protect the driver from racing
release with anything else.

Do you know a reason not to hold the group mutex across
release_device? I think that is the most straightforward and
future proof.

Arguably all the device ops should be serialized under the group
mutex.

> @@ -1022,6 +1030,14 @@ void iommu_group_remove_device(struct device *dev)
>  	dev_info(dev, "Removing from iommu group %d\n", group->id);
>  	mutex_lock(&group->mutex);
> +	if (WARN_ON(group->domain != group->default_domain &&
> +		    group->domain != group->blocking_domain)) {

This will false trigger, if there are two VFIO devices then the group
will remained owned when we unplug one just of them, but the group's domain
will be a VFIO owned domain. 

It is why I put the list_empty() protection, as the test only works if
it is the last device.

> +		if (group->default_domain)
> +			__iommu_attach_device(group->default_domain, dev);
> +		else
> +			__iommu_detach_device(group->domain, dev);
> +	}

This was very appealing, but I rejected it because it is too difficult
to support multi-device groups that share the RID.

In that case we expect that the first attach/detach of a device on the
shared RID will reconfigure the iommu and the attach/deatch of all the
other devices on the group with the same parameters will be a NOP.

So in a VFIO configuration where two drivers are bound to a single
group with shared RID and we unplug one device, this will rebind the
shared RID and thus the entire group to blocking/default and break the
still running VFIO on the second device.

The device centric interface only works if we always apply the
operation to every device in the group..

This is why I kept it as ops->release_device() with an implicit detach
of the current domain inside the driver. release_device() has that
special meaning of 'detach the domain but do not change a shared RID'

And it misses the logic to WARN_ON if a domain is set and an external
entity wrongly uses iommu_group_remove_device()..

Jason