Re: [PATCH v7 6/6] vfio/pci: Add PCIe TPH SET_ST interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The rollback logic for successfully programmed steering tag entries silently fails when an entry write fails, leaving the device in an inconsistent state.
- [Medium] The `mem_type` field of `struct vfio_pci_tph_entry` is not validated when `cpu == U32_MAX`, breaking future ABI extensibility.
--

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

vfio/pci: Add PCIe TPH SET_ST interface

This commit adds the VFIO_PCI_TPH_SET_ST operation to support batch
programming of steering tag entries. It includes logic to roll back
successfully programmed entries to 0 if any entry fails, aiming to prevent
inconsistent device states.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index c328515bcaaff..5d10de546d5c6 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -1604,6 +1604,90 @@ static int vfio_pci_tph_get_st(struct vfio_pci_core_device *vdev,
> +	for (; i < st.count; i++) {
> +		/* Check reserved fields and st are zero */
> +		if (memchr_inv(&ents[i].reserved0, 0, sizeof(ents[i].reserved0)) ||
> +		    memchr_inv(&ents[i].reserved1, 0, sizeof(ents[i].reserved1)) ||
> +		    ents[i].st != 0) {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		if (ents[i].cpu == U32_MAX) {
> +			err = pcie_tph_set_st_entry(pdev, ents[i].index, 0);
> +			if (err)
> +				goto out;
> +			continue;

If userspace sets cpu to U32_MAX to clear a steering tag, does this bypass
the validation of mem_type? 

By skipping validation and allowing userspace to pass uninitialized data in
this field, could this break forward compatibility if mem_type needs to be
repurposed later for this case?

> +		}
> +
> +		if (ents[i].mem_type == VFIO_PCI_TPH_MEM_TYPE_VM) {
> +			mtype = TPH_MEM_TYPE_VM;
> +		} else if (ents[i].mem_type == VFIO_PCI_TPH_MEM_TYPE_PM) {
> +			mtype = TPH_MEM_TYPE_PM;
> +		} else {
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		err = pcie_tph_get_cpu_st(pdev, mtype, ents[i].cpu, &st_val);
> +		if (err)
> +			goto out;
> +		err = pcie_tph_set_st_entry(pdev, ents[i].index, st_val);
> +		if (err)
> +			goto out;
> +	}
> +
> +out:
> +	if (err) {
> +		/* Roll back previously programmed entries to 0 */
> +		for (j = 0; j < i; j++)
> +			pcie_tph_set_st_entry(pdev, ents[j].index, 0);

If a prior call to pcie_tph_set_st_entry() fails (e.g., due to an out of
bounds index) and disables TPH on the device internally, will these rollback
calls silently fail?

Since pcie_tph_set_st_entry() checks whether TPH is enabled and returns
-EINVAL if it is disabled, the partially programmed entries might remain on
the device without being cleared.

If the user later re-enables TPH, could the device end up using an
inconsistent steering tag table?

> +	}
> +	kvfree(ents);
> +	return err;
> +}

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