Re: [PATCH RFC v3 64/88] vfio: Add vfio_device_gmem_revoke helper for guest_memfd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] vfio: link failure in vfio_device_gmem_revoke() with
  CONFIG_VFIO=y, KVM=m
- [High] vfio: unsynchronized gmem_inode access in
  vfio_device_gmem_revoke()
- [Low] vfio: return 0 in !CONFIG_KVM_GUEST_MEMFD
  vfio_device_gmem_revoke() stub
--

commit 64f7b4c8e914dff002926c09d51e57c7fda9c500
Author: Ackerley Tng <ackerleytng@google.com>

vfio: Add vfio_device_gmem_revoke helper for guest_memfd

This commit adds vfio_device_gmem_revoke() so VFIO bus drivers can revoke
and reclaim device memory ranges from an attached guest_memfd instance.

> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index e4e7bfe77d03b..fd5f71dfc44b7 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -1643,6 +1643,16 @@ static const struct guest_memfd_provider_operations vfio_device_gmem_provider_op
>  	.mmap                 = vfio_device_gmem_mmap,
>  	.check_set_attributes = vfio_device_gmem_check_set_attributes,
>  };
> +
> +int vfio_device_gmem_revoke(struct vfio_device *device, pgoff_t start,
> +			    size_t nr_pages, const unsigned long *pfn)
> +{
> +	if (!device->gmem_inode)
> +		return 0;
> +
> +	return kvm_gmem_revoke(device->gmem_inode, start, nr_pages, pfn);
> +}
> +EXPORT_SYMBOL_GPL(vfio_device_gmem_revoke);
>  #endif

[Severity: High]
Does calling kvm_gmem_revoke() from drivers/vfio/vfio_main.c break linking
vmlinux when CONFIG_VFIO=y and CONFIG_KVM=m?

CONFIG_KVM_GUEST_MEMFD is a bool selected by CONFIG_KVM_X86 on x86_64 even
when CONFIG_KVM=m, which links virt/kvm/guest_memfd.o into kvm.ko while
vfio_main.o is built into vmlinux.  With CONFIG_VFIO=m and CONFIG_KVM=m, the
direct symbol reference also forces vfio.ko to depend on kvm.ko even when
VFIO is used without KVM.

[Severity: High]
Can vfio_device_gmem_revoke() race with vfio_device_gmem_release() and
cause a use-after-free or NULL pointer dereference on device->gmem_inode?

device->gmem_inode is set and cleared under device->dev_set->lock, outside
the bus driver's lock (such as vdev->memory_lock), while bus driver
revocation paths call vfio_device_gmem_revoke() without holding
device->dev_set->lock or pinning the inode:

CPU0                                  CPU1
kvm_gmem_destroy_inode()
  vfio_device_gmem_release()
    mutex_lock(&dev_set->lock)
    ops->gmem_release(device)
      down_write(&vdev->memory_lock)
      ...
      up_write(&vdev->memory_lock)
                                      vfio_pci_zap_bars()
                                        vfio_device_gmem_revoke()
                                          if (!device->gmem_inode)
    device->gmem_inode = NULL;
    mutex_unlock(&dev_set->lock)
  ...
  kvm_gmem_free_inode()
                                          kvm_gmem_revoke(device->gmem_inode,
                                                          ...)
                                            <-- use-after-free / NULL deref

Because device->gmem_inode is read twice without READ_ONCE(), clearing it
on CPU0 can also cause CPU1 to reload NULL as the inode argument to
kvm_gmem_revoke().  Similarly, in vfio_device_gmem_attach(),
ops->gmem_attach() finishes before device->gmem_inode is assigned, leaving
a window where a concurrent vfio_device_gmem_revoke() sees NULL and skips
revocation.

> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index ffb01e655735f..aade091a91fe8 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -394,6 +394,18 @@ bool vfio_file_is_valid(struct file *file);
>  bool vfio_file_enforced_coherent(struct file *file);
>  void vfio_file_set_kvm(struct file *vfio_file, struct file *kvm);
>  
> +#ifdef CONFIG_KVM_GUEST_MEMFD
> +int vfio_device_gmem_revoke(struct vfio_device *device, pgoff_t start,
> +			    size_t nr_pages, const unsigned long *pfn);
> +#else
> +static inline int vfio_device_gmem_revoke(struct vfio_device *device,
> +					  pgoff_t start, size_t nr_pages,
> +					  const unsigned long *pfn)
> +{
> +	return -EOPNOTSUPP;
> +}
> +#endif

[Severity: Low]
Should the !CONFIG_KVM_GUEST_MEMFD stub for vfio_device_gmem_revoke()
return 0 instead of -EOPNOTSUPP?

When CONFIG_KVM_GUEST_MEMFD is enabled and no guest_memfd is attached to
the device (!device->gmem_inode), vfio_device_gmem_revoke() is a no-op and
returns 0.  Returning -EOPNOTSUPP when CONFIG_KVM_GUEST_MEMFD is disabled
makes callers that check the return value fail even though no guest_memfd
can be attached.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gmem-alternative-providers-v3-0-8484dde66a7e@google.com?part=64