Re: [PATCH v3 2/2] PCI/IOV: Fix race between SR-IOV enable/disable and hotplug

[Benjamin Block]

On Mon, Mar 16, 2026 at 06:57:53PM -0700, Guenter Roeck wrote:
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index 7de5b18647beb69127ba11234fb9f1dec9b50540..4a659c34935e116dd6d0b4ce42ed12a1ba9418d1 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -495,7 +495,9 @@ static ssize_t sriov_numvfs_store(struct device *dev,
> >  
> >  	if (num_vfs == 0) {
> >  		/* disable VFs */
> > +		pci_lock_rescan_remove();
> >  		ret = pdev->driver->sriov_configure(pdev, 0);
> > +		pci_unlock_rescan_remove();
> >  		goto exit;
> >  	}
> >  
> > @@ -507,7 +509,9 @@ static ssize_t sriov_numvfs_store(struct device *dev,
> >  		goto exit;
> >  	}
> >  
> > +	pci_lock_rescan_remove();
> >  	ret = pdev->driver->sriov_configure(pdev, num_vfs);
> > +	pci_unlock_rescan_remove();
> >  	if (ret < 0)
> >  		goto exit;
> >  
>
> Google's experimental AI review agent provided the following feedback
> on this patch.
> 
>   Could this introduce an AB-BA deadlock between the device lock and the
>   rescan/remove lock?
> 
>   Earlier in sriov_numvfs_store(), device_lock(&pdev->dev) is acquired. The
>   patch then attempts to acquire pci_lock_rescan_remove() while holding the
>   device lock.
> 
>   However, during a hotplug removal of the PF (for example, via sysfs),
>   remove_store() first acquires pci_lock_rescan_remove() and subsequently
>   calls pci_stop_and_remove_bus_device_locked(). That path eventually calls
>   device_release_driver(), which attempts to acquire device_lock(&pdev->dev).
> 
>   If sriov_numvfs_store() and a concurrent removal of the PF race, it appears
>   they could deadlock waiting on each other's locks.
> 
> The actual call sequence (at least in v6.12.y, where this patch was
> backported to) is as follows.
>   remove_store()
>   -> pci_stop_and_remove_bus_device_locked()
>      -> pci_lock_rescan_remove()
>         -> pci_stop_and_remove_bus_device()
> 	   -> pci_stop_bus_device()
> 	   -> pci_remove_bus_device()
> 	      -> pci_remove_bus()
> 	         -> device_unregister()
> 		    -> device_del()
> 		       -> device_lock()
> 
> I don't claim to fully understand the code, but the AI does seem to have a
> point. Please let me know if the AI analysis is correct or if it misses
> something.

Ugh. Well. That sucks. This lock is a sheer endless well of joy.
No, well, I think the AI is correct.

We've since discussed to move away from that patch again, or rather,
improve it further by applying this in top:
    https://lore.kernel.org/linux-pci/20260310074303.17480-2-ionut.nechita@xxxxxxxxxxxxx/

Because it improves some scenarios, such as driver core unbinds.
But looking at it from this angle, it suffers from the same AB-BA cyclic
deadlock.

  remove_store()
    |
    +- pci_stop_and_remove_bus_device_locked()
        |
        +- takes: pci_rescan_remove_lock                    # XXX
        |
        +- pci_stop_and_remove_bus_device()
            |
            +- pci_stop_bus_device()
                |
                +- pci_stop_dev()
                    |
                    +- device_release_driver()
                        |
                        +- device_release_driver_internal()
                            |
                            +- __device_driver_lock()
                                |
                                +- __device_driver_lock() - takes: pdev->dev

  unbind_store()
    |
    +- device_driver_detach()
        |
        +- device_release_driver_internal()
            |
            +- __device_driver_lock() - takes: pdev->dev    # XXX
            |
            +- __device_release_driver()
                |
                +- device_remove()
                    |
                    +- pci_device_remove()
                        |
                        +- vfio_pci_remove()
                            |
                            +- vfio_pci_core_sriov_configure()
                                |
                                +- pci_disable_sriov()
                                    |
                                    +- sriov_disable()
                                        |
                                        +- sriov_del_vfs()
                                            |
                                            +- takes: pci_rescan_remove_lock

And there is no way I can see how we can reverse the lock order in the
unbind_store() case, since everything above pci_device_remove() is owned
by the driver core itself. I don't see a way for us to put a hook in
there to take `pci_rescan_remove_lock`.

It's similar to what I'm trying to fix in:
    https://lore.kernel.org/linux-pci/354b9e4a54ced67f3c89df198041df19434fe4c8.1773235561.git.bblock@xxxxxxxxxxxxx/
Taking `pci_rescan_remove_lock` inside the release functions is fraught
with traps, especially with SR-IOV in the mix.

One quick idea: can we somehow unbind the device from any device driver
in remove_store() before calling
pci_stop_and_remove_bus_device_locked()?  That way we would not have any
SR-IOV functions attached anymore at the point where we remove the PF,
since the DD are expected to clean them up.

-- 
Best Regards und Beste Grüße, Benjamin Block
               PGP KeyID: 9610 2BB8 2E17 6F65 2362  6DF2 46E0 4E05 67A3 2E9E