Re: [PATCH v4 1/1] PCI/IOV: Add reentrant locking in sriov_add_vfs/sriov_del_vfs for complete serialization

[Manivannan Sadhasivam]

On Sat, Feb 28, 2026 at 02:01:40PM +0200, Ionut Nechita (Wind River) wrote:
> From: Ionut Nechita <ionut.nechita@xxxxxxxxxxxxx>
> 
> After reverting commit 05703271c3cd ("PCI/IOV: Add PCI rescan-remove
> locking when enabling/disabling SR-IOV") and moving the lock to
> sriov_numvfs_store(), the path through driver .remove() (e.g. rmmod,
> or manual unbind) that calls pci_disable_sriov() directly remains
> unprotected against concurrent hotplug events. This affects any SR-IOV
> capable driver that calls pci_disable_sriov() from its .remove()
> callback (i40e, ice, mlx5, bnxt, etc.).
> 
> On s390, platform-generated hot-unplug events for VFs can race with
> sriov_del_vfs() when a PF driver is being unloaded. The platform event
> handler takes pci_rescan_remove_lock, but sriov_del_vfs() does not,
> leading to double removal and list corruption.
> 
> We cannot use a plain mutex_lock() here because sriov_del_vfs() may also
> be called from paths that already hold pci_rescan_remove_lock (e.g.
> remove_store -> pci_stop_and_remove_bus_device_locked, or
> sriov_numvfs_store with the lock taken by the previous patch). Using
> mutex_lock() in those cases would deadlock.
> 
> Instead, introduce owner tracking for pci_rescan_remove_lock via a new
> pci_lock_rescan_remove_reentrant() helper. This function checks if the
> current task already holds the lock:
>  - If the lock is not held: acquires it and returns true, providing
>    full serialization against concurrent hotplug events (including
>    platform-generated events on s390).
>  - If the lock is already held by the current task (reentrant call from
>    remove_store or sriov_numvfs_store paths): returns false without
>    re-acquiring, avoiding deadlock while the caller already provides
>    the necessary serialization.
>  - If the lock is held by another task (concurrent hotplug): blocks
>    until the lock is released, then acquires it, providing complete
>    serialization. This is the key improvement over a trylock approach.
> 

Just curious. Why can't you use mutex_trylock() here?

- Mani

> A matching pci_unlock_rescan_remove_reentrant() helper takes the return
> value of the lock function as argument, so callers don't need to
> open-code the conditional unlock.
> 
> The "reentrant" naming is chosen to avoid confusion with existing
> mutex_lock_nested() which is a lockdep annotation concept, not actual
> reentrant locking.
> 
> Note: owner-tracking patterns for reentrant lock behavior exist elsewhere
> in the kernel, for example in the regulator core (drivers/regulator/core.c)
> with rdev->mutex_owner, and in the PPP subsystem (drivers/net/ppp/
> ppp_generic.c) with xmit_recursion->owner.
> 
> The declarations are placed in include/linux/pci.h alongside the existing
> pci_lock_rescan_remove()/pci_unlock_rescan_remove() declarations to
> maintain API consistency and allow use by external drivers if needed.
> 
> Fixes: 18f9e9d150fc ("PCI/IOV: Factor out sriov_add_vfs()")
> Cc: stable@xxxxxxxxxxxxxxx
> Tested-by: Dragos Tatulea <dtatulea@xxxxxxxxxx>
> Reviewed-by: Benjamin Block <bblock@xxxxxxxxxxxxx>
> Tested-by: Benjamin Block <bblock@xxxxxxxxxxxxx>
> Signed-off-by: Ionut Nechita <ionut_n2001@xxxxxxxxx>
> Signed-off-by: Ionut Nechita <ionut.nechita@xxxxxxxxxxxxx>
> ---
> Changes in v4:
>  - Rebased on linux-next (next-20260227)
>  - Declared pci_rescan_remove_owner as const pointer
>    (const struct task_struct *) to make clear it is not meant to
>    modify the task (Benjamin Block)
>  - Added Reviewed-by and Tested-by from Benjamin Block (IBM)
> 
> Changes in v3:
>  - Rebased on linux-next (next-20260225)
>  - Added Tested-by from Dragos Tatulea (NVIDIA)
>  - No code changes from v2
> 
> Changes in v2:
>  - Renamed from pci_lock_rescan_remove_nested() to
>    pci_lock_rescan_remove_reentrant() to avoid confusion with
>    mutex_lock_nested() lockdep annotations (Benjamin Block)
>  - Added pci_unlock_rescan_remove_reentrant(const bool locked) helper
>    to avoid open-coding conditional unlock at each call site
>    (Benjamin Block)
>  - Moved declarations from drivers/pci/pci.h to include/linux/pci.h
>    alongside existing lock/unlock declarations (Benjamin Block)
>  - Simplified callers: removed negation of return value and manual
>    conditional unlock in favor of the paired lock/unlock helpers
> 
>  drivers/pci/iov.c   |  7 +++++++
>  drivers/pci/probe.c | 19 +++++++++++++++++++
>  include/linux/pci.h |  2 ++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 91ac4e37ecb9c..adbe4ecc587c9 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -629,19 +629,23 @@ static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
>  {
>  	unsigned int i;
>  	int rc;
> +	bool locked;
>  
>  	if (dev->no_vf_scan)
>  		return 0;
>  
> +	locked = pci_lock_rescan_remove_reentrant();
>  	for (i = 0; i < num_vfs; i++) {
>  		rc = pci_iov_add_virtfn(dev, i);
>  		if (rc)
>  			goto failed;
>  	}
> +	pci_unlock_rescan_remove_reentrant(locked);
>  	return 0;
>  failed:
>  	while (i--)
>  		pci_iov_remove_virtfn(dev, i);
> +	pci_unlock_rescan_remove_reentrant(locked);
>  
>  	return rc;
>  }
> @@ -764,10 +768,13 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  static void sriov_del_vfs(struct pci_dev *dev)
>  {
>  	struct pci_sriov *iov = dev->sriov;
> +	bool locked;
>  	int i;
>  
> +	locked = pci_lock_rescan_remove_reentrant();
>  	for (i = 0; i < iov->num_VFs; i++)
>  		pci_iov_remove_virtfn(dev, i);
> +	pci_unlock_rescan_remove_reentrant(locked);
>  }
>  
>  static void sriov_disable(struct pci_dev *dev)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index bccc7a4bdd794..c7f672eac0698 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3509,19 +3509,38 @@ EXPORT_SYMBOL_GPL(pci_rescan_bus);
>   * routines should always be executed under this mutex.
>   */
>  DEFINE_MUTEX(pci_rescan_remove_lock);
> +static const struct task_struct *pci_rescan_remove_owner;
>  
>  void pci_lock_rescan_remove(void)
>  {
>  	mutex_lock(&pci_rescan_remove_lock);
> +	pci_rescan_remove_owner = current;
>  }
>  EXPORT_SYMBOL_GPL(pci_lock_rescan_remove);
>  
>  void pci_unlock_rescan_remove(void)
>  {
> +	pci_rescan_remove_owner = NULL;
>  	mutex_unlock(&pci_rescan_remove_lock);
>  }
>  EXPORT_SYMBOL_GPL(pci_unlock_rescan_remove);
>  
> +bool pci_lock_rescan_remove_reentrant(void)
> +{
> +	if (pci_rescan_remove_owner == current)
> +		return false;
> +	pci_lock_rescan_remove();
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(pci_lock_rescan_remove_reentrant);
> +
> +void pci_unlock_rescan_remove_reentrant(const bool locked)
> +{
> +	if (locked)
> +		pci_unlock_rescan_remove();
> +}
> +EXPORT_SYMBOL_GPL(pci_unlock_rescan_remove_reentrant);
> +
>  static int __init pci_sort_bf_cmp(const struct device *d_a,
>  				  const struct device *d_b)
>  {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1c270f1d51230..080950f0bab33 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1535,6 +1535,8 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev);
>  unsigned int pci_rescan_bus(struct pci_bus *bus);
>  void pci_lock_rescan_remove(void);
>  void pci_unlock_rescan_remove(void);
> +bool pci_lock_rescan_remove_reentrant(void);
> +void pci_unlock_rescan_remove_reentrant(const bool locked);
>  
>  /* Vital Product Data routines */
>  ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
> -- 
> 2.53.0
> 
> 

-- 
மணிவண்ணன் சதாசிவம்