Re: [PATCH v3 4/5] KVM/vfio: Use file-based reference counting for KVM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm/vfio: memory leak due to circular file reference
--

commit 67e757ae1d0e68e93f61ad99fbdfb9cdb735b83e
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM/vfio: Use file-based reference counting for KVM

This commit replaces manual module reference counting for KVM integration with
file-based reference counting. It passes struct file pointers instead of
struct kvm pointers across the VFIO-KVM interface, using get_file() and
fput() to manage references.

> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index b2299e5bc6df3..5bf8cbdff377c 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -860,11 +860,20 @@ bool vfio_group_enforced_coherent(struct vfio_group *group)
>  	return ret;
>  }
>  
> -void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
> +void vfio_group_set_kvm(struct vfio_group *group, struct file *kvm)
>  {
> +	struct file *old;
> +
> +	if (kvm)
> +		get_file(kvm);
> +
>  	spin_lock(&group->kvm_ref_lock);
> +	old = group->kvm;
>  	group->kvm = kvm;
>  	spin_unlock(&group->kvm_ref_lock);

[Severity: High]
Does this create a circular file reference? VFIO takes a reference on the KVM VM
file here in vfio_group_set_kvm(), while KVM also takes a reference on the
VFIO file.

[ ... ]

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 423ead48aafe3..ed96acfa86358 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -1544,9 +1515,13 @@ bool vfio_file_enforced_coherent(struct file *file)
>  }
>  EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
>  
> -static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm)
> +static void vfio_device_file_set_kvm(struct file *file, struct file *kvm)
>  {
>  	struct vfio_device_file *df = file->private_data;
> +	struct file *old;
> +
> +	if (kvm)
> +		get_file(kvm);
>  
>  	/*
>  	 * The kvm is first recorded in the vfio_device_file, and will
> @@ -1554,28 +1529,32 @@ static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm)
>  	 * iommufd successfully in the vfio device cdev path.
>  	 */
>  	spin_lock(&df->kvm_ref_lock);
> +	old = df->kvm;
>  	df->kvm = kvm;
>  	spin_unlock(&df->kvm_ref_lock);

[Severity: High]
Similarly, does storing the KVM VM file reference in the VFIO device file
here in vfio_device_file_set_kvm() contribute to the circular reference?

[ ... ]

> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 6cdc4e9a333a3..19548a4309426 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -168,7 +173,7 @@ static int kvm_vfio_file_add(struct kvm_device *dev, unsigned int fd)
>  	kvf->file = get_file(filp);
>  	list_add_tail(&kvf->node, &kv->file_list);
>  
> -	kvm_vfio_file_set_kvm(kvf->file, dev->kvm);
> +	kvm_vfio_file_set_kvm(kvf->file, kvm_file);
>  	kvm_vfio_update_coherency(dev);
>  
>  	return 0;

[Severity: High]
Could this complete the circular reference loop? Here KVM takes a reference to
the VFIO file via get_file(filp), while VFIO takes a reference to the KVM file
when kvm_vfio_file_set_kvm() is called.

If a process opens both files, adds the VFIO file descriptor to the KVM VM via
the KVM_DEV_VFIO_GROUP_ADD ioctl, and exits, exit_files() will drop the userspace
FD references. However, both files' f_count remain >= 1 due to these internal
references. Doesn't this cause both files to never reach their release functions,
resulting in a permanent memory and resource leak of the KVM VM and VFIO device
structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-vfio-v3-0-4a294307797b@linux.ibm.com?part=4