[PATCH v2 0/2] PCI/IOV: Fix deadlock when removing PF with enabled SR-IOV

[PATCH v2 0/2] PCI/IOV: Fix deadlock when removing PF with enabled SR-IOV

[Niklas Schnelle]

Hi Bjorn,

Doing additional testing for a distribution backport of commit
05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when
enabling/disabling SR-IOV") Benjamin found a hang with s390's
recover attribute. Further investigation showed this to be a deadlock by
recursively trying to take pci_rescan_remove lock when removing a PF
with enabled SR-IOV.

The issue can be reproduced on both s390 and x86_64 with:

    $ echo <NUM> > /sys/bus/pci/devices/<pf>/sriov_numvfs
    $ echo 1 > /sys/bus/pci/devices/<pf>/remove

As this seems worse than the original, hard to hit, race fixed by the
cited commit I think we first want to revert the broken fix.

Following that patch 2 attempts to fix the original issue by taking the
PCI rescan/remove lock directly before calling into the driver's
sriov_configure() callback enforcing the rule that this should only
be called with the pci_rescan_remove_lock held.

Thanks,
Niklas

Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Changes in v2:
- Collected R-b from Benjamin
- Link to v1: https://lore.kernel.org/r/20251030-revert_sriov_lock-v1-0-70f82ade426f@linux.ibm.com

---
Niklas Schnelle (2):
      Revert "PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV"
      PCI/IOV: Fix race between SR-IOV enable/disable and hotplug

 drivers/pci/iov.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
---
base-commit: dcb6fa37fd7bc9c3d2b066329b0d27dedf8becaa
change-id: 20251029-revert_sriov_lock-aef4557f360f

Best regards,
-- 
Niklas Schnelle

[PATCH v2 1/2] Revert "PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV"

[Niklas Schnelle]

This reverts commit 05703271c3cd ("PCI/IOV: Add PCI rescan-remove
locking when enabling/disabling SR-IOV") which causes a deadlock by
recursively taking pci_rescan_remove_lock when sriov_del_vfs() is called
as part of pci_stop_and_remove_bus_device(). For example with the
following sequence of commands:

$ echo <NUM> > /sys/bus/pci/devices/<pf>/sriov_numvfs
$ echo 1 > /sys/bus/pci/devices/<pf>/remove

A trimmed trace of the deadlock on a mlx5 device is as below:

 mutex_lock_nested+0x3c/0x50
 sriov_disable+0x34/0x140
 mlx5_sriov_disable+0x50/0x80 [mlx5_core]
 remove_one+0x5e/0xf0 [mlx5_core]
 pci_device_remove+0x3c/0xa0
 device_release_driver_internal+0x18e/0x280
 pci_stop_bus_device+0x82/0xa0
 pci_stop_and_remove_bus_device_locked+0x5e/0x80
 remove_store+0x72/0x90

This is not a complete fix as it restores the issue the cited commit
tried to solve.

Cc: stable@vger.kernel.org
Fixes: 05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV")
Reported-by: Benjamin Block <bblock@linux.ibm.com>
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 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 77dee43b785838d215b58db2d22088e9346e0583..ac4375954c9479b5f4a0e666b5215094fdaeefc2 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -629,18 +629,15 @@ 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);
 		if (rc)
 			goto failed;
 	}
-	pci_unlock_rescan_remove();
 	return 0;
 failed:
 	while (i--)
 		pci_iov_remove_virtfn(dev, i);
-	pci_unlock_rescan_remove();
 
 	return rc;
 }
@@ -765,10 +762,8 @@ static void sriov_del_vfs(struct pci_dev *dev)
 	struct pci_sriov *iov = dev->sriov;
 	int i;
 
-	pci_lock_rescan_remove();
 	for (i = 0; i < iov->num_VFs; i++)
 		pci_iov_remove_virtfn(dev, i);
-	pci_unlock_rescan_remove();
 }
 
 static void sriov_disable(struct pci_dev *dev)

-- 
2.48.1

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

[Niklas Schnelle]

Commit 05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when
enabling/disabling SR-IOV") tried to fix a race between the VF removal
inside sriov_del_vfs() and concurrent hot unplug by taking the PCI
rescan/remove lock in sriov_del_vfs(). Similarly the PCI rescan/remove
lock was also taken in sriov_add_vfs() to protect addition of VFs.

This approach however causes deadlock on trying to remove PFs with
SR-IOV enabled because PFs disable SR-IOV during removal and this
removal happens under the PCI rescan/remove lock. So the original fix
had to be reverted.

Instead of taking the PCI rescan/remove lock in sriov_add_vfs() and
sriov_del_vfs(), fix the race that occurs with SR-IOV enable and disable
vs hotplug higher up in the callchain by taking the lock in
sriov_numvfs_store() before calling into the driver's sriov_configure()
callback.

Cc: stable@vger.kernel.org
Fixes: 05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV")
Reported-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/pci/iov.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index ac4375954c9479b5f4a0e666b5215094fdaeefc2..c6dc1b44bf602a0b1785b684f768fcd563f5b2aa 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;
 

-- 
2.48.1

Re: [PATCH v2 0/2] PCI/IOV: Fix deadlock when removing PF with enabled SR-IOV

[Niklas Schnelle]

