Re: [PATCH v8 06/15] iommufd/device: Hold a KVM file reference for the device lifetime

[Alexey Kardashevskiy <aik@amd.com>]

On 6/11/26 18:16, Tian, Kevin wrote:
>> From: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>> Sent: Thursday, November 5, 2026 12:16 AM
>>
>> The KVM file stored in an iommufd_device currently relies on the
>> reference held by VFIO while the device is open. Normally, unbind waits
>> for outstanding device references before VFIO drops its KVM reference.
>> However, REMOVE_WAIT only waits for 60 seconds. If that timeout expires,
>> the device object remains in the iommufd xarray.
>>
>> For example, an IOMMU_GET_HW_INFO ioctl can hold a device reference
>> while
>> blocked on a userspace buffer fault:
>>
>>    iommufd_fops_ioctl()
>>      iommufd_get_hw_info()
>>        iommufd_get_device()
>>          iommufd_get_object()
>>            iommufd_lock_obj()       /* takes users and wait_cnt refs */
>>        copy_to_user() / clear_user() /* may block before putting refs */
>>
>> A concurrent final release of the VFIO device file takes this path:
>>
>>    vfio_device_fops_release()
>>      vfio_df_unbind_iommufd()
>>        vfio_df_close()
>>          vfio_df_device_last_close()
>>            vfio_df_iommufd_unbind()
>>              vfio_iommufd_physical_unbind()
>>                iommufd_device_unbind()
>>                  iommufd_object_destroy_user()
>>                    iommufd_object_remove(..., REMOVE_WAIT)
>>                      iommufd_object_dec_wait() /* timeout: -EBUSY */
>>                    WARN_ON(ret)
>>        vfio_device_put_kvm()
>>          fput(device->kvm)
>>
>> iommufd_object_destroy_user() warns on -EBUSY but does not propagate it:
>> iommufd_device_unbind() returns void. Thus VFIO close effectively ignores
>> the unbind failure and proceeds to drop its KVM file reference even
>> though the iommufd_device is still alive and its ID remains usable.
>>
>> Once the remaining KVM file references are released, a later ioctl on
>> the still-open iommufd can access the stale pointer:
>>
>>    iommufd_fops_ioctl()
>>      iommufd_viommu_alloc_ioctl()
>>        iommufd_get_device()         /* finds the surviving device ID */
>>        get_file(idev->kvm_file)     /* accesses the freed KVM file */
>>
>> Take a KVM file reference when binding the device and release it when
>> the iommufd_device is actually destroyed. Release it before the igroup
>> check in the destructor so that bind failure cleanup is covered too.
>> Document the ownership in the bind API and device structure.
> 
> I don't think this needs to be fixed. There is already a WARN_ON()
> in iommufd_object_destroy_user() and many bad things could
> happen afterwards...

imho it is just a right thing to do - hold a reference to the pointer in a structure.

> If we really think an ioctl could take more than 60s in sane scenarios,

It is quite possible when some TSM operation involves (in)validation of hundreds GBs of MMIO.

> it's a more complex issue to be handled in iommufd core.
> 
>>
>> Fixes: d8bb80c8f0e5 ("iommufd/device: Associate KVM file pointer with
>> iommufd_device")
> 
> btw no fix tag for an issue introduced by an earlier patch in the
> same series.

+1. should be just squashed into 05/15. Thanks,


-- 
Alexey