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

[Alex Williamson <alex@shazbot.org>]

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,

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 */