Re: [GIT PULL 4/6] KVM/vfio: Use file-based reference counting for KVM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] vfio: Circular file reference between KVM and VFIO introduces massive VM leak
- [High] kvm: build breakage due to missing file_to_kvm_* and struct kvm->file
- [High] vfio/kvm: circular file reference cycle prevents KVM and VFIO cleanup
--

commit 658cd9bf8e504d53c76b0f2917e7380f6de55ab6
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM/vfio: Use file-based reference counting for KVM
    
This commit replaces manual module reference counting with file-based
reference counting for KVM integration. It passes struct file pointers
instead of struct kvm pointers throughout the VFIO-KVM interface.

> diff --git a/arch/s390/kvm/s390/pci.c b/arch/s390/kvm/s390/pci.c
> index 82892e1e03d94..d79bd3bdc68e9 100644
> --- a/arch/s390/kvm/s390/pci.c
> +++ b/arch/s390/kvm/s390/pci.c
> @@ -498,17 +498,22 @@ static void kvm_s390_pci_dev_release(struct zpci_dev *zdev)
>   * available, enable them and let userspace indicate whether or not they will
>   * be used (specify SHM bit to disable).
>   */
> -static int kvm_s390_pci_register_kvm(void *opaque, struct kvm *kvm)
> +static int kvm_s390_pci_register_kvm(void *opaque, struct file *kvm_file)
>  {
>  	struct zpci_dev *zdev = opaque;
> +	struct kvm *kvm;
>  	int rc;
>  
>  	if (!zdev)
>  		return -EINVAL;
>  
> +	kvm = file_to_kvm_s390(kvm_file);

[Severity: High]
Does this cause a build failure? I couldn't find the definition for
file_to_kvm_s390() in this patch or anywhere in the tree. The same issue
appears in drivers/s390/crypto/vfio_ap_ops.c for vfio_ap_mdev_set_kvm().

[ ... ]

> diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
> index 7e8195a311bb0..f12558dfcd812 100644
> --- a/arch/x86/kvm/mmu/page_track.c
> +++ b/arch/x86/kvm/mmu/page_track.c
> @@ -237,10 +239,11 @@ static int kvm_enable_external_write_tracking(struct kvm *kvm)
>   * register the notifier so that event interception for the tracked guest
>   * pages can be received.
>   */
> -int kvm_page_track_register_notifier(struct kvm *kvm,
> +int kvm_page_track_register_notifier(struct file *file,
>  				     struct kvm_page_track_notifier_node *n)
>  {
>  	struct kvm_page_track_notifier_head *head;
> +	struct kvm *kvm = file_to_kvm_x86(file);

[Severity: High]
Similarly, is file_to_kvm_x86() defined? It appears to be missing, which
would break the x86 build.

[ ... ]

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 423ead48aafe3..ea8e6cc13c457 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);

[Severity: Critical]
Does taking a strong reference to the KVM file here create a circular
reference cycle?

When userspace adds a VFIO device to a KVM VM, kvm_vfio_file_add() takes a
reference to the VFIO file. If vfio_device_file_set_kvm() then takes a
reference to the KVM file, both files hold references to each other.

Because both file descriptors hold each other's references, their f_count
will never reach 0 when userspace closes the FDs, permanently leaking the
KVM VM and the VFIO structures.

[ ... ]

> 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
> @@ -144,6 +144,7 @@ static int kvm_vfio_file_add(struct kvm_device *dev, unsigned int fd)
>  {
>  	struct kvm_vfio *kv = dev->private;
>  	struct kvm_vfio_file *kvf;
> +	struct file *kvm_file __free(fput) = NULL;
>  	struct file *filp __free(fput) = NULL;
>  
>  	filp = fget(fd);
> @@ -154,6 +155,10 @@ static int kvm_vfio_file_add(struct kvm_device *dev, unsigned int fd)
>  	if (!kvm_vfio_file_is_valid(filp))
>  		return -EINVAL;
>  
> +	kvm_file = get_file_active(&dev->kvm->file);

[Severity: High]
Does struct kvm contain a member named 'file'? Looking at
include/linux/kvm_host.h, it doesn't appear to exist, which would cause a
compilation error here.

> +	if (!kvm_file)
> +		return -ENOENT;
> +
>  	guard(mutex)(&kv->lock);
>  
>  	list_for_each_entry(kvf, &kv->file_list, node) {
> @@ -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);

[Severity: Critical]
This is where KVM takes its strong file reference on the VFIO device file,
which completes the reference cycle discussed above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009142749.186650-5-borntraeger@linux.ibm.com?part=1