Re: [PATCH 1/2] PCI/IOV: Add missing PCI rescan-remove locking when enabling/disabling SR-IOV

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Wed, 2025-09-24 at 10:57 -0700, Farhan Ali wrote:
> On 8/26/2025 1:52 AM, Niklas Schnelle wrote:
> > Before disabling SR-IOV via config space accesses to the parent PF,
> > sriov_disable() first removes the PCI devices representing the VFs.
> > 
> > Since commit 9d16947b7583 ("PCI: Add global pci_lock_rescan_remove()")
> > such removal operations are serialized against concurrent remove and
> > rescan using the pci_rescan_remove_lock. No such locking was ever added
> > in sriov_disable() however. In particular when commit 18f9e9d150fc
> > ("PCI/IOV: Factor out sriov_add_vfs()") factored out the PCI device
> > removal into sriov_del_vfs() there was still no locking around the
> > pci_iov_remove_virtfn() calls.
> > 
> > On s390 the lack of serialization in sriov_disable() may cause double
> > remove and list corruption with the below (amended) trace being observed:
> > 
> >   PSW:  0704c00180000000 0000000c914e4b38 (klist_put+56)
> >   GPRS: 000003800313fb48 0000000000000000 0000000100000001 0000000000000001
> >         00000000f9b520a8 0000000000000000 0000000000002fbd 00000000f4cc9480
> >         0000000000000001 0000000000000000 0000000000000000 0000000180692828
> >         00000000818e8000 000003800313fe2c 000003800313fb20 000003800313fad8
> >   #0 [3800313fb20] device_del at c9158ad5c
> >   #1 [3800313fb88] pci_remove_bus_device at c915105ba
> >   #2 [3800313fbd0] pci_iov_remove_virtfn at c9152f198
> >   #3 [3800313fc28] zpci_iov_remove_virtfn at c90fb67c0
> >   #4 [3800313fc60] zpci_bus_remove_device at c90fb6104
> >   #5 [3800313fca0] __zpci_event_availability at c90fb3dca
> >   #6 [3800313fd08] chsc_process_sei_nt0 at c918fe4a2
> >   #7 [3800313fd60] crw_collect_info at c91905822
> >   #8 [3800313fe10] kthread at c90feb390
> >   #9 [3800313fe68] __ret_from_fork at c90f6aa64
> >   #10 [3800313fe98] ret_from_fork at c9194f3f2.
> > 
> > This is because in addition to sriov_disable() removing the VFs, the
> > platform also generates hot-unplug events for the VFs. This being
> > the reverse operation to the hotplug events generated by sriov_enable()
> > and handled via pdev->no_vf_scan. And while the event processing takes
> > pci_rescan_remove_lock and checks whether the struct pci_dev still
> > exists, the lack of synchronization makes this checking racy.
> > 
> > Other races may also be possible of course though given that this lack
> > of locking persisted so long obversable races seem very rare. Even on
> > s390 the list corruption was only observed with certain devices since
> > the platform events are only triggered by the config accesses that come
> > after the removal, so as long as the removal finnished synchronously
> > they would not race. Either way the locking is missing so fix this by
> > adding it to the sriov_del_vfs() helper.
> > 
> > Just lik PCI rescan-remove locking is also missing in sriov_add_vfs()
> > including for the error case where pci_stop_ad_remove_bus_device() is
> > called without the PCI rescan-remove lock being held. Even in the non
> > error case adding new PCI devices and busses should be serialized via
> > the PCI rescan-remove lock. Add the necessary locking.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 18f9e9d150fc ("PCI/IOV: Factor out sriov_add_vfs()")
> > Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >   drivers/pci/iov.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index ac4375954c9479b5f4a0e666b5215094fdaeefc2..77dee43b785838d215b58db2d22088e9346e0583 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -629,15 +629,18 @@ static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
> >   	if (dev->no_vf_scan)
> >   		return 0;
> >   
> > +	pci_lock_rescan_remove();
> >   	for (i = 0; i < num_vfs; i++) {
> >   		rc = pci_iov_add_virtfn(dev, i);
> 
> Should we move the lock/unlock to pci_iov_add_virtfn? As that's where 
> the device is added to the bus? Similarly move the locking/unlocking to 
> pci_iov_remove_virtfn?
> 
> Thanks
> Farhan
> 
> 

I contemplated this as well. Most of the existing uses of
pci_lock/unlock_rescan_remove() are relatively coarse grained covering
e.g. the scanning of a whole bus. So I tried to keep this in line with
that such that all the VFs are added in a single critical section.

Thanks,
Niklas