Re: [PATCH v5 01/13] iommufd/viommu: Add IOMMUFD_OBJ_VDEVICE and IOMMU_VDEVICE_ALLOC ioctl

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Oct 25, 2024 at 04:50:30PM -0700, Nicolin Chen wrote:
> +/**
> + * struct iommu_vdevice_alloc - ioctl(IOMMU_VDEVICE_ALLOC)
> + * @size: sizeof(struct iommu_vdevice_alloc)
> + * @viommu_id: vIOMMU ID to associate with the virtual device
> + * @dev_id: The pyhsical device to allocate a virtual instance on the vIOMMU
> + * @__reserved: Must be 0
> + * @virt_id: Virtual device ID per vIOMMU, e.g. vSID of ARM SMMUv3, vDeviceID
> + *           of AMD IOMMU, and vID of a nested Intel VT-d to a Context Table.
> + * @out_vdevice_id: Output virtual instance ID for the allocated object

How about:

@out_vdevice_id: Object handle for the vDevice. Pass to IOMMU_DESTORY


> + * Allocate a virtual device instance (for a physical device) against a vIOMMU.
> + * This instance holds the device's information (related to its vIOMMU) in a VM.
> + */
> +struct iommu_vdevice_alloc {
> +	__u32 size;
> +	__u32 viommu_id;
> +	__u32 dev_id;
> +	__u32 __reserved;
> +	__aligned_u64 virt_id;
> +	__u32 out_vdevice_id;
> +	__u32 __reserved2;

Lets not have two u32 reserved, put the out_vdevice_id above virt_id

> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 5fd3dd420290..e50113305a9c 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -277,6 +277,17 @@ EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD);
>   */
>  void iommufd_device_unbind(struct iommufd_device *idev)
>  {
> +	u32 vdev_id = 0;
> +
> +	/* idev->vdev object should be destroyed prior, yet just in case.. */
> +	mutex_lock(&idev->igroup->lock);
> +	if (idev->vdev)

Then should it have a WARN_ON here?

> +		vdev_id = idev->vdev->obj.id;
> +	mutex_unlock(&idev->igroup->lock);
> +	/* Relying on xa_lock against a race with iommufd_destroy() */
> +	if (vdev_id)
> +		iommufd_object_remove(idev->ictx, NULL, vdev_id, 0);

That doesn't seem right, iommufd_object_remove() should never be used
to destroy an object that userspace created with an IOCTL, in fact
that just isn't allowed.

Ugh, there is worse here, we can't hold a long term reference on a
kernel owned object:

	idev->vdev = vdev;
	refcount_inc(&idev->obj.users);

As it prevents the kernel from disconnecting it.

I came up with this that seems like it will work. Maybe we will need
to improve it later. Instead of using the idev, just keep the raw
struct device. We can hold a refcount on the struct device without
races. There is no need for the idev igroup lock since the xa_lock
does everything we need.

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index e50113305a9c47..5fd3dd42029015 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -277,17 +277,6 @@ EXPORT_SYMBOL_NS_GPL(iommufd_ctx_has_group, IOMMUFD);
  */
 void iommufd_device_unbind(struct iommufd_device *idev)
 {
-	u32 vdev_id = 0;
-
-	/* idev->vdev object should be destroyed prior, yet just in case.. */
-	mutex_lock(&idev->igroup->lock);
-	if (idev->vdev)
-		vdev_id = idev->vdev->obj.id;
-	mutex_unlock(&idev->igroup->lock);
-	/* Relying on xa_lock against a race with iommufd_destroy() */
-	if (vdev_id)
-		iommufd_object_remove(idev->ictx, NULL, vdev_id, 0);
-
 	iommufd_object_destroy_user(idev->ictx, &idev->obj);
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD);
diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
index 9849474f429f98..6e870bce2a0cd0 100644
--- a/drivers/iommu/iommufd/driver.c
+++ b/drivers/iommu/iommufd/driver.c
@@ -46,6 +46,6 @@ struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 	lockdep_assert_held(&viommu->vdevs.xa_lock);
 
 	vdev = xa_load(&viommu->vdevs, vdev_id);
-	return vdev ? vdev->idev->dev : NULL;
+	return vdev ? vdev->dev : NULL;
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_viommu_find_dev, IOMMUFD);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 365cf5a56cdf20..275f954235940c 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -152,9 +152,6 @@ static inline void iommufd_put_object(struct iommufd_ctx *ictx,
 		wake_up_interruptible_all(&ictx->destroy_wait);
 }
 
