Re: [PATCH v20 14/16] vfio/pci: Add DS-ST TPH policy and extend TPH_RESOLVE capabilities

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The newly introduced UAPI capability flag `VFIO_DEVICE_TPH_CAP_RESOLVE_DMABUF_ST` is never exposed to userspace.
- [Medium] DMABUF steering tag resolution returns a steering tag based solely on policy, ignoring physical device support.

Pre-existing issues:
- [High] Unbalanced lock release in `vfio_pci_get_dmabuf_tph()` due to unconditionally calling `dma_resv_unlock()`.
--

--- Patch [14]: [PATCH v20 14/16] vfio/pci: Add DS-ST TPH policy and extend TPH_RESOLVE capabilities ---
commit e8e0f1c694e6597598ac910df3f902bedd7601ee
Author: Chengwen Feng <fengchengwen@huawei.com>
Subject: vfio/pci: Add DS-ST TPH policy and extend TPH_RESOLVE capabilities

Extends VFIO TPH policy to add DS-ST level 2 policy, relaxing write 
restrictions and extending TPH_RESOLVE logic. Adds support for CPU
memory sources and DMABUF sources returning both PH and ST under the
DS-ST policy.

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

[ ... ]

> @@ -1622,6 +1640,17 @@ static u32 vfio_pci_get_tph_capability(struct vfio_pci_core_device *vdev)
>  		if (pcie_tph_dsm_supported(vdev->pdev))
>  			flags |= VFIO_DEVICE_TPH_CAP_ST_CPU;
>  	}
> +	if (vdev->tph_policy >= VFIO_PCI_TPH_POLICY_DS_ST &&
> +		(vdev->tph_cap_virt & PCI_TPH_CAP_ST_DS)) {
> +		if (vfio_pci_get_tph_st_size(vdev) > 0) {
> +			flags |= VFIO_DEVICE_TPH_CAP_ST_NONE;
> +			flags |= VFIO_DEVICE_TPH_CAP_ST_DMABUF;
> +			if (pcie_tph_dsm_supported(vdev->pdev))
> +				flags |= VFIO_DEVICE_TPH_CAP_ST_CPU;
> +		}
> +		if (pcie_tph_dsm_supported(vdev->pdev))
> +			flags |= VFIO_DEVICE_TPH_CAP_RESOLVE_CPU_ST;
> +	}

[Severity: Medium]
The commit message and headers mention adding the capability
VFIO_DEVICE_TPH_CAP_RESOLVE_DMABUF_ST. Should it be added to the flags
bitmask here alongside VFIO_DEVICE_TPH_CAP_RESOLVE_CPU_ST? 

If it's left out, userspace might not be able to discover that the feature
is actually supported.

>  
>  	return flags;
>  }

[ ... ]

> @@ -1751,6 +1784,7 @@ static int vfio_pci_core_feature_tph_resolve(struct vfio_pci_core_device *vdev,
>  
>  	resolve.valid = 0;
>  	resolve.ph = 0;
> +	resolve.st = 0;
>  
>  	if (user_flags & VFIO_DEVICE_TPH_SRC_DMABUF) {
>  		ret = vfio_pci_get_dmabuf_tph(resolve.src, extended,

[Severity: High]
This isn't a bug introduced by this patch, but looking at
vfio_pci_get_dmabuf_tph() which is called here:

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 dma_resv_lock_interruptible() fails (for example, if it's interrupted
by a signal and returns -EINTR), will the unconditional call to
dma_resv_unlock() corrupt the underlying ww_mutex state?

> @@ -1759,8 +1793,21 @@ static int vfio_pci_core_feature_tph_resolve(struct vfio_pci_core_device *vdev,
>  			return ret;
>  		resolve.ph = ph;
>  		resolve.valid = VFIO_DEVICE_TPH_VALID_PH;
> +		if (vdev->tph_policy >= VFIO_PCI_TPH_POLICY_DS_ST) {
> +			resolve.st = tag;
> +			resolve.valid |= VFIO_DEVICE_TPH_VALID_ST;
> +		}
> +		goto out;
>  	}

[Severity: Medium]
Does this evaluation need to also check if the physical device supports the
DS-ST mode?

If the global module policy allows DS-ST but the device lacks the underlying
capability (vdev->tph_cap_virt & PCI_TPH_CAP_ST_DS), it seems this could
still return a valid steering tag to userspace for a tag it cannot actually
program.

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