[RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

[RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

[YuanShang]

An upstream bridge is shared by every function below it, so taking it
with pci_dev_trylock() makes the release-time reset fail whenever two
functions under the same bridge are released at the same time.  One
wins the trylock and resets; the others take the goto and are handed
back to the next user without ever being reset.  Nothing is logged.

This is easy to hit with SR-IOV, where all VFs of a device sit behind
the same bridge.  Releasing just two VFs concurrently is already
enough; which one wins the trylock is random between runs.

Take the bridge lock blocking instead.  The lock order (bridge, then
device) matches pci_bus_lock(), which takes both blocking.  The
trylock on the device itself is left alone.

Concurrent releases now serialise their resets, so a teardown of many
functions under one bridge pays one FLR settling time per function.

Fixes: 962ae6892d8b ("vfio/pci: Lock upstream bridge for vfio_pci_core_disable()")
Cc: stable@vger.kernel.org
Signed-off-by: YuanShang <YuanShang.Mao@amd.com>
---
Hi Alex, Anthony,

Sending this as an RFC because I would like to know whether the approach
is acceptable before going further.

I hit this with SR-IOV: all VFs of a device share one upstream bridge, so
when several VFs are released at the same time only one of them wins the
trylock and gets reset.  The rest silently skip the reset.  Two concurrent
releases are enough to reproduce it here.

My question is whether taking the bridge lock blocking is safe.  I have
run this without problems, but I do not fully understand what the trylock
on the bridge was protecting against -- 962ae6892d8b added it to silence
the unlocked-SBR warning, and it is not obvious to me whether blocking
there can deadlock.  If there is a path I am missing, I would rather hear
it now.

An alternative would be to keep the trylock but at least log when the
reset is skipped, since today it is completely silent.

Not yet tested under lockdep; I am setting that up.

 drivers/vfio/pci/vfio_pci_core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 362c375a0579..8966a5bea406 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -790,8 +790,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 	 */
 	if (vdev->reset_works) {
 		bridge = pci_upstream_bridge(pdev);
-		if (bridge && !pci_dev_trylock(bridge))
-			goto out_restore_state;
+		if (bridge)
+			pci_dev_lock(bridge);
 		if (pci_dev_trylock(pdev)) {
 			if (!__pci_reset_function_locked(pdev))
 				vdev->needs_reset = false;
@@ -801,7 +801,6 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 			pci_dev_unlock(bridge);
 	}
 
-out_restore_state:
 	pci_restore_state(pdev);
 out:
 	pci_disable_device(pdev);
-- 
2.25.1

Re: [RFC PATCH] vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing pci_dev_trylock(bridge) with a blocking pci_dev_lock(bridge) introduces a 3-thread AB-BA-CA deadlock.
--

commit 170de39cb1fd30df6e523a31c390c9881837b836
Author: YuanShang <YuanShang.Mao@amd.com>

vfio/pci: Block for the upstream bridge lock in vfio_pci_core_disable()

This commit changes vfio_pci_core_disable() to take the upstream bridge lock
blocking instead of with a trylock. This is intended to serialize concurrent
resets of virtual functions under the same bridge.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 6757054e9d875..01f0238057c76 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -791,8 +791,8 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>  	 */
>  	if (vdev->reset_works) {
>  		bridge = pci_upstream_bridge(pdev);
> -		if (bridge && !pci_dev_trylock(bridge))
> -			goto out_restore_state;
> +		if (bridge)
> +			pci_dev_lock(bridge);

[Severity: High]
Does this blocking lock create a deadlock cycle with PCI device removal and
bus reset operations?

Replacing pci_dev_trylock(bridge) with a blocking pci_dev_lock(bridge)
introduces a 3-thread AB-BA-CA deadlock. This can freeze the involved threads
and lock the PCI bus subsystem, leading to a hung task and an unusable system.

Could this sequence occur?

Thread 1 (driver unbind): Triggers driver removal. device_release_driver()
acquires device_lock(&pdev->dev) and calls vfio_unregister_group_dev(),
which blocks waiting for the device's open file descriptors to be closed
(waiting for device->comp completion).

Thread 2 (PCI bus reset): A concurrent bus reset calls pci_bus_lock(),
which acquires pci_dev_lock(bridge) and then iterates children to acquire
pci_dev_lock(pdev) (which uses device_lock(&pdev->dev)). It blocks waiting
for Thread 1.

Thread 3 (userspace close): Userspace closes the VFIO device FD. In
vfio_device_fops_release(), the driver calls vfio_pci_core_disable(), which
now blocks on pci_dev_lock(bridge) waiting for Thread 2.

Because Thread 3 is blocked, it never reaches vfio_device_put_registration()
to signal device->comp, causing Thread 1 to wait forever.

This race condition seems triggerable when an unprivileged user closes an
open VFIO FD concurrently with a root-initiated driver unbind and/or PCI
bus reset.

>  		if (pci_dev_trylock(pdev)) {
>  			if (!__pci_reset_function_locked(pdev))
>  				vdev->needs_reset = false;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908080946.2235849-1-YuanShang.Mao@amd.com?part=1