Re: [PATCH v6 4/5] vfio/pci: implement get_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:
- [Medium] Unconditional validation of the flag-gated `ph` parameter causes valid requests clearing metadata to be rejected.
- [Low] Mixing `goto`-based error handling with scope-based cleanup helpers (`scoped_guard`) in the same function.
- [Low] Unprotected read of `priv->vdev` races with concurrent write to `NULL`.
--

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

vfio/pci: implement get_tph and DMA_BUF_TPH feature

This commit implements the dma-buf get_tph callback for vfio-pci-exported
dma-bufs. It also adds the VFIO_DEVICE_FEATURE_DMA_BUF_TPH feature to allow
userspace to attach TPH metadata to a dma-buf.

> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> index 1a177ce7de546..f1bc3733d3db5 100644
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
[ ... ]
> @@ -327,12 +358,71 @@ 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)
> +{
> +	struct vfio_device_feature_dma_buf_tph set_tph;
> +	struct vfio_pci_dma_buf *priv;
> +	struct dma_buf *dmabuf;
> +	int ret;
> +
> +	if (!pcie_tph_supported(vdev->pdev))
> +		return -EOPNOTSUPP;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> +				 sizeof(set_tph));
> +	if (ret != 1)
> +		return ret;
> +
> +	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;
> +
> +	/* PCIe TLP Processing Hint is a 2-bit field. */
> +	if (set_tph.ph & ~0x3)
> +		return -EINVAL;

[Severity: Medium]
Will this unconditionally reject requests clearing the metadata?

The documentation mentions that setting flags to 0 clears previously published
metadata. However, because this check is evaluated unconditionally, userspace
leaving the ph field uninitialized (containing stack garbage) when passing
flags = 0 will cause the kernel to erroneously reject a valid clear request
with -EINVAL.

Should this validation only occur if the relevant flags are set?

> +
> +	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]
Does this unprotected read of priv->vdev race with concurrent unbound operations?

In vfio_pci_dma_buf_cleanup(), there is a concurrent write setting 
priv->vdev = NULL under the vdev->memory_lock semaphore.

Since this read occurs without locks, can it race with the concurrent write
and cause KCSAN warnings or load tearing? Would using READ_ONCE() here and
WRITE_ONCE() in vfio_pci_dma_buf_cleanup() help prevent this?

> +		ret = -EINVAL;
> +		goto out_put;
> +	}
> +
> +	scoped_guard(mutex, &priv->tph_lock) {
> +		priv->tph_st = set_tph.steering_tag;
> +		priv->tph_st_ext = set_tph.steering_tag_ext;
> +		priv->tph_ph = set_tph.ph;
> +		priv->tph_st_valid = !!(set_tph.flags & VFIO_DMA_BUF_TPH_ST);
> +		priv->tph_st_ext_valid =
> +			!!(set_tph.flags & VFIO_DMA_BUF_TPH_ST_EXT);
> +	}
> +	ret = 0;
> +
> +out_put:

[Severity: Low]
Is it okay to mix goto-based error handling and scope-based cleanup helpers in
this function?

The kernel cleanup guidelines discourage mixing goto labels for cleanup (like
goto out_put here) and scoped_guard() in the same function to avoid confusing
ownership semantics.

Would it be better to either use a __free() wrapper for the dmabuf or stick to
traditional mutex_lock()/mutex_unlock() here?

> +	dma_buf_put(dmabuf);
> +	return ret;
> +}

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