On Wed, 2025-11-19 at 13:34 +0100, Niklas Schnelle wrote:
> Hi Bjorn,
> 
> Doing additional testing for a distribution backport of commit
> 05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when
> enabling/disabling SR-IOV") Benjamin found a hang with s390's
> recover attribute. Further investigation showed this to be a deadlock by
> recursively trying to take pci_rescan_remove lock when removing a PF
> with enabled SR-IOV.
> 
> The issue can be reproduced on both s390 and x86_64 with:
> 
>     $ echo <NUM> > /sys/bus/pci/devices/<pf>/sriov_numvfs
>     $ echo 1 > /sys/bus/pci/devices/<pf>/remove
> 
> As this seems worse than the original, hard to hit, race fixed by the
> cited commit I think we first want to revert the broken fix.
> 
> Following that patch 2 attempts to fix the original issue by taking the
> PCI rescan/remove lock directly before calling into the driver's
> sriov_configure() callback enforcing the rule that this should only
> be called with the pci_rescan_remove_lock held.
> 
> Thanks,
> Niklas
> 
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
> Changes in v2:
> - Collected R-b from Benjamin
> - Link to v1: https://lore.kernel.org/r/20251030-revert_sriov_lock-v1-0-70f82ade426f@linux.ibm.com
> 
> ---
> Niklas Schnelle (2):
>       Revert "PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV"
>       PCI/IOV: Fix race between SR-IOV enable/disable and hotplug
> 
>  drivers/pci/iov.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> ---
> base-commit: dcb6fa37fd7bc9c3d2b066329b0d27dedf8becaa
> change-id: 20251029-revert_sriov_lock-aef4557f360f
> 
> Best regards,

Since this is really a regression without a separate report, yet this
has a description and reproducer I'm just adding this here:

#regzbot ^introduced: 05703271c3cd

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

[Gerd Bayer]

On Wed, 2025-11-19 at 13:34 +0100, Niklas Schnelle wrote:
> This reverts commit 05703271c3cd ("PCI/IOV: Add PCI rescan-remove
> locking when enabling/disabling SR-IOV") which causes a deadlock by
> recursively taking pci_rescan_remove_lock when sriov_del_vfs() is called
> as part of pci_stop_and_remove_bus_device(). For example with the
> following sequence of commands:
> 
> $ echo <NUM> > /sys/bus/pci/devices/<pf>/sriov_numvfs
> $ echo 1 > /sys/bus/pci/devices/<pf>/remove
> 
> A trimmed trace of the deadlock on a mlx5 device is as below:
> 
>  mutex_lock_nested+0x3c/0x50
>  sriov_disable+0x34/0x140
>  mlx5_sriov_disable+0x50/0x80 [mlx5_core]
>  remove_one+0x5e/0xf0 [mlx5_core]
>  pci_device_remove+0x3c/0xa0
>  device_release_driver_internal+0x18e/0x280
>  pci_stop_bus_device+0x82/0xa0
>  pci_stop_and_remove_bus_device_locked+0x5e/0x80
>  remove_store+0x72/0x90

I just hit this recursive lock issue when running "reset" instead of
"remove"


$ echo <NUM> > /sys/bus/pci/devices/<pf>/sriov_numvfs
$ echo 1 > /sys/bus/pci/devices/<pf>/reset     

> This is not a complete fix as it restores the issue the cited commit
> tried to solve.
> 
> Cc: stable@vger.kernel.org
> Fixes: 05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV")
> Reported-by: Benjamin Block <bblock@linux.ibm.com>
> 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 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 77dee43b785838d215b58db2d22088e9346e0583..ac4375954c9479b5f4a0e666b5215094fdaeefc2 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -629,18 +629,15 @@ 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);
>  		if (rc)
>  			goto failed;
>  	}
> -	pci_unlock_rescan_remove();
>  	return 0;
>  failed:
>  	while (i--)
>  		pci_iov_remove_virtfn(dev, i);
> -	pci_unlock_rescan_remove();
>  
>  	return rc;
>  }
> @@ -765,10 +762,8 @@ static void sriov_del_vfs(struct pci_dev *dev)
>  	struct pci_sriov *iov = dev->sriov;
>  	int i;
>  
> -	pci_lock_rescan_remove();
>  	for (i = 0; i < iov->num_VFs; i++)
>  		pci_iov_remove_virtfn(dev, i);
> -	pci_unlock_rescan_remove();
>  }
>  
>  static void sriov_disable(struct pci_dev *dev)

Feel free to add my
Acked-by: Gerd Bayer <gbayer@linux.ibm.com>

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

[Gerd Bayer]

On Wed, 2025-11-19 at 13:34 +0100, Niklas Schnelle wrote:
> Commit 05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when
> enabling/disabling SR-IOV") tried to fix a race between the VF removal
> inside sriov_del_vfs() and concurrent hot unplug by taking the PCI
> rescan/remove lock in sriov_del_vfs(). Similarly the PCI rescan/remove
> lock was also taken in sriov_add_vfs() to protect addition of VFs.
> 
> This approach however causes deadlock on trying to remove PFs with
> SR-IOV enabled because PFs disable SR-IOV during removal and this
> removal happens under the PCI rescan/remove lock. So the original fix
> had to be reverted.
> 
> Instead of taking the PCI rescan/remove lock in sriov_add_vfs() and
> sriov_del_vfs(), fix the race that occurs with SR-IOV enable and disable
> vs hotplug higher up in the callchain by taking the lock in
> sriov_numvfs_store() before calling into the driver's sriov_configure()
> callback.

I agree, adding the lock to sriov_numvfs_store() is adding the missing
serialization against other threads that create or remove PCI devices
without getting into the middle of implicit PCI VF device removal in
the course of a PF removal that grabbed pci_lock_rescan_remove()
already.

> 
> Cc: stable@vger.kernel.org
> Fixes: 05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV")
> Reported-by: Benjamin Block <bblock@linux.ibm.com>
> Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  drivers/pci/iov.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index ac4375954c9479b5f4a0e666b5215094fdaeefc2..c6dc1b44bf602a0b1785b684f768fcd563f5b2aa 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;
>  

This LGTM, feel free to add
Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>

Thank you!