Re: [PATCH v3 03/16] iommufd/viommu: Add IOMMU_VDEVICE_ALLOC ioctl

On Thu, Oct 17, 2024 at 03:52:30PM -0300, Jason Gunthorpe wrote:

...

• if (viommu->ops && viommu->ops->vdevice_alloc)

• vdev = viommu->ops->vdevice_alloc(viommu, idev->dev, virt_id);
  

• else

• vdev = __iommufd_vdevice_alloc(ucmd->ictx, sizeof(*vdev));
  

• if (IS_ERR(vdev)) {

• rc = PTR_ERR(vdev);
  

• goto out_unlock_igroup;
  

• }
• vdev->idev = idev;
• vdev->id = virt_id;
• vdev->viommu = viommu;
• idev->vdev = vdev;
• refcount_inc(&idev->obj.users);
• refcount_inc(&viommu->obj.users);
• curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
• if (curr) {

• rc = xa_err(curr) ? : -EBUSY;
  

• goto out_abort;
  

• }
• cmd->out_vdevice_id = vdev->obj.id;
• rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
• if (rc)

• goto out_abort;
  

• iommufd_object_finalize(ucmd->ictx, &vdev->obj);
• goto out_unlock_igroup;

+out_abort:

• iommufd_object_abort_and_destroy(ucmd->ictx, &vdev->obj);

But be mindful of this abort, it doesn't want to be inside the lock if it also gets the lock.. fail_nth should be updated to cover these new ioctls to look for tricky things like that

I added an abort() beside destroy():

+void iommufd_vdevice_abort(struct iommufd_object *obj) +{ + struct iommufd_vdevice *old, *vdev = + container_of(obj, struct iommufd_vdevice, obj); + struct iommufd_viommu *viommu = vdev->viommu; + struct iommufd_device *idev = vdev->idev; + + lockdep_assert_not_held(&idev->igroup->lock); + + if (viommu->ops && viommu->ops->vdevice_free) + viommu->ops->vdevice_free(vdev); + + old = xa_cmpxchg(&viommu->vdevs, vdev->id, vdev, NULL, GFP_KERNEL); + if (old) + WARN_ON(old != vdev); + + refcount_dec(&viommu->obj.users); + refcount_dec(&idev->obj.users); + idev->vdev = NULL; +} + +void iommufd_vdevice_destroy(struct iommufd_object *obj) +{ + struct iommufd_vdevice *vdev = + container_of(obj, struct iommufd_vdevice, obj); + + mutex_lock(&vdev->idev->igroup->lock); + iommufd_vdevice_abort(obj); + mutex_unlock(&vdev->idev->igroup->lock); +} ----------------------------------------------------------

And I added fail_nth coverage for IOMMU_VIOMMU/VDEVICE_ALLOC cmds.

Thanks Nicolin