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);
