Re: [PATCH v8 4/7] vfio/pci: Add PCIe TPH interface with capability query

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] UAPI Backwards Compatibility Breakage and Buffer Overflow Risk
- [High] Missing memory_lock Causes Hardware State Races During Reset
- [Medium] UAPI Bloat / Bypassing VFIO_DEVICE_FEATURE Framework
- [Medium] Incorrect Module Parameter Scope Limits Vendor Drivers
--

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

vfio/pci: Add PCIe TPH interface with capability query
    
Add VFIO_DEVICE_PCI_TPH IOCTL to allow userspace to query device TPH
capabilities, supported modes, and steering tag table information.

> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 0c771064c0b84..40bf5aa9fd0be 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -60,6 +60,12 @@ static bool disable_denylist;
>  module_param(disable_denylist, bool, 0444);
>  MODULE_PARM_DESC(disable_denylist, "Disable use of device denylist. Disabling the denylist allows binding to devices with known errata that may lead to exploitable stability or security issues when accessed by untrusted users.");
>  
> +#ifdef CONFIG_PCIE_TPH
> +static bool enable_unsafe_tph_ds_mode;
> +module_param(enable_unsafe_tph_ds_mode, bool, 0444);

Vendor-specific VFIO drivers (such as mlx5_vfio_pci or hisi_vfio_pci) load
the shared vfio_pci_core.ko library directly and do not depend on vfio_pci.ko.

If users of these vendor drivers want to enable unsafe TPH DS mode, they
have no way to do so unless they explicitly load the generic vfio_pci driver,
which alters the core state for all bound devices.

Should this parameter reside directly in vfio_pci_core.c instead?

[ ... ]

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f8d093aacf8a..0e97b128fd637 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1461,6 +1463,54 @@ static int vfio_pci_ioctl_ioeventfd(struct vfio_pci_core_device *vdev,
>  				  ioeventfd.fd);
>  }
>  
> +static int vfio_pci_tph_get_cap(struct vfio_pci_core_device *vdev,
> +				struct vfio_device_pci_tph_op *op,
> +				void __user *uarg)
> +{
> +	struct pci_dev *pdev = vdev->pdev;
> +	struct vfio_pci_tph_cap cap = {0};
> +	u8 mode;
> +
> +	if (op->argsz < offsetofend(struct vfio_device_pci_tph_op, cap))
> +		return -EINVAL;

If struct vfio_pci_tph_cap is extended in future kernel releases, offsetofend()
will increase. Will new kernels reject valid requests from older userspace
binaries with -EINVAL, breaking backwards compatibility?

> +
> +	mode = pcie_tph_get_st_modes(pdev);
> +	/* Hide unsafe device-specific (DS) mode by default */
> +	if (!enable_unsafe_tph_ds_mode)
> +		mode &= ~PCI_TPH_CAP_ST_DS;
> +	if (mode == 0 || mode == PCI_TPH_CAP_ST_NS)
> +		return -EOPNOTSUPP;
> +
> +	if (mode & PCI_TPH_CAP_ST_IV)
> +		cap.supported_modes |= VFIO_PCI_TPH_MODE_IV;
> +	if (mode & PCI_TPH_CAP_ST_DS)
> +		cap.supported_modes |= VFIO_PCI_TPH_MODE_DS;
> +	cap.st_table_sz = pcie_tph_get_st_table_size(pdev);
> +
> +	if (copy_to_user(uarg, &cap, sizeof(cap)))

If the capability structure grows in the future, will the kernel write beyond
the bounds of an older userspace application's allocated buffer?

> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int vfio_pci_ioctl_tph(struct vfio_pci_core_device *vdev,
> +			      void __user *uarg)
> +{
> +	struct vfio_device_pci_tph_op op = {0};
> +	size_t minsz = sizeof(op.argsz) + sizeof(op.op);
> +
> +	if (copy_from_user(&op, uarg, minsz))
> +		return -EFAULT;
> +
> +	switch (op.op) {
> +	case VFIO_PCI_TPH_GET_CAP:
> +		return vfio_pci_tph_get_cap(vdev, &op, uarg + minsz);

This handler does not acquire down_read(&vdev->memory_lock). Does this allow
TPH IOCTLs to run concurrently with a PCI hot reset
(VFIO_DEVICE_PCI_HOT_RESET) which takes the write lock via
vfio_pci_zap_and_down_write_memory_lock()?

Concurrent execution could allow vfio_pci_tph_get_cap() to read configuration
space while the secondary bus is in reset, yielding bogus data.

More importantly, later in this series when vfio_pci_tph_set_st() is added to
write directly to the MSI-X table via MMIO, performing MMIO writes to a device
actively undergoing reset can cause PCI Master Aborts. On certain platforms,
this can trigger AER panics or EEH isolation.

[ ... ]
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 5de618a3a5eee..81da2bd0c21b0 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
[ ... ]
> +/**
> + * VFIO_DEVICE_PCI_TPH - _IO(VFIO_TYPE, VFIO_BASE + 22)
> + *
> + * IOCTL for managing PCIe TLP Processing Hints (TPH) on
> + * a VFIO-assigned PCI device. Provides operations to query
> + * device capabilities, enable/disable TPH, retrieve CPU's
> + * steering tags, and program steering tag tables.
> + *
> + * Return: 0 on success, negative errno on failure.
> + *         -EOPNOTSUPP: Operation not supported
> + *         -ENODEV: Device or required functionality not present
> + *         -EINVAL: Invalid argument or TPH not supported
> + */
> +#define VFIO_DEVICE_PCI_TPH	_IO(VFIO_TYPE, VFIO_BASE + 22)

This introduces a new top-level IOCTL to manage PCIe TPH capabilities instead
of using the standard VFIO_DEVICE_FEATURE IOCTL.

Could this be implemented using VFIO_DEVICE_FEATURE, which was explicitly
designed to act as a unified multiplexer for querying and setting new device
features, to avoid unnecessarily bloating the UAPI namespace and duplicating
boilerplate for sizing and feature discovery?

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