Re: [RFC PATCH] iommufd: Destroy vdevice on device unbind

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Jason Gunthorpe <jgg@ziepe.ca> writes:

> On Tue, Jun 17, 2025 at 01:37:04PM +0530, Aneesh Kumar K.V wrote:
>  
>> How do we reclaim that object id for further reuse? 
>
> Maybe just don't? Userspace did something it shouldn't, it now leaked
> 8 bytes of kernel memory until the FD is closed.
>

Between the two sequences below, Sequence 1 is the correct one, since we
want the object ID to be released after calling ioctl(DESTROY,
vdevice_id), right?

Sequence 1 (Correct):

close(vfio_cdev)          → triggers vdevice destruction  
ioctl(DESTROY, vdevice_id) → reclaims vdevice object ID  
close(iommufd)  

Sequence 2:

ioctl(DESTROY, vdevice_id) → returns EBUSY  
close(vfio_cdev)           → triggers vdevice destruction  
close(iommufd)  

Just to confirm: We agree that an EBUSY return from ioctl(DESTROY,
vdevice_id) is expected if it's called before vfio_df_unbind_iommufd(),
correct?

>> is it that if there is a request for a iommufd_object_remove() with object
>> refcount > 1, we insert a XA_ZERO_ENTRY and convert that to NULL entry
>> on IOMMU_DESTROY?
>
> Oh no we can't do that, if the refcount is elevated that is a problem,
> it means some thread somewhere is using that memory.
>
> We can sleep and wait for shortterm_users to go to zero and if users
> is still elevated then we are toast. WARN_ON and reatin it in the
> xarray and hope for the best.
>
> So the thread that will trigger the detruction needs to have a users
> refcount of 1. Meaning users needs to be one while idle in the xarray,
> and the idevice destruction will obtain a users=2 from its pointer
> under some kind of lock.
>
>> -enum {
>> -	REMOVE_WAIT_SHORTTERM = 1,
>> -};
>> +#define	REMOVE_WAIT_SHORTTERM	BIT(0)
>> +#define	REMOVE_OBJ_FORCE	BIT(1)
>
> You can keep the enum for flags, but 'force' isn't the right name. I
> would think it is 'tombstone'
>

These values represent bit flags (e.g., 1, 2, 4, ...), meaning they are
not mutually exclusive and can be combined using bitwise operations. As
such, using an enum—which is typically intended for mutually exclusive
values—is not appropriate in this case?

>
>> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
>> index b7aa725e6b37..d27b61787a53 100644
>> --- a/drivers/iommu/iommufd/main.c
>> +++ b/drivers/iommu/iommufd/main.c
>> @@ -88,7 +88,8 @@ struct iommufd_object *iommufd_get_object(struct iommufd_ctx *ictx, u32 id,
>>  
>>  	xa_lock(&ictx->objects);
>>  	obj = xa_load(&ictx->objects, id);
>> -	if (!obj || (type != IOMMUFD_OBJ_ANY && obj->type != type) ||
>> +	if (!obj || xa_is_zero(obj) ||
>> +	    (type != IOMMUFD_OBJ_ANY && obj->type != type) ||
>>  	    !iommufd_lock_obj(obj))
>
> xa_load can't return xa_is_zero(), xas_load() can
>
> We already use XA_ZERO_ENTRY to hold an ID during allocation till
> finalize.
>
> I think you want to add a new API
>
> iommufd_object_tombstone_user(idev->ictx, &idev->vdev->obj);
>
> Which I think is the same as the existing
> iommufd_object_destroy_user() except it uses tombstone..
>
> The only thing tombstone does is:
>
> 	xas_store(&xas, (flags & REMOVE_OBJ_TOMBSTONE) ? XA_ZERO_ENTRY : NULL);
>
> All the rest of the logic including the users and shorterm check would
> be the same.
>
>> --- a/drivers/iommu/iommufd/viommu.c
>> +++ b/drivers/iommu/iommufd/viommu.c
>> @@ -213,6 +213,8 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
>>  	/* vdev lifecycle now managed by idev */
>>  	idev->vdev = vdev;
>>  	refcount_inc(&vdev->obj.users);
>> +	/* Increment refcount since userspace can hold the obj id */
>> +	refcount_inc(&vdev->obj.users);
>>  	goto out_put_idev_unlock;
>
> I don't think this should change.. There should be no extra user refs
> or userspace can't destroy it.
>
> The pointer back from the idevice needs locking to protect it while a
> refcount is obtained.
>
> Jason

-aneesh