Re: [PATCH v9 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 3 potential issue(s) to consider:
- [High] Unconditional validation of the flag-gated `ph` syscall parameter can falsely reject valid requests to clear TPH metadata.
- [Medium] Packing new `tph_*` bitfields into the same byte as the existing `revoked` bitfield introduces a data race.
- [Medium] Lockless plain read of `priv->vdev` can race with a concurrent write of `NULL` during device cleanup.
--

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

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.
It adds VFIO_DEVICE_FEATURE_DMA_BUF_TPH to allow 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..d6f5dd3210009 100644
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
> @@ -19,7 +20,14 @@ struct vfio_pci_dma_buf {
>  	u32 nr_ranges;
>  	struct kref kref;
>  	struct completion comp;
> -	u8 revoked : 1;
> +
> +	/* Protected by dmabuf->resv. */
> +	u16 tph_st_ext;
> +	u8 tph_st;
> +	u8 revoked:1;
> +	u8 tph_st_valid:1;
> +	u8 tph_st_ext_valid:1;
> +	u8 tph_ph:2;
>  };

[Severity: Medium]
Does this bitfield packing introduce a data race?

While the new tph_* fields are safely updated under the dmabuf->resv lock
in vfio_pci_core_feature_dma_buf_tph(), existing code in
vfio_pci_dma_buf_move() reads the revoked bit without holding the
dmabuf->resv lock:

vfio_pci_dma_buf_move()
    if (priv->revoked != revoked) {
        dma_resv_lock(priv->dmabuf->resv, NULL);
        ...

Because they share the same underlying byte, the locked read-modify-write of
the tph_* fields will concurrently access the byte while the lockless read of
the revoked flag occurs. This can violate the C memory model and trigger
KCSAN warnings. Could the revoked flag be declared as a standalone bool to
avoid sharing the byte with the dynamic tph_* bitfields?

> [ ... ]
> @@ -333,6 +362,72 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
> [ ... ]
> +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 (copy_from_user(&set_tph, arg, sizeof(set_tph)))
> +		return -EFAULT;
> +
> +	if (set_tph.flags & ~(VFIO_DMA_BUF_TPH_ST | VFIO_DMA_BUF_TPH_ST_EXT))
> +		return -EINVAL;
> +
> +	if (set_tph.ph & ~0x3)
> +		return -EINVAL;

[Severity: High]
Can this unconditional check falsely reject valid requests to clear TPH
metadata?

When userspace sets the flags to 0 to clear or invalidate published TPH
metadata, the ph parameter is unused and might contain uninitialized stack
garbage. Since syscall parameters that are only meaningful when certain flags
are set should not be validated when those flags are absent, could this
cause legitimate clearing requests to fail with -EINVAL?

> [ ... ]
> +	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) {
> +		ret = -EINVAL;
> +		goto out_put;
> +	}

[Severity: Medium]
Could this plain read of priv->vdev race with a concurrent device cleanup?

Here, priv->vdev is accessed to check ownership without holding any locks
that would protect against cleanup. Concurrently, if the original exporting
device is being closed, vfio_pci_dma_buf_cleanup() sets priv->vdev = NULL
locklessly relative to this ioctl execution context:

vfio_pci_dma_buf_cleanup()
    list_del_init(&priv->dmabufs_elm);
    priv->vdev = NULL;
    vfio_device_put_registration(&vdev->vdev);

This plain read racing with a plain write could cause undefined behavior
under the kernel memory model and trigger KCSAN warnings. Should this be
annotated with READ_ONCE() and WRITE_ONCE() to prevent compiler reordering
or load tearing?

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