On Fri, 2023-08-18 at 15:15 -0300, Jason Gunthorpe wrote:
> 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.
Agreed.
>
> > 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() :(
Ah, TIL. Thanks!
>
> Does this work for you?
Yup, for both -ap and -ccw devices that I have handy. Thanks for the
quick turnaround!
Thanks,
Eric
>
> 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);
>
