Re: [PATCH v19 13/18] vfio/pci: Introduce VFIO_DEVICE_FEATURE_TPH family uapi for PCI TPH control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing dma_resv lock around `vfio_pci_dma_buf_get_pci_tph` in `vfio_pci_dma_buf_get_tph_by_fd`.
--

commit dbd8da4bdde63982d3f30b59ace151720f7b743f
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Introduce VFIO_DEVICE_FEATURE_TPH family uapi for PCI TPH control

Add user API definitions for PCI TPH device feature set, including three
feature IDs for global enable, resolving PH/ST, and batch programming.

> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> index c16f460c01d68..d9c5e434e586f 100644
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
> @@ -81,6 +88,26 @@ static void vfio_pci_dma_buf_unmap(struct dma_buf_attachment *attachment,
>  	kref_put(&priv->kref, vfio_pci_dma_buf_done);
>  }
>  
> +static int vfio_pci_dma_buf_get_pci_tph(struct dma_buf *dmabuf, bool extended,
> +					u16 *tag, u8 *ph)
> +{
> +	struct vfio_pci_dma_buf *priv = dmabuf->priv;
> +
> +	dma_resv_assert_held(priv->dmabuf->resv);
> +
> +	if (extended) {

[ ... ]

> @@ -106,6 +133,26 @@ static const struct dma_buf_ops vfio_pci_dmabuf_ops = {
>  	.release = vfio_pci_dma_buf_release,
>  };
>  
> +int vfio_pci_dma_buf_get_tph_by_fd(int fd, bool extended, u16 *tag, u8 *ph)
> +{
> +	struct dma_buf *dmabuf;
> +	int ret = 0;
> +
> +	dmabuf = dma_buf_get(fd);
> +	if (IS_ERR(dmabuf))
> +		return PTR_ERR(dmabuf);
> +
> +	if (dmabuf->ops != &vfio_pci_dmabuf_ops) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = vfio_pci_dma_buf_get_pci_tph(dmabuf, extended, tag, ph);
              ^^^^

[Severity: High]
Is it necessary to acquire the dmabuf->resv lock before calling
vfio_pci_dma_buf_get_pci_tph()?

Since vfio_pci_dma_buf_get_pci_tph() has an explicit dma_resv_assert_held(),
calling it here without the lock could trigger a lockdep assertion failure
and cause data races when reading the priv fields.

Should this code take the dma_resv_lock(dmabuf->resv, NULL) before calling
the inner getter to satisfy the assertion and prevent races?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702124224.57168-1-fengchengwen@huawei.com?part=13