diff mbox series

[v9,01/26] PCI: Fix race condition in pci_enable/disable_device()

Message ID 20201218174011.340514-2-s.miroshnichenko@yadro.com (mailing list archive)
State New
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Allow BAR movement during boot and hotplug | expand

Commit Message

Sergei Miroshnichenko Dec. 18, 2020, 5:39 p.m. UTC
This is a yet another approach to fix an old [1-2] concurrency issue, when:
 - two or more devices are being hot-added into a bridge which was
   initially empty;
 - a bridge with two or more devices is being hot-added;
 - the BIOS/bootloader/firmware doesn't pre-enable bridges during boot;

The problem is that a bridge is reported as enabled before the MEM/IO bits
are actually written to the PCI_COMMAND register, so another driver thread
starts memory requests through the not-yet-enabled bridge:

 CPU0                                        CPU1

 pci_enable_device_mem()                     pci_enable_device_mem()
   pci_enable_bridge()                         pci_enable_bridge()
     pci_is_enabled()
       return false;
     atomic_inc_return(enable_cnt)
     Start actual enabling the bridge
     ...                                         pci_is_enabled()
     ...                                           return true;
     ...                                     Start memory requests <-- FAIL
     ...
     Set the PCI_COMMAND_MEMORY bit <-- Must wait for this

Protect the pci_enable_bridge(), similarly to the previous solution from
commit 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges"),
but adding per-device mutexes.

To prevent false positives from the lockdep, use a lock_nested() with a
"depth" of a bridge within the PCI topology.

CC: Srinath Mannam <srinath.mannam@broadcom.com>
CC: Marta Rybczynska <mrybczyn@kalray.eu>
Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>

[1] https://lore.kernel.org/linux-pci/1501858648-22228-1-git-send-email-srinath.mannam@broadcom.com/T/#u
    [RFC PATCH v3] pci: Concurrency issue during pci enable bridge

[2] https://lore.kernel.org/linux-pci/744877924.5841545.1521630049567.JavaMail.zimbra@kalray.eu/T/#u
    [RFC PATCH] nvme: avoid race-conditions when enabling devices
---
 drivers/pci/pci.c   | 16 ++++++++++++++++
 drivers/pci/probe.c |  1 +
 include/linux/pci.h |  1 +
 3 files changed, 18 insertions(+)

Comments

Sergei Miroshnichenko Dec. 28, 2020, 3:37 p.m. UTC | #1
Hi,

The kbuild test robot has reported a build error on some configs,
pointing out that another #ifdef should be used. I've checked, the
DEBUG_LOCK_ALLOC seems to suit better here:

On Fri, 2020-12-18 at 20:39 +0300, Sergei Miroshnichenko wrote:
> This is a yet another approach to fix an old [1-2] concurrency issue,
> when:
>  - two or more devices are being hot-added into a bridge which was
>    initially empty;
>  - a bridge with two or more devices is being hot-added;
>  - the BIOS/bootloader/firmware doesn't pre-enable bridges during
> boot;
> 
> ...
>  
> +#ifdef CONFIG_PROVE_LOCKING

+#ifdef CONFIG_DEBUG_LOCK_ALLOC

> +static int pci_bridge_depth(struct pci_dev *dev)
> +{
> +	struct pci_dev *bridge = pci_upstream_bridge(dev);
> +
> +	if (!bridge)
> +		return 0;
> +
> +	return 1 + pci_bridge_depth(bridge);
> +}
> +#endif /* CONFIG_PROVE_LOCKING */

+#endif /* CONFIG_DEBUG_LOCK_ALLOC */

> +
>  static void pci_enable_bridge(struct pci_dev *dev)
>  {
>  	struct pci_dev *bridge;
>  	int retval;
>  
> +	mutex_lock_nested(&dev->enable_mutex, pci_bridge_depth(dev));
> +
>  	bridge = pci_upstream_bridge(dev);
>  	if (bridge)
>  		pci_enable_bridge(bridge);

Is there a proper way to send a "hotfix" for a single patch of the
series of 26, without resending them all?

Best regards,
Serge
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9fecc25d213..076b908127fe 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1844,11 +1844,25 @@  int pci_reenable_device(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_reenable_device);
 
+#ifdef CONFIG_PROVE_LOCKING
+static int pci_bridge_depth(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = pci_upstream_bridge(dev);
+
+	if (!bridge)
+		return 0;
+
+	return 1 + pci_bridge_depth(bridge);
+}
+#endif /* CONFIG_PROVE_LOCKING */
+
 static void pci_enable_bridge(struct pci_dev *dev)
 {
 	struct pci_dev *bridge;
 	int retval;
 
+	mutex_lock_nested(&dev->enable_mutex, pci_bridge_depth(dev));
+
 	bridge = pci_upstream_bridge(dev);
 	if (bridge)
 		pci_enable_bridge(bridge);
@@ -1856,6 +1870,7 @@  static void pci_enable_bridge(struct pci_dev *dev)
 	if (pci_is_enabled(dev)) {
 		if (!dev->is_busmaster)
 			pci_set_master(dev);
+		mutex_unlock(&dev->enable_mutex);
 		return;
 	}
 
@@ -1864,6 +1879,7 @@  static void pci_enable_bridge(struct pci_dev *dev)
 		pci_err(dev, "Error enabling bridge (%d), continuing\n",
 			retval);
 	pci_set_master(dev);
+	mutex_unlock(&dev->enable_mutex);
 }
 
 static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 953f15abc850..2f9631287719 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2240,6 +2240,7 @@  struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 	INIT_LIST_HEAD(&dev->bus_list);
 	dev->dev.type = &pci_dev_type;
 	dev->bus = pci_bus_get(bus);
+	mutex_init(&dev->enable_mutex);
 
 	return dev;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b32126d26997..81d54889bd51 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -455,6 +455,7 @@  struct pci_dev {
 	unsigned int	no_command_memory:1;	/* No PCI_COMMAND_MEMORY */
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
+	struct mutex	enable_mutex;
 
 	u32		saved_config_space[16]; /* Config space saved at suspend time */
 	struct hlist_head saved_cap_space;