Re: [PATCH v20 13/16] vfio/pci: Add IV-ST TPH policy and VFIO TPH_ST batch programming support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unconditional call to `dma_resv_unlock` even when `dma_resv_lock_interruptible` fails, leading to unbalanced unlock.
--

--- Patch [13]: [PATCH v20 13/16] vfio/pci: Add IV-ST TPH policy and VFIO TPH_ST batch programming support ---
commit d4cf54f638f6e3b2fed97fa978cb257c72da35af
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Add IV-ST TPH policy and VFIO TPH_ST batch programming support
    
This commit extends VFIO TPH implementation to support IV-ST mode based on
the existing NO-ST baseline. It adds the VFIO_DEVICE_FEATURE_TPH_ST feature
to enable batch ST table programming for NONE, DMABUF, and CPU sources.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 82582c8ef69da..df31f4e8766fa 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -1680,6 +1690,32 @@ static int vfio_pci_get_dmabuf_tph(int fd, bool extended, u16 *st, u8 *ph)
>  	return ret;
>  }
>  
> +static int vfio_pci_get_tph_st_tag(struct pci_dev *pdev, u32 src_bits,
> +				   u32 src_hndl, bool extended, u16 *st)
> +{
> +	u8 req_type = extended ? PCI_TPH_REQ_EXT_TPH : PCI_TPH_REQ_TPH_ONLY;
> +	int ret = 0;
> +	u8 ph;
> +
> +	if (src_bits & VFIO_DEVICE_TPH_SRC_DMABUF)
> +		ret = vfio_pci_get_dmabuf_tph(src_hndl, extended, st, &ph);

[Severity: High]
This isn't a bug introduced by this patch, but looking at the implementation
of vfio_pci_get_dmabuf_tph() which is called here, is there an unbalanced
unlock if the lock acquisition is interrupted?

In drivers/vfio/pci/vfio_pci_core.c:vfio_pci_get_dmabuf_tph():

	ret = dma_resv_lock_interruptible(dmabuf->resv, NULL);
	if (ret == 0)
		ret = dma_buf_get_pci_tph(dmabuf, extended, st, ph);
	dma_resv_unlock(dmabuf->resv);

If a signal is delivered to the thread while it is attempting to acquire the
lock, dma_resv_lock_interruptible() will return a non-zero error. The
if (ret == 0) check correctly skips the inner function, but does this code
still unconditionally execute dma_resv_unlock() and corrupt the lock state?

> +	else if (src_bits & VFIO_DEVICE_TPH_SRC_CPU_VOLATILE)
> +		ret = pcie_tph_get_cpu_st_ext(pdev, TPH_MEM_TYPE_VM,
> +					      req_type, src_hndl, st);
> +	else if (src_bits & VFIO_DEVICE_TPH_SRC_CPU_PERSISTENT)
> +		ret = pcie_tph_get_cpu_st_ext(pdev, TPH_MEM_TYPE_PM,
> +					      req_type, src_hndl, st);
> +	else if (src_bits & VFIO_DEVICE_TPH_SRC_NONE)
> +		*st = 0;
> +	else
> +		ret = -EINVAL;
> +
> +	if (ret != 0)
> +		*st = 0;
> +
> +	return ret;
> +}

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