Re: [PATCH] vfio: Get/put KVM only for the first/last vfio_df_open/close in cdev path

[Yi Liu <yi.l.liu@intel.com>]

On 2024/7/1 16:43, Tian, Kevin wrote:
>> From: Zhao, Yan Y <yan.y.zhao@intel.com>
>> Sent: Friday, June 28, 2024 11:19 PM
>>
>> In the device cdev path, adjust the handling of the KVM reference count to
>> only increment with the first vfio_df_open() and decrement after the final
>> vfio_df_close(). This change addresses a KVM reference leak that occurs
>> when a device cdev file is opened multiple times and attempts to bind to
>> iommufd repeatedly.
>>
>> Currently, vfio_df_get_kvm_safe() is invoked prior to each vfio_df_open()
>> in the cdev path during iommufd binding. The corresponding
>> vfio_device_put_kvm() is executed either when iommufd is unbound or if an
>> error occurs during the binding process.
>>
>> However, issues arise when a device binds to iommufd more than once. The
>> second vfio_df_open() will fail during iommufd binding, and
>> vfio_device_put_kvm() will be triggered, setting device->kvm to NULL.
>> Consequently, when iommufd is unbound from the first successfully bound
>> device, vfio_device_put_kvm() becomes ineffective, leading to a leak in the
>> KVM reference count.
> 
> To be accurate this happens only when two binds are issued via different
> fds otherwise below check will happen earlier when two binds are in a
> same fd:
> 
> 	/* one device cannot be bound twice */
> 	if (df->access_granted) {
> 		ret = -EINVAL;
> 		goto out_unlock;
> 	}

yes

>>
>> Below is the calltrace that will be produced in this scenario when the KVM
>> module is unloaded afterwards, reporting "BUG kvm_vcpu (Tainted: G S):
>> Objects remaining in kvm_vcpu on __kmem_cache_shutdown()".
>>
>> Call Trace:
>>   <TASK>
>>   dump_stack_lvl+0x80/0xc0
>>   slab_err+0xb0/0xf0
>>   ? __kmem_cache_shutdown+0xc1/0x4e0
>>   ? rcu_is_watching+0x11/0x50
>>   ? lock_acquired+0x144/0x3c0
>>   __kmem_cache_shutdown+0x1b7/0x4e0
>>   kmem_cache_destroy+0xa6/0x260
>>   kvm_exit+0x80/0xc0 [kvm]
>>   vmx_exit+0xe/0x20 [kvm_intel]
>>   __x64_sys_delete_module+0x143/0x250
>>   ? ktime_get_coarse_real_ts64+0xd3/0xe0
>>   ? syscall_trace_enter+0x143/0x210
>>   do_syscall_64+0x6f/0x140
>>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>
>> Fixes: 5fcc26969a16 ("vfio: Add VFIO_DEVICE_BIND_IOMMUFD")
>> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
>> ---
>>   drivers/vfio/device_cdev.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
>> index bb1817bd4ff3..3b85d01d1b27 100644
>> --- a/drivers/vfio/device_cdev.c
>> +++ b/drivers/vfio/device_cdev.c
>> @@ -65,6 +65,7 @@ long vfio_df_ioctl_bind_iommufd(struct
>> vfio_device_file *df,
>>   {
>>   	struct vfio_device *device = df->device;
>>   	struct vfio_device_bind_iommufd bind;
>> +	bool put_kvm = false;
>>   	unsigned long minsz;
>>   	int ret;
>>
>> @@ -101,12 +102,15 @@ long vfio_df_ioctl_bind_iommufd(struct
>> vfio_device_file *df,
>>   	}
>>
>>   	/*
>> -	 * Before the device open, get the KVM pointer currently
>> +	 * Before the device's first open, get the KVM pointer currently
>>   	 * associated with the device file (if there is) and obtain
>> -	 * a reference.  This reference is held until device closed.
>> +	 * a reference.  This reference is held until device's last closed.
>>   	 * Save the pointer in the device for use by drivers.
>>   	 */
>> -	vfio_df_get_kvm_safe(df);
>> +	if (device->open_count == 0) {
>> +		vfio_df_get_kvm_safe(df);
>> +		put_kvm = true;
>> +	}
>>
>>   	ret = vfio_df_open(df);
>>   	if (ret)
>> @@ -129,7 +133,8 @@ long vfio_df_ioctl_bind_iommufd(struct
>> vfio_device_file *df,
>>   out_close_device:
>>   	vfio_df_close(df);
>>   out_put_kvm:
>> -	vfio_device_put_kvm(device);
>> +	if (put_kvm)
>> +		vfio_device_put_kvm(device);
>>   	iommufd_ctx_put(df->iommufd);
>>   	df->iommufd = NULL;
>>   out_unlock:
>>
> 
> what about extending vfio_df_open() to unify the get/put_kvm()
> and open_count trick in one place?
> 
> int vfio_df_open(struct vfio_device_file *df, struct kvm *kvm,
> 	spinlock_t *kvm_ref_lock)
> {

this should work. But need a comment to note why need pass in both kvm
and kvm_ref_lock given df has both of them. :)

> 	struct vfio_device *device = df->device;
> 	int ret = 0;
> 	
> 	lockdep_assert_held(&device->dev_set->lock);
> 
> 	if (device->open_count == 0) {
> 		spin_lock(kvm_ref_lock);
> 		vfio_device_get_kvm_safe(device, kvm);
> 		spin_unlock(kvm_ref_lock);
> 	}

perhaps it can be put in the "if (device->open_count == 1)" branch, just
before invoking vfio_df_device_first_open().

> 
> 	/*
> 	 * Only the group path allows the device to be opened multiple
> 	 * times.  The device cdev path doesn't have a secure way for it.
> 	 */
> 	if (device->open_count != 0 && !df->group)
> 		return -EINVAL;
> 
> 	device->open_count++;
> 	if (device->open_count == 1) {
> 		ret = vfio_df_device_first_open(df);
> 		if (ret)
> 			device->open_count--;
> 	}
> 
> 	if (ret)
> 		vfio_device_put_kvm(device);
> 	return ret;
> }
> 
> void vfio_df_close(struct vfio_device_file *df)
> {
>   	struct vfio_device *device = df->device;
> 
> 	lockdep_assert_held(&device->dev_set->lock);
> 
> 	vfio_assert_device_open(device);
> 	if (device->open_count == 1) {
> 		vfio_df_device_last_close(df);
> 		vfio_device_put_kvm(device);
> 	}
> 	device->open_count--;
> }

-- 
Regards,
Yi Liu