Re: [PATCH v3 1/2] vfio: add dma-buf get_tph callback and DMA_BUF_TPH feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Data Race / Lockless Concurrency Bug in TPH metadata updates
- [Medium] Data race on `priv->vdev` ownership check during cross-device ioctl invocation
--

commit d00a3ff646f832b56279c8a55984e20630463a25
Author: Zhiping Zhang <zhipingz@meta.com>

vfio: add dma-buf get_tph callback and DMA_BUF_TPH feature

Add a dma-buf callback that returns raw TPH metadata from the exporter
and a new VFIO_DEVICE_FEATURE_DMA_BUF_TPH ioctl to set it.

> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> index f87fd32e4a017..28247602e3599 100644
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c

[ ... ]

> @@ -331,6 +377,71 @@ 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)
> +{

[ ... ]

> +	if (dmabuf->ops != &vfio_pci_dmabuf_ops) {
> +		ret = -EINVAL;
> +		goto out_put;
> +	}
> +
> +	priv = dmabuf->priv;
> +	down_write(&vdev->memory_lock);
> +	if (priv->vdev != vdev) {

Can this race with a concurrent cleanup of another device?

If userspace passes a dmabuf from device A to the ioctl for device B,
device B's memory_lock is held here. If device A is concurrently closed,
vfio_pci_dma_buf_cleanup() could be setting priv->vdev = NULL while holding
device A's memory_lock, leading to a data race on priv->vdev.

Could we use READ_ONCE()/WRITE_ONCE() for priv->vdev, or acquire
dma_resv_lock() before the check?

> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	priv->steering_tag = set_tph.steering_tag;
> +	priv->steering_tag_ext = set_tph.steering_tag_ext;
> +	priv->ph = set_tph.ph;
> +	/*
> +	 * Publish the TPH values before the gate flag, so that lockless
> +	 * readers in vfio_pci_dma_buf_get_tph() see fully-initialized
> +	 * fields once they observe a non-zero tph_flags.
> +	 */
> +	smp_store_release(&priv->tph_flags, set_tph.flags);

What happens if this ioctl is called multiple times on the same dmabuf?

If priv->tph_flags is already non-zero from a previous call, a concurrent
lockless reader in vfio_pci_dma_buf_get_tph() could pass the
smp_load_acquire() check and read the metadata fields while they are being
updated here.

Does this cause a data race or torn reads?

Also, if the second call drops a previously set flag, could the reader
consume uninitialized data from set_tph?

Should we reject updates if priv->tph_flags is already set, or use a
seqlock_t to serialize lockless readers?

> +	ret = 0;
> +
> +out_unlock:
> +	up_write(&vdev->memory_lock);
> +out_put:
> +	dma_buf_put(dmabuf);
> +	return ret;
> +}

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