Re: [PATCH v10 3/4] vfio/pci: implement get_pci_tph and DMA_BUF_TPH feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Lockless plain load of `priv->vdev` races with concurrent modification in cleanup path.
- [Low] Missing `WRITE_ONCE()` for `priv->revoked` leads to data race with `READ_ONCE()`.
--

commit d524eab36e9482385fe1c1a6c6280e667bf56a9c
Author: Zhiping Zhang <zhipingz@meta.com>
Subject: vfio/pci: implement get_pci_tph and DMA_BUF_TPH feature

This commit implements dma-buf get_pci_tph for vfio-pci exported
dma-bufs and adds VFIO_DEVICE_FEATURE_DMA_BUF_TPH. This allows
userspace to publish TPH metadata for a VFIO-owned device.

> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> index c16f460c01d68..8de72f9e75024 100644
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
> @@ -30,7 +38,7 @@ static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf,
>  	if (!attachment->peer2peer)
>  		return -EOPNOTSUPP;
>  
> -	if (priv->revoked)
> +	if (READ_ONCE(priv->revoked))

[Severity: Low]
Since this updates the read of priv->revoked to use READ_ONCE(), should the
corresponding writers in vfio_pci_dma_buf_move() also be updated to use
WRITE_ONCE()?

For example, when a concurrent state change like a device reset writes to
this field:

vfio_pci_dma_buf_move() {
        ...
        priv->revoked = true;
        ...
}

Will the lack of WRITE_ONCE() here trigger KCSAN data race warnings when
evaluated concurrently by the new READ_ONCE() check?

>  		return -ENODEV;
>  
>  	if (!dma_buf_attach_revocable(attachment))

[ ... ]

> @@ -333,6 +362,72 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
>  	return ret;
>  }
>  
> +int vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev,
> +				      u32 flags,
> +				      struct vfio_device_feature_dma_buf_tph __user *arg,
> +				      size_t argsz)
> +{

[ ... ]

> +	dmabuf = dma_buf_get(set_tph.dmabuf_fd);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	if (dmabuf->ops != &vfio_pci_dmabuf_ops) {
> +		ret = -EINVAL;
> +		goto out_put;
> +	}
> +
> +	priv = dmabuf->priv;
> +	if (priv->vdev != vdev) {

[Severity: Low]
Can this lockless read of priv->vdev race with concurrent modifications in
the cleanup path?

If userspace calls the VFIO_DEVICE_FEATURE_DMA_BUF_TPH ioctl on device A,
passing a dmabuf_fd belonging to device B, while device B is concurrently
being closed, could this race with vfio_pci_dma_buf_cleanup()?

vfio_pci_dma_buf_cleanup() {
        ...
        priv->vdev = NULL;
        ...
}

Will this concurrent plain store trigger KCSAN warnings during the pointer
equality check?

> +		ret = -EINVAL;
> +		goto out_put;
> +	}
> +
> +	ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630224328.3218796-1-zhipingz@meta.com?part=3