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

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Sep 08, 2022 at 10:27:06PM +0100, Robin Murphy wrote:

> Oh, because s390 is using iommu_get_domain_for_dev() in its release_device
> callback, which needs to dereference the group to work, and the current
> domain may also be a non-default one which we can't prevent from
> disappearing racily, that was why :(

Hum, the issue there is the use of device->iommu_group - but that just
means I didn't split properly. How about this incremental:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c451bf715182ac..99ef799f3fe6b5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -351,6 +351,7 @@ void iommu_release_device(struct device *dev)
 	 * them until the have been detached. release_device() is expected to
 	 * detach all domains connected to the dev.
 	 */
+	dev->iommu_group = NULL;
 	kobject_put(group->devices_kobj);
 
 	module_put(ops->owner);
@@ -980,7 +981,6 @@ static void __iommu_group_remove_device(struct device *dev)
 
 	kfree(device->name);
 	kfree(device);
-	dev->iommu_group = NULL;
 }
 
 /**
@@ -995,6 +995,7 @@ void iommu_group_remove_device(struct device *dev)
 	struct iommu_group *group = dev->iommu_group;
 
 	__iommu_group_remove_device(dev);
+	dev->iommu_group = NULL;
 	kobject_put(group->devices_kobj);
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);

To me it makes sense that the driver should be able to continue to
query the iommu_group during release anyhow..

And to your other question, the reason I split the function is because
I couldn't really say WTF iommu_group_remove_device() was supposed to
do. The __ version make ssense as part of the remove_device, due to
the sequencing with ops->release()

But the other one doesn't have that. So I want to put in a:

   WARN_ON(group->blocking_domain || group->default_domain);

Because calling it after those domains are allocated looks broken to
me.

Jason