[PATCH] vfio/pci: Only lock the upstream bridge if the reset may need it

[PATCH] vfio/pci: Only lock the upstream bridge if the reset may need it

[Samiullah Khawaja]

Commit 962ae6892d8b ("vfio/pci: Lock upstream bridge for
vfio_pci_core_disable()") makes vfio_pci_core_disable() trylock the
upstream bridge before resetting the device, and skips the reset if the
trylock fails.

All VFs of a PF have the same upstream bridge. Take a device with two
VFs, both used by one process. When the process is killed, both VFs can
be released at the same time. The first one takes the bridge lock and
does its FLR, the second one fails the trylock and is never reset.

The bridge lock is only needed for a bus reset, which is done through
the bridge. A VF is reset with FLR, and pci_parent_bus_reset() never
resets the bus for it because the PF and the other VFs are on the same
bus.

Add pci_reset_needs_bridge_lock(), which returns true if any reset
method enabled for the device is a bus reset, and only lock the bridge
in vfio_pci_core_disable() when it does.

Fixes: 962ae6892d8b ("vfio/pci: Lock upstream bridge for vfio_pci_core_disable()")
Cc: stable@vger.kernel.org
Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
---
Tested in QEMU with an igb PF behind a PCIe root port, bound to vfio-pci
with two VFs. Both VFs are opened through VFIO and then closed from two
threads at the same time, counting FLRs with a kprobe on pcie_flr().
Without this patch one VF missed its FLR in 50 of 50 rounds, with it
both VFs were reset in every round. I will send a VFIO selftest for this
separately.
---
 drivers/pci/pci.c                | 28 ++++++++++++++++++++++++++++
 drivers/vfio/pci/vfio_pci_core.c |  3 ++-
 include/linux/pci.h              |  1 +
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7c6f2aba8144..c44e4d4d8774 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5256,6 +5256,34 @@ int __pci_reset_function_locked(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
 
+/**
+ * pci_reset_needs_bridge_lock - check whether a reset of @dev may need the
+ * upstream bridge to be locked
+ * @dev: PCI device to check
+ *
+ * A bus reset of @dev is done through its upstream bridge, so the caller must
+ * hold the bridge lock. The other reset methods only access @dev itself.
+ *
+ * Return: true if any of the reset methods enabled for @dev is a bus reset.
+ */
+bool pci_reset_needs_bridge_lock(struct pci_dev *dev)
+{
+	int i, m;
+
+	for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
+		m = dev->reset_methods[i];
+		if (!m)
+			break;
+
+		if (pci_reset_fn_methods[m].reset_fn == pci_reset_bus_function ||
+		    pci_reset_fn_methods[m].reset_fn == cxl_reset_bus_function)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(pci_reset_needs_bridge_lock);
+
 /**
  * pci_init_reset_methods - check whether device can be safely reset
  * and store supported reset mechanisms.
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index fcbb2da860e2..ba3dfdadae39 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -777,7 +777,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 	 * overwrite the previously restored configuration information.
 	 */
 	if (vdev->reset_works) {
-		bridge = pci_upstream_bridge(pdev);
+		bridge = pci_reset_needs_bridge_lock(pdev) ?
+			 pci_upstream_bridge(pdev) : NULL;
 		if (bridge && !pci_dev_trylock(bridge))
 			goto out_restore_state;
 		if (pci_dev_trylock(pdev)) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index aa3765a20a96..cb2856f444ca 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1486,6 +1486,7 @@ void pcie_print_link_status(struct pci_dev *dev);
 int pcie_reset_flr(struct pci_dev *dev, bool probe);
 int pcie_flr(struct pci_dev *dev);
 int __pci_reset_function_locked(struct pci_dev *dev);
+bool pci_reset_needs_bridge_lock(struct pci_dev *dev);
 int pci_reset_function(struct pci_dev *dev);
 int pci_reset_function_locked(struct pci_dev *dev);
 int pci_try_reset_function(struct pci_dev *dev);

base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.56.0.611.g1030f0b940-goog

Re: [PATCH] vfio/pci: Only lock the upstream bridge if the reset may need it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] PCI: pci_reset_needs_bridge_lock() is true for VFs behind CXL
  ports
- [Medium] PCI: pci_reset_needs_bridge_lock() is true for VFs on hotplug
  slots
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [Medium] PCI: incomplete sibling check in cxl_reset_bus_function() [1]
- [Critical] vfio/pci: double free in vfio_pci_set_power_state() [2]
--

commit 5344c77aa429b69701e7ba84d963f4e63e1dda8b
Author: Samiullah Khawaja <skhawaja@google.com>

vfio/pci: Only lock the upstream bridge if the reset may need it

This commit adds pci_reset_needs_bridge_lock() to check whether a device has
a bus reset method enabled, and updates vfio_pci_core_disable() to only lock
the upstream bridge when a bus reset may be used.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7c6f2aba81443..c44e4d4d87743 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5256,6 +5256,34 @@ int __pci_reset_function_locked(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
>  
> +/**
> + * pci_reset_needs_bridge_lock - check whether a reset of @dev may need the
> + * upstream bridge to be locked
> + * @dev: PCI device to check
> + *
> + * A bus reset of @dev is done through its upstream bridge, so the caller must
> + * hold the bridge lock. The other reset methods only access @dev itself.
> + *
> + * Return: true if any of the reset methods enabled for @dev is a bus reset.
> + */
> +bool pci_reset_needs_bridge_lock(struct pci_dev *dev)
> +{
> +	int i, m;
> +
> +	for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
> +		m = dev->reset_methods[i];
> +		if (!m)
> +			break;
> +
> +		if (pci_reset_fn_methods[m].reset_fn == pci_reset_bus_function ||

[Severity: Medium]
Does pci_reset_needs_bridge_lock() still return true for VFs on hotplug
slots, leaving concurrent VF resets broken on hotplug-capable ports?

When pci_iov_add_virtfn() creates a VF on a bus with a hotplug slot (such as
pciehp with PCI_SLOT_ALL_DEVICES), pci_setup_device() assigns virtfn->slot
and virtfn->multifunction is cleared to 0 before pci_init_reset_methods()
runs.  During probing, pci_reset_bus_function() calls
pci_dev_reset_slot_function() before pci_parent_bus_reset():

drivers/pci/pci.c:pci_dev_reset_slot_function() {
	if (dev->subordinate || !dev->slot ||
	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
	    (dev->multifunction && !dev->slot->per_func_slot))
		return -ENOTTY;

	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
}

Because pci_dev_reset_slot_function() returns 0, pci_reset_bus_function is
recorded in virtfn->reset_methods[] behind pcie_reset_flr (and similarly for
a single-function PF probed before VFs are enabled).  Checking all entries
in dev->reset_methods[] causes pci_reset_needs_bridge_lock() to return true
even though __pci_reset_function_locked() will use pcie_reset_flr first, so
concurrent vfio_pci_core_disable() calls still contend on the bridge trylock
and skip the FLR.

> +		    pci_reset_fn_methods[m].reset_fn == cxl_reset_bus_function)

[Severity: Medium]
This problem wasn't introduced by this patch, but cxl_reset_bus_function()
[1] returns 0 on probe without checking whether sibling devices on the
bus prevent a bus reset:

drivers/pci/pci.c:cxl_reset_bus_function() {
	...
	dvsec = cxl_port_dvsec(bridge);
	if (!dvsec)
		return -ENOTTY;

	if (probe)
		return 0;
	...
	rc = pci_reset_bus_function(dev, probe);
}

During probing, returning 0 before calling pci_reset_bus_function() bypasses
the sibling checks in pci_dev_reset_slot_function() and
pci_parent_bus_reset().  For a CXL VF or a device sharing a bus, the PCI
core records cxl_reset_bus_function as supported, but when the reset is
later executed with probe == false, pci_reset_bus_function() fails with
-ENOTTY.

> +			return true;
> +	}

[Severity: Medium]
Will pci_reset_needs_bridge_lock() also always return true for VFs behind a
CXL-capable upstream port?

In cxl_reset_bus_function(), pci_upstream_bridge() resolves a VF to its PF's
upstream bridge via pci_physfn(), and if that bridge has a CXL Port DVSEC,
the probe path immediately returns 0.  Every VF behind such a port therefore
has cxl_reset_bus_function in dev->reset_methods[] (behind pcie_reset_flr),
so pci_reset_needs_bridge_lock() returns true and concurrent
vfio_pci_core_disable() calls on those VFs still fail pci_dev_trylock() on
the shared bridge and skip the reset.

[ ... ]

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index fcbb2da860e2a..ba3dfdadae399 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -777,7 +777,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>  	 * overwrite the previously restored configuration information.
>  	 */
>  	if (vdev->reset_works) {
> -		bridge = pci_upstream_bridge(pdev);
> +		bridge = pci_reset_needs_bridge_lock(pdev) ?
> +			 pci_upstream_bridge(pdev) : NULL;
>  		if (bridge && !pci_dev_trylock(bridge))
>  			goto out_restore_state;
>  		if (pci_dev_trylock(pdev)) {

[Severity: Critical]
This problem wasn't introduced by this patch, but vfio_pci_core_disable()
[2] calls vfio_pci_set_power_state(vdev, PCI_D0) without holding
vdev->memory_lock, which can race with another path transitioning the device
to D0 and cause a use-after-free and double free of vdev->pm_save:

CPU0                                  CPU1
vfio_pci_core_iotcl() / config write  vfio_pci_core_disable()
  down_write(&vdev->memory_lock)
  vfio_pci_set_power_state(..., D0)
                                        vfio_pci_set_power_state(..., D0)
    needs_restore = true
                                          needs_restore = true
    pci_set_power_state()
                                          pci_set_power_state()
    pci_load_and_free_saved_state(
      &vdev->pm_save)
                                          pci_load_and_free_saved_state(
                                            &vdev->pm_save)
                                            <-- use-after-free / double free

When the device is in D3hot, both threads see pdev->current_state >=
PCI_D3hot before the state is updated, so both set needs_restore = true and
call pci_load_and_free_saved_state(&vdev->pm_save) concurrently.

[1] https://sashiko.dev/#/bug/linux-f16fb8bb-75c3-4e2c-af09-9d033af32bc8
[2] https://sashiko.dev/#/bug/linux-d67caf72-8141-4778-be9b-75a34618f8f7

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112235700.2029562-1-skhawaja@google.com?part=1