Re: [PATCH v2 0/4] Fix device_lock deadlock on two probe() paths

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Aug 18, 2023 at 02:00:21PM -0400, Eric Farman wrote:

> Well, I'm trying to chase down an apparent deadlock in the mdev cases
> that is introduced with the commit [1] blamed by these fixes. Seems
> that when iommu_group_{add|remove}_device gets called out of vfio (for
> example, vfio-ap or -ccw), the device lock is already held so
> attempting to get it again isn't going to go well.

Oh, yes. Thankfully due to all the recent cleanup there is now only
one caller of iommu_group_add_device() - VFIO and only on the
vfio_register_emulated_iommu_dev() path.

All those callers are under mdev probe callbacks so they all must have
the device lock held. iommu_group_remove_device is the same. Simple
fix to just assert the device lock is held.

> I'm puzzled why lockdep isn't shouting over this for me, so I added a
> lockdep_assert_not_held() in those paths to get a proper callchain:

The driver core mostly disables lockdep on the device_lock() :(

Does this work for you?

Thanks,
Jason

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 18162049bd2294..1f58bfacb47951 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1166,12 +1166,11 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 {
 	struct group_device *gdev;
 
-	device_lock(dev);
+	device_lock_assert(dev);
+
 	gdev = iommu_group_alloc_device(group, dev);
-	if (IS_ERR(gdev)) {
-		device_unlock(dev);
+	if (IS_ERR(gdev))
 		return PTR_ERR(gdev);
-	}
 
 	iommu_group_ref_get(group);
 	dev->iommu_group = group;
@@ -1179,7 +1178,6 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	mutex_lock(&group->mutex);
 	list_add_tail(&gdev->list, &group->devices);
 	mutex_unlock(&group->mutex);
-	device_unlock(dev);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
@@ -1195,14 +1193,13 @@ void iommu_group_remove_device(struct device *dev)
 {
 	struct iommu_group *group;
 
-	device_lock(dev);
+	device_lock_assert(dev);
+
 	group = dev->iommu_group;
 	if (group) {
 		dev_info(dev, "Removing from iommu group %d\n", group->id);
 		__iommu_group_remove_device(dev);
 	}
-	device_unlock(dev);
-
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);