[PATCH] iommufd/selftest: Simplify iommufd_device_remove_vdev()

[PATCH] iommufd/selftest: Simplify iommufd_device_remove_vdev()

[Jason Gunthorpe]

Peiyang reports that this function indirectly includes a fault injection
point through iommufd_get_object() that was intended to cover the uAPI use
of object IDs, not in places like this that cannot fail.

On deeper inspection this can be written using a dedicated helper to
obtain a users refcount relying entirely on the xa locking instead of
going through the whole get/put scheme. The new helper doesn't need the
fault injection point.

Fixes: 850f14f5b919 ("iommufd: Destroy vdevice on idevice destroy")
Reported-by: Peiyang He <peiyang_he@smail.nju.edu.cn>
Closes: https://lore.kernel.org/r/870BB9ADBBEDDD1A+37c5bfab-ad32-4fc5-a302-57c81a8432b5@smail.nju.edu.cn
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c          | 13 +++----------
 drivers/iommu/iommufd/iommufd_private.h |  9 +--------
 drivers/iommu/iommufd/main.c            | 20 ++++++++++++++++++++
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 170a7005f0bc84..03a4cc07c0e5b7 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -148,29 +148,22 @@ static void iommufd_device_remove_vdev(struct iommufd_device *idev)
 	if (!idev->vdev)
 		goto out_unlock;
 
-	vdev = iommufd_get_vdevice(idev->ictx, idev->vdev->obj.id);
+	vdev = idev->vdev;
+
 	/*
 	 * An ongoing vdev destroy ioctl has removed the vdev from the object
 	 * xarray, but has not finished iommufd_vdevice_destroy() yet as it
 	 * needs the same mutex. We exit the locking then wait on wait_cnt
 	 * reference for the vdev destruction.
 	 */
-	if (IS_ERR(vdev))
+	if (iommufd_inc_users(idev->ictx, &vdev->obj))
 		goto out_unlock;
 
-	/* Should never happen */
-	if (WARN_ON(vdev != idev->vdev)) {
-		iommufd_put_object(idev->ictx, &vdev->obj);
-		goto out_unlock;
-	}
-
 	/*
 	 * vdev is still alive. Hold a users refcount to prevent racing with
 	 * userspace destruction, then use iommufd_object_tombstone_user() to
 	 * destroy it and leave a tombstone.
 	 */
-	refcount_inc(&vdev->obj.users);
-	iommufd_put_object(idev->ictx, &vdev->obj);
 	mutex_unlock(&idev->igroup->lock);
 	iommufd_object_tombstone_user(idev->ictx, &vdev->obj);
 	return;
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 43fbc5bed8de3f..29a850053ea84d 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -182,6 +182,7 @@ static inline bool iommufd_lock_obj(struct iommufd_object *obj)
 	return true;
 }
 
