Re: [PATCH v8 9/9] vfio/pci: Revoke a DMABUF on request from userspace

[Matt Evans <matt@ozlabs.org>]

Hi Alex,

On 14/10/2026 00:13, Alex Williamson wrote:
> On Tue,  6 Oct 2026 20:36:37 +0100
> Matt Evans <matt@ozlabs.org> wrote:
>> +#ifdef CONFIG_VFIO_PCI_DMABUF
>> +int vfio_pci_core_feature_dma_buf_revoke(
>> +	struct vfio_pci_core_device *vdev, u32 flags,
>> +	struct vfio_device_feature_dma_buf_revoke __user *arg,
>> +	size_t argsz)
>> +{
>> +	struct vfio_device_feature_dma_buf_revoke db_revoke;
>> +	struct vfio_pci_dma_buf *priv;
>> +	struct dma_buf *dmabuf;
>> +	int ret;
>> +
>> +	if (!vdev->pci_ops || !vdev->pci_ops->get_dmabuf_phys)
>> +		return -EOPNOTSUPP;
>> +
>> +	ret = vfio_check_feature(flags, argsz,
>> +				 VFIO_DEVICE_FEATURE_SET,
>> +				 sizeof(db_revoke));
>> +	if (ret != 1)
>> +		return ret;
>> +
>> +	if (copy_from_user(&db_revoke, arg, sizeof(db_revoke)))
>> +		return -EFAULT;
>> +
>> +	dmabuf = dma_buf_get(db_revoke.dmabuf_fd);
>> +	if (IS_ERR(dmabuf))
>> +		return PTR_ERR(dmabuf);
>> +
>> +	priv = dmabuf->priv;
>> +	/*
>> +	 * Sanity-check the DMABUF is really a vfio_pci_dma_buf _and_
>> +	 * relates to the VFIO device it was provided with.
>> +	 *
>> +	 * If the DMABUF relates to this vdev then priv->vdev is
>> +	 * stable because this open fd prevents cleanup.
>> +	 *
>> +	 * If it relates to a different vdev, reading priv->vdev might
>> +	 * race with a concurrent cleanup on that device.  But if so,
>> +	 * it points to a non-matching vdev or NULL and is unusable
>> +	 * either way.
>> +	 */
>> +	if (dmabuf->ops != &vfio_pci_dmabuf_ops ||
>> +	    READ_ONCE(priv->vdev) != vdev) {
>> +		ret = -ENODEV;
>> +		goto out_put_buf;
>> +	}
>> +
>> +	/*
>> +	 * memory_lock(R) is taken to stop vfio_pci_dev_set_hot_reset()
>> +	 * from getting it and then blocking all devices in the dev_set behind
>> +	 * this revoke's drain.
>> +	 */
>> +	down_read(&vdev->memory_lock);
>> +	down_write(&vdev->dmabuf_lock);
>> +	if (priv->status == VFIO_PCI_DMABUF_DEAD) {
>> +		ret = -EBADFD;
> 
> Kind of ugly to special case the condition in the caller that's already
> tested in the function.  Why not return an errno for this purpose?
> Thanks,

Sure, especially since that comparison's already there.

(Rather than generating -EBADFD in the function (that error doesn't make 
a lot of sense outside of this ioctl), it can return $anything and get 
converted here.)

Thanks,


Matt

> 
> Alex
> 
>> +	} else {
>> +		vfio_pci_dma_buf_set_status(priv, VFIO_PCI_DMABUF_DEAD);
>> +		ret = 0;
>> +	}
>> +	up_write(&vdev->dmabuf_lock);
>> +	up_read(&vdev->memory_lock);
>> +
>> +out_put_buf:
>> +	dma_buf_put(dmabuf);
>> +
>> +	return ret;
>> +}
>> +#endif /* CONFIG_VFIO_PCI_DMABUF */
>