Re: [PATCH v20 15/16] vfio/pci: Sync core PCI TPH software state across device lifecycle and resets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] VFIO_DEVICE_PCI_HOT_RESET fails to synchronize software TPH state for all devices in the affected reset group.
- [High] Unlocked calls to `pcie_disable_tph()` in VFIO ioctl reset paths cause TOCTOU data races with concurrent TPH configuration.
- [High] TPH virtual configuration space (`vconfig`) remains stale after reset ioctls, permanently breaking future TPH enablement.
- [High] pcie_disable_tph() accesses PCI configuration space without holding a runtime PM reference, which can race with device suspension.
--

--- Patch [15]: [PATCH v20 15/16] vfio/pci: Sync core PCI TPH software state across device lifecycle and resets ---
commit 0b983b3e1196f13bb26ca2c1d7517ec00ad08a77
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Sync core PCI TPH software state across device lifecycle and resets

Hardware resets clear PCI TPH registers, but core PCI driver software TPH
state can become stale across device open/close and reset paths. This patch
calls pcie_disable_tph() during device enable/disable and after reset ioctls
to synchronize the software tracking state with the hardware.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index ae2e0e2252a1e..c509adb76955d 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
[ ... ]
> @@ -1573,9 +1579,15 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
>  	case VFIO_DEVICE_IOEVENTFD:
>  		return vfio_pci_ioctl_ioeventfd(vdev, uarg);
>  	case VFIO_DEVICE_PCI_HOT_RESET:
> -		return vfio_pci_ioctl_pci_hot_reset(vdev, uarg);
> +		ret = vfio_pci_ioctl_pci_hot_reset(vdev, uarg);
> +		if (!ret)
> +			pcie_disable_tph(vdev->pdev);

[Severity: High]
Does this successfully clear the software state for all devices in the reset
group?

When userspace invokes VFIO_DEVICE_PCI_HOT_RESET, the hardware TPH state
is cleared for all devices in the reset group. However, only the targeted
vdev has pcie_disable_tph() called.

Will other devices in the reset group retain pdev->tph_enabled == 1,
causing pcie_enable_tph_ext() to fail with -EBUSY when userspace attempts
to re-enable TPH later?


[Severity: High]
Can pcie_disable_tph() race with device suspension here?

vfio_pci_ioctl_pci_hot_reset() acquires PM runtime references for all devices
but drops them via pm_runtime_put() before returning. If the device is
configured for runtime PM, it may immediately begin suspending.

Since pcie_disable_tph() performs an unlocked pci_write_config_dword(),
does it risk accessing the PCI configuration space of a suspended device,
potentially leading to PCIe Completion Timeouts or dropped writes?

> +		return ret;
>  	case VFIO_DEVICE_RESET:
> -		return vfio_pci_ioctl_reset(vdev, uarg);
> +		ret = vfio_pci_ioctl_reset(vdev, uarg);
> +		if (!ret)
> +			pcie_disable_tph(vdev->pdev);

[Severity: High]
Is there a data race with concurrent TPH configuration?

The reset handlers drop dev_set->lock and vdev->memory_lock before
returning, allowing pcie_disable_tph() to run locklessly.

If a concurrent userspace thread writes to the TPH virtual config space, it
takes memory_lock and calls pcie_enable_tph_ext(). Can these two threads
concurrently modify PCI_TPH_CTRL and pdev->tph_enabled, corrupting the
tracking state?


[Severity: High]
Does this leave the virtual configuration space (vconfig) stale?

While pcie_disable_tph() clears the hardware state and pdev->tph_enabled,
it doesn't update vdev->vconfig.

If the guest OS later rewrites the config space to re-enable TPH,
vfio_tph_config_write() will see:

    new_ctrl = le32_to_cpu(*(__le32 *)&vdev->vconfig[start + PCI_TPH_CTRL]);
    if (new_ctrl == org_ctrl)
        goto out;

Since vconfig still contains the enabled state from before the reset, won't
this treat the write as a no-op, leaving TPH permanently disabled in hardware?

> +		return ret;
>  	case VFIO_DEVICE_SET_IRQS:
>  		return vfio_pci_ioctl_set_irqs(vdev, uarg);
>  	default:

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