Re: [PATCH v8 7/7] 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 error path in `vfio_pci_tph_set_st()` destructively zeroes out successfully programmed ST entries on partial failure, causing an unknown and inconsistent device state.
- [High] Positive PCIBIOS error codes from `pcie_tph_set_st_entry()` are propagated to userspace `ioctl()`, resulting in false success reporting.
--

commit c7946d420debcc101b2f2daf21858db188b20ee7
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 an error path that attempts to roll back
successfully programmed entries to 0 if any entry fails.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -1606,6 +1606,94 @@ static int vfio_pci_tph_get_st(struct vfio_pci_core_device *vdev,
>  	return err;
>  }
>  
> +static int vfio_pci_tph_set_st(struct vfio_pci_core_device *vdev,
> +			       struct vfio_device_pci_tph_op *op,
> +			       void __user *uarg)
> +{

[ ... ]

> +		if (ents[i].cpu == U32_MAX) {
> +			err = pcie_tph_set_st_entry(pdev, ents[i].index, 0);
> +			if (err)
> +				goto out;

Could this return a positive PCIBIOS error code to userspace?

pcie_tph_set_st_entry() eventually calls pci_write_config_word(), which returns
positive PCIBIOS_* error codes (like PCIBIOS_SET_FAILED). Since the VFS ioctl
handler only treats strictly negative values as errors, returning a positive
value directly will cause userspace to falsely assume the ioctl succeeded.

Should this error be converted using pcibios_err_to_errno()?

> +			continue;
> +		}
> +
> +		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);

Does this error path actually leave the device in an inconsistent state if the
entries contained valid non-zero tags before this ioctl was called?

Since the original ST values are not read or saved prior to modification,
this "rollback" destructively overwrites them with 0 rather than restoring the
true previous state.

Could we read and save the original values before modifying them, or return
an error without attempting a partial revert?

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

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