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

[sashiko-bot@kernel.org]

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