-int iommufd_verify_unfinalized_object(struct iommufd_ctx *ictx,
-				      struct iommufd_object *to_verify);
-
 void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj);
 void iommufd_object_abort_and_destroy(struct iommufd_ctx *ictx,
 				      struct iommufd_object *obj);
@@ -391,7 +388,6 @@ struct iommufd_device {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;
 	struct iommufd_group *igroup;
-	struct iommufd_vdevice *vdev;
 	struct list_head group_item;
 	/* always the physical device */
 	struct device *dev;
@@ -523,7 +519,7 @@ void iommufd_vdevice_abort(struct iommufd_object *obj);
 struct iommufd_vdevice {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;
-	struct iommufd_device *idev;
+	struct device *dev;
 	struct iommufd_viommu *viommu;
 	u64 id; /* per-vIOMMU virtual ID */
 };
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 696ac9e0e74b89..c90fe15af98be4 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -43,9 +43,10 @@ void iommufd_object_finalize(struct iommufd_ctx *ictx,
 {
 	void *old;
 
-	old = xa_store(&ictx->objects, obj->id, obj, GFP_KERNEL);
+	old = xa_cmpxchg(&ictx->objects, obj->id, XA_ZERO_ENTRY, obj,
+			 GFP_KERNEL);
 	/* obj->id was returned from xa_alloc() so the xa_store() cannot fail */
-	WARN_ON(old);
+	WARN_ON(old != XA_ZERO_ENTRY);
 }
 
 /* Undo _iommufd_object_alloc() if iommufd_object_finalize() was not called */
@@ -89,26 +90,6 @@ struct iommufd_object *iommufd_get_object(struct iommufd_ctx *ictx, u32 id,
 	return obj;
 }
 
-int iommufd_verify_unfinalized_object(struct iommufd_ctx *ictx,
-				      struct iommufd_object *to_verify)
-{
-	XA_STATE(xas, &ictx->objects, 0);
-	struct iommufd_object *obj;
-	int rc = 0;
-
-	if (!to_verify || !to_verify->id)
-		return -EINVAL;
-	xas.xa_index = to_verify->id;
-
-	xa_lock(&ictx->objects);
-	obj = xas_load(&xas);
-	/* Being an unfinalized object, the loaded obj is a reserved space */
-	if (obj != XA_ZERO_ENTRY)
-		rc = -ENOENT;
-	xa_unlock(&ictx->objects);
-	return rc;
-}
-
 static int iommufd_object_dec_wait_shortterm(struct iommufd_ctx *ictx,
 					     struct iommufd_object *to_destroy)
 {
diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index 2b9a9a80298d8e..e7385676f17659 100644
--- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c
@@ -55,12 +55,6 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_hwpt;
 	}
 
-	rc = iommufd_verify_unfinalized_object(ucmd->ictx, &viommu->obj);
-	if (rc) {
-		kfree(viommu);
-		goto out_put_hwpt;
-	}
-
 	viommu->type = cmd->type;
 	viommu->ictx = ucmd->ictx;
 	viommu->hwpt = hwpt_paging;
@@ -95,27 +89,18 @@ 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_held(&idev->igroup->lock);
 
 	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;
+	put_device(vdev->dev);
 }
 
 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);
 }
 
 int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
@@ -140,30 +125,16 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_viommu;
 	}
 
-	mutex_lock(&idev->igroup->lock);
-	if (idev->vdev) {
-		rc = -EEXIST;
-		goto out_unlock_igroup;
-	}
-
 	vdev = iommufd_object_alloc(ucmd->ictx, vdev, IOMMUFD_OBJ_VDEVICE);
 	if (IS_ERR(vdev)) {
 		rc = PTR_ERR(vdev);
 		goto out_unlock_igroup;
 	}
 
-	rc = iommufd_verify_unfinalized_object(ucmd->ictx, &vdev->obj);
-	if (rc) {
-		kfree(vdev);
-		goto out_unlock_igroup;
-	}
-
-	vdev->idev = idev;
 	vdev->id = virt_id;
+	vdev->dev = idev->dev;
+	get_device(idev->dev);
 	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);
@@ -182,7 +153,6 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 out_abort:
 	iommufd_object_abort_and_destroy(ucmd->ictx, &vdev->obj);
 out_unlock_igroup:
-	mutex_unlock(&idev->igroup->lock);
 	iommufd_put_object(ucmd->ictx, &idev->obj);
 out_put_viommu:
 	iommufd_put_object(ucmd->ictx, &viommu->obj);