+int iommufd_inc_users(struct iommufd_ctx *ictx, struct iommufd_object *obj);
 struct iommufd_object *iommufd_get_object(struct iommufd_ctx *ictx, u32 id,
 					  enum iommufd_object_type type);
 static inline void iommufd_put_object(struct iommufd_ctx *ictx,
@@ -698,14 +699,6 @@ void iommufd_vdevice_abort(struct iommufd_object *obj);
 int iommufd_hw_queue_alloc_ioctl(struct iommufd_ucmd *ucmd);
 void iommufd_hw_queue_destroy(struct iommufd_object *obj);
 
-static inline struct iommufd_vdevice *
-iommufd_get_vdevice(struct iommufd_ctx *ictx, u32 id)
-{
-	return container_of(iommufd_get_object(ictx, id,
-					       IOMMUFD_OBJ_VDEVICE),
-			    struct iommufd_vdevice, obj);
-}
-
 #ifdef CONFIG_IOMMUFD_TEST
 int iommufd_test(struct iommufd_ucmd *ucmd);
 void iommufd_selftest_destroy(struct iommufd_object *obj);
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 8c6d43601afbed..1b2867fb502c65 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -180,6 +180,26 @@ struct iommufd_object *iommufd_get_object(struct iommufd_ctx *ictx, u32 id,
 	return obj;
 }
 
+/*
+ * Increment the users count of an object outside the context of an ioctl
+ * that has already locked it. The users refcount cannot be increased unless
+ * the object is installed in the xarray, otherwise things are racing with
+ * a parallel destruction.
+ */
+int iommufd_inc_users(struct iommufd_ctx *ictx, struct iommufd_object *obj)
+{
+	struct iommufd_object *cur;
+
+	xa_lock(&ictx->objects);
+	cur = xa_load(&ictx->objects, obj->id);
+	if (cur == obj)
+		refcount_inc(&obj->users);
+	xa_unlock(&ictx->objects);
+	if (cur != obj)
+		return -EBUSY;
+	return 0;
+}
+
 static int iommufd_object_dec_wait(struct iommufd_ctx *ictx,
 				   struct iommufd_object *to_destroy)
 {

base-commit: dba4254e216d7482d91d93e45faa3ccbefe79337
-- 
2.43.0

Re: [PATCH] iommufd/selftest: Simplify iommufd_device_remove_vdev()

[Nicolin Chen]

On Mon, Jul 13, 2026 at 02:25:11PM -0300, Jason Gunthorpe wrote:
> Peiyang reports that this function indirectly includes a fault injection
> point through iommufd_get_object() that was intended to cover the uAPI use
> of object IDs, not in places like this that cannot fail.
> 
> On deeper inspection this can be written using a dedicated helper to
> obtain a users refcount relying entirely on the xa locking instead of
> going through the whole get/put scheme. The new helper doesn't need the
> fault injection point.
> 
> Fixes: 850f14f5b919 ("iommufd: Destroy vdevice on idevice destroy")
> Reported-by: Peiyang He <peiyang_he@smail.nju.edu.cn>
> Closes: https://lore.kernel.org/r/870BB9ADBBEDDD1A+37c5bfab-ad32-4fc5-a302-57c81a8432b5@smail.nju.edu.cn
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

RE: [PATCH] iommufd/selftest: Simplify iommufd_device_remove_vdev()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, July 14, 2026 1:25 AM
> 
> Peiyang reports that this function indirectly includes a fault injection
> point through iommufd_get_object() that was intended to cover the uAPI use
> of object IDs, not in places like this that cannot fail.
> 
> On deeper inspection this can be written using a dedicated helper to
> obtain a users refcount relying entirely on the xa locking instead of
> going through the whole get/put scheme. The new helper doesn't need the
> fault injection point.

I thought a plain refcount_inc_not_zero() in iommufd_device_remove_vdev()
could work too, but having the new helper check xarray is more defensive.

> 
> +/*
> + * Increment the users count of an object outside the context of an ioctl
> + * that has already locked it. The users refcount cannot be increased unless

this is a bit misleading as existing refcount_inc() in iommufd_device_bind()
etc. is also outside the ioctl context but they don't need call this helper 
as the object is already held.

> + * the object is installed in the xarray, otherwise things are racing with
> + * a parallel destruction.
> + */
> +int iommufd_inc_users(struct iommufd_ctx *ictx, struct iommufd_object
> *obj)

iommufd_try_inc_users()?

> +{
> +	struct iommufd_object *cur;
> +
> +	xa_lock(&ictx->objects);
> +	cur = xa_load(&ictx->objects, obj->id);
> +	if (cur == obj)
> +		refcount_inc(&obj->users);
> +	xa_unlock(&ictx->objects);
> +	if (cur != obj)
> +		return -EBUSY;
> +	return 0;
> +}
> +

otherwise,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH] iommufd/selftest: Simplify iommufd_device_remove_vdev()

[Jason Gunthorpe]

On Tue, Jul 14, 2026 at 01:11:09AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, July 14, 2026 1:25 AM
> > 
> > Peiyang reports that this function indirectly includes a fault injection
> > point through iommufd_get_object() that was intended to cover the uAPI use
> > of object IDs, not in places like this that cannot fail.
> > 
> > On deeper inspection this can be written using a dedicated helper to
> > obtain a users refcount relying entirely on the xa locking instead of
> > going through the whole get/put scheme. The new helper doesn't need the
> > fault injection point.
> 
> I thought a plain refcount_inc_not_zero() in iommufd_device_remove_vdev()
> could work too, but having the new helper check xarray is more defensive.

It does seem like that would be safe alone

> > + * the object is installed in the xarray, otherwise things are racing with
> > + * a parallel destruction.
> > + */
> > +int iommufd_inc_users(struct iommufd_ctx *ictx, struct iommufd_object
> > *obj)
> 
> iommufd_try_inc_users()?

Sure

Thanks,
Jason

RE: [PATCH] iommufd/selftest: Simplify iommufd_device_remove_vdev()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, July 14, 2026 1:25 AM
>
[...] 
>  drivers/iommu/iommufd/device.c          | 13 +++----------
>  drivers/iommu/iommufd/iommufd_private.h |  9 +--------
>  drivers/iommu/iommufd/main.c            | 20 ++++++++++++++++++++
>  3 files changed, 24 insertions(+), 18 deletions(-)
> 

btw for the subject line, it's clearer to remove 'selftest' as the touched code
has no direct link to selftest (though it's where the issue was reported).