| Message ID | 08bc40a6af3e614e97a78fbaab688bfcd14520ac.camel@kernel.crashing.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [RFC] pci: Proof of concept at fixing pci_enable_device/bridge races | expand |
On Thu, Aug 16, 2018 at 07:50:13AM +1000, Benjamin Herrenschmidt wrote: > (Resent with lkml on copy) > > [Note: This isn't meant to be merged, it need splitting at the very > least, see below] > > This is something I cooked up quickly today to test if that would fix > my problems with large number of switch and NVME devices on POWER. > Is that a problem that can be reproduced with a qemu setup ? Thanks, Guenter
On Wed, 2018-08-15 at 15:40 -0700, Guenter Roeck wrote: > On Thu, Aug 16, 2018 at 07:50:13AM +1000, Benjamin Herrenschmidt wrote: > > (Resent with lkml on copy) > > > > [Note: This isn't meant to be merged, it need splitting at the very > > least, see below] > > > > This is something I cooked up quickly today to test if that would fix > > my problems with large number of switch and NVME devices on POWER. > > > > Is that a problem that can be reproduced with a qemu setup ? With difficulty... mt-tcg might help, but you need a rather large systems to reproduce it. My repro-case is a 2 socket POWER9 system (about 40 cores off the top of my mind, so 160 threads) with 72 NVME devices underneath a tree of switches (I don't have the system at hand today to check how many). It's possible to observe it I suppose on a smaller system (in theory a single bridge with 2 devices is enough) but in practice the timing is extremely hard to hit. You need a combination of: - The bridges come up disabled (which is the case when Linux does the resource assignment, such as on POWER but not on x86 unless it's hotplug) - The nvme devices try to enable them simultaneously Also the resulting error is a UR, I don't know how well qemu models that. On the above system, I get usually *one* device failing due to the race out of 72, and not on every boot. However, the bug is known (see Bjorn's reply to the other thread) "Re: PCIe enable device races (Was: [PATCH v3] PCI: Data corruption happening due to race condition)" on linux-pci, so I'm not the only one with a repro-case around. Cheers, Ben.
[+cc Srinath, Guenter, Jens, Lukas, Konstantin, Marta, Pierre-Yves] On Thu, Aug 16, 2018 at 07:50:13AM +1000, Benjamin Herrenschmidt wrote: > [Note: This isn't meant to be merged, it need splitting at the very > least, see below] > > This is something I cooked up quickly today to test if that would fix > my problems with large number of switch and NVME devices on POWER. > > So far it does... > > The issue (as discussed in the Re: PCIe enable device races thread) is > that pci_enable_device and related functions along with pci_set_master > and pci_enable_bridge are fundamentally racy. > > There is no lockign protecting the state of the device in pci_dev and > if multiple devices under the same bridge try to enable it simultaneously > one some of them will potentially start accessing it before it has actually > been enabled. > > Now there are a LOT more fields in pci_dev that aren't covered by any > form of locking. Most of the PCI core relies on the assumption that only a single thread touches a device at a time. This is generally true of the core during enumeration because enumeration is single-threaded. It's generally true in normal driver operation because the core shouldn't touch a device after a driver claims it. But there are several exceptions, and I think we need to understand those scenarios before applying locks willy-nilly. One big exception is that enabling device A may also touch an upstream bridge B. That causes things like your issue and Srinath's issue where drivers simultaneously enabling two devices below the same bridge corrupt the bridge's state [1]. Marta reported essentially the same issue [2]. Hari's issue [3] seems related to a race between a driver work queue and the core enumerating the device. I should have pushed harder to understand this; I feel like we papered over the immediate problem without clearing up the larger issue of why the core enumeration path (pci_bus_add_device()) is running at the same time as a driver. DPC/AER error handling adds more cases where the core potentially accesses devices asynchronously to the driver. User-mode sysfs controls like ASPM are also asynchronous to drivers. Even setpci is a potential issue, though I don't know how far we want to go to protect it. I think we should make setpci taint the kernel anyway. It might be nice if we had some sort of writeup of the locking strategy as a whole. [1] https://lkml.kernel.org/r/1501858648-22228-1-git-send-email-srinath.mannam@broadcom.com [2] https://lkml.kernel.org/r/744877924.5841545.1521630049567.JavaMail.zimbra@kalray.eu [3] https://lkml.kernel.org/r/1530608741-30664-2-git-send-email-hari.vyas@broadcom.com
On Thu, 2018-08-16 at 22:07 -0500, Bjorn Helgaas wrote: > [+cc Srinath, Guenter, Jens, Lukas, Konstantin, Marta, Pierre-Yves] > > On Thu, Aug 16, 2018 at 07:50:13AM +1000, Benjamin Herrenschmidt wrote: > > [Note: This isn't meant to be merged, it need splitting at the very > > least, see below] > > > > This is something I cooked up quickly today to test if that would fix > > my problems with large number of switch and NVME devices on POWER. > > > > So far it does... > > > > The issue (as discussed in the Re: PCIe enable device races thread) is > > that pci_enable_device and related functions along with pci_set_master > > and pci_enable_bridge are fundamentally racy. > > > > There is no lockign protecting the state of the device in pci_dev and > > if multiple devices under the same bridge try to enable it simultaneously > > one some of them will potentially start accessing it before it has actually > > been enabled. > > > > Now there are a LOT more fields in pci_dev that aren't covered by any > > form of locking. > > Most of the PCI core relies on the assumption that only a single > thread touches a device at a time. This is generally true of the core > during enumeration because enumeration is single-threaded. It's > generally true in normal driver operation because the core shouldn't > touch a device after a driver claims it. Mostly :-) There are a few exceptions though. > But there are several exceptions, and I think we need to understand > those scenarios before applying locks willy-nilly. We need to stop creating ad-hoc locks. We have a good handle already on the main enable/disable and bus master scenario, and the race with is_added. Ignore the patch itself, it has at least 2 bugs with PM, I'll send a series improving things a bit later. > One big exception is that enabling device A may also touch an upstream > bridge B. That causes things like your issue and Srinath's issue > where drivers simultaneously enabling two devices below the same > bridge corrupt the bridge's state [1]. Marta reported essentially the > same issue [2]. > > Hari's issue [3] seems related to a race between a driver work queue > and the core enumerating the device. I should have pushed harder to > understand this; I feel like we papered over the immediate problem > without clearing up the larger issue of why the core enumeration path > (pci_bus_add_device()) is running at the same time as a driver. It's not. What is happening is that is_added is set by pci_bus_add_device() after it has bound the driver. An easy fix would have been to move it up instead: diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 737d1c52f002..ff4d536d43fc 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -323,16 +323,16 @@ void pci_bus_add_device(struct pci_dev *dev) pci_proc_attach_device(dev); pci_bridge_d3_update(dev); + dev->is_added = 1; dev->match_driver = true; retval = device_attach(&dev->dev); if (retval < 0 && retval != -EPROBE_DEFER) { + dev->is_added = 0; pci_warn(dev, "device attach failed (%d)\n", retval); pci_proc_detach_device(dev); pci_remove_sysfs_dev_files(dev); return; } - - dev->is_added = 1; } EXPORT_SYMBOL_GPL(pci_bus_add_device); (Untested). Note: another advantage of the above is that the current code has an odd asymetry: is_added is currently set after we attach but also cleared after we detatch. If we want to keep the flag being set after attaching, then we do indeed need to protect it against concurrent access to other fields. The easiest way to do that would have been to remove the :1 as this: - unsigned int is_added:1; + unsigned int is_added; Again, none of these approach involves the invasive patch your merged which uses that atomic operation which provides the false sense of security that your are somewhat "protected" while in fact you only protect the field itself, but provide no protection about overall concurrency of the callers which might clash in different ways. Finally, we could also move is_added under the protection of the new mutex I propose adding, but that would really only work as long as we move all the :1 fields protected by that mutex together inside the struct pci_dev structure as to avoid collisions with other fields being modified. All of the above are preferable to what you merged. > DPC/AER error handling adds more cases where the core potentially > accesses devices asynchronously to the driver. > > User-mode sysfs controls like ASPM are also asynchronous to drivers. > > Even setpci is a potential issue, though I don't know how far we want > to go to protect it. I think we should make setpci taint the kernel > anyway. I wouldn't bother too much about it. > It might be nice if we had some sort of writeup of the locking > strategy as a whole. > > [1] https://lkml.kernel.org/r/1501858648-22228-1-git-send-email-srinath.mannam@broadcom.com > [2] https://lkml.kernel.org/r/744877924.5841545.1521630049567.JavaMail.zimbra@kalray.eu > [3] https://lkml.kernel.org/r/1530608741-30664-2-git-send-email-hari.vyas@broadcom.com Rather than not having one ? :) This is what I'm proposing here. Let me send patch series demonstrating the start of this, which also fix both above issues and completely remove that rather annoying atomic priv_flags. I would also like to get rid of the atomic enable_cnt but that will need a bit more churn through archs and drivers. Cheers, Ben.
On Thu, Aug 16, 2018 at 09:38:41AM +1000, Benjamin Herrenschmidt wrote: > On Wed, 2018-08-15 at 15:40 -0700, Guenter Roeck wrote: > > On Thu, Aug 16, 2018 at 07:50:13AM +1000, Benjamin Herrenschmidt wrote: > > > (Resent with lkml on copy) > > > > > > [Note: This isn't meant to be merged, it need splitting at the very > > > least, see below] > > > > > > This is something I cooked up quickly today to test if that would fix > > > my problems with large number of switch and NVME devices on POWER. > > > > > > > Is that a problem that can be reproduced with a qemu setup ? > > With difficulty... mt-tcg might help, but you need a rather large > systems to reproduce it. > > My repro-case is a 2 socket POWER9 system (about 40 cores off the top > of my mind, so 160 threads) with 72 NVME devices underneath a tree of > switches (I don't have the system at hand today to check how many). > > It's possible to observe it I suppose on a smaller system (in theory a > single bridge with 2 devices is enough) but in practice the timing is > extremely hard to hit. > > You need a combination of: > > - The bridges come up disabled (which is the case when Linux does the > resource assignment, such as on POWER but not on x86 unless it's > hotplug) > > - The nvme devices try to enable them simultaneously > > Also the resulting error is a UR, I don't know how well qemu models > that. > Not well enough, apparently. I tried for a while, registering as many nvme drives as the system would take, but I was not able to reproduce the problem with qemu. It was worth a try, though. Guenter
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 316496e99da9..9c4895c40f1d 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1348,9 +1348,13 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars) */ int pci_reenable_device(struct pci_dev *dev) { + int rc = 0; + + mutex_lock(&dev->lock); if (pci_is_enabled(dev)) - return do_pci_enable_device(dev, (1 << PCI_NUM_RESOURCES) - 1); - return 0; + rc = do_pci_enable_device(dev, (1 << PCI_NUM_RESOURCES) - 1); + mutex_unlock(&dev->lock); + return rc; } EXPORT_SYMBOL(pci_reenable_device); @@ -1363,12 +1367,6 @@ static void pci_enable_bridge(struct pci_dev *dev) if (bridge) pci_enable_bridge(bridge); - if (pci_is_enabled(dev)) { - if (!dev->is_busmaster) - pci_set_master(dev); - return; - } - retval = pci_enable_device(dev); if (retval) pci_err(dev, "Error enabling bridge (%d), continuing\n", @@ -1379,9 +1377,16 @@ static void pci_enable_bridge(struct pci_dev *dev) static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) { struct pci_dev *bridge; - int err; + int err = 0; int i, bars = 0; + /* Handle upstream bridges first to avoid locking issues */ + bridge = pci_upstream_bridge(dev); + if (bridge) + pci_enable_bridge(bridge); + + mutex_lock(&dev->lock); + /* * Power state could be unknown at this point, either due to a fresh * boot or a device removal call. So get the current power state @@ -1394,12 +1399,9 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK); } + /* Already enabled ? */ if (atomic_inc_return(&dev->enable_cnt) > 1) - return 0; /* already enabled */ - - bridge = pci_upstream_bridge(dev); - if (bridge) - pci_enable_bridge(bridge); + goto bail; /* only skip sriov related */ for (i = 0; i <= PCI_ROM_RESOURCE; i++) @@ -1412,6 +1414,8 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) err = do_pci_enable_device(dev, bars); if (err < 0) atomic_dec(&dev->enable_cnt); + bail: + mutex_unlock(&dev->lock); return err; } @@ -1618,6 +1622,7 @@ static void do_pci_disable_device(struct pci_dev *dev) if (pci_command & PCI_COMMAND_MASTER) { pci_command &= ~PCI_COMMAND_MASTER; pci_write_config_word(dev, PCI_COMMAND, pci_command); + dev->is_busmaster = 0; } pcibios_disable_device(dev); @@ -1632,8 +1637,10 @@ static void do_pci_disable_device(struct pci_dev *dev) */ void pci_disable_enabled_device(struct pci_dev *dev) { + mutex_lock(&dev->lock); if (pci_is_enabled(dev)) do_pci_disable_device(dev); + mutex_unlock(&dev->lock); } /** @@ -1657,12 +1664,13 @@ void pci_disable_device(struct pci_dev *dev) dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0, "disabling already-disabled device"); + mutex_lock(&dev->lock); if (atomic_dec_return(&dev->enable_cnt) != 0) - return; + goto bail; do_pci_disable_device(dev); - - dev->is_busmaster = 0; + bail: + mutex_unlock(&dev->lock); } EXPORT_SYMBOL(pci_disable_device); @@ -3764,8 +3772,12 @@ void __weak pcibios_set_master(struct pci_dev *dev) */ void pci_set_master(struct pci_dev *dev) { - __pci_set_master(dev, true); - pcibios_set_master(dev); + mutex_lock(&dev->lock); + if (!dev->is_busmaster) { + __pci_set_master(dev, true); + pcibios_set_master(dev); + } + mutex_unlock(&dev->lock); } EXPORT_SYMBOL(pci_set_master); @@ -3775,7 +3787,9 @@ EXPORT_SYMBOL(pci_set_master); */ void pci_clear_master(struct pci_dev *dev) { + mutex_lock(&dev->lock); __pci_set_master(dev, false); + mutex_unlock(&dev->lock); } EXPORT_SYMBOL(pci_clear_master); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 611adcd9c169..77701bfe0d3d 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2102,6 +2102,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->lock); return dev; } diff --git a/include/linux/pci.h b/include/linux/pci.h index c133ccfa002e..09e94ba6adf0 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -33,6 +33,7 @@ #include <linux/io.h> #include <linux/resource_ext.h> #include <uapi/linux/pci.h> +#include <linux/mutex.h> #include <linux/pci_ids.h> @@ -289,6 +290,8 @@ struct pci_dev { struct proc_dir_entry *procent; /* Device entry in /proc/bus/pci */ struct pci_slot *slot; /* Physical slot this device is in */ + struct mutex lock; + unsigned int devfn; /* Encoded device & function index */ unsigned short vendor; unsigned short device;
(Resent with lkml on copy) [Note: This isn't meant to be merged, it need splitting at the very least, see below] This is something I cooked up quickly today to test if that would fix my problems with large number of switch and NVME devices on POWER. So far it does... The issue (as discussed in the Re: PCIe enable device races thread) is that pci_enable_device and related functions along with pci_set_master and pci_enable_bridge are fundamentally racy. There is no lockign protecting the state of the device in pci_dev and if multiple devices under the same bridge try to enable it simultaneously one some of them will potentially start accessing it before it has actually been enabled. Now there are a LOT more fields in pci_dev that aren't covered by any form of locking. Additionally, some other parts such as ASPM or SR-IOV seem to be trying to do their own ad-hoc locking, but will manipulate bit fields in pci_dev that might be sharing words with other unrelated bit fields racily. So I think we need to introduce a pci_dev mutex aimed at synchronizing the main enable/master state of a given device, and possibly also the bazillion of state bits held inside pci_dev. I suggest a progressive approach: 1- First we add the mutex, and put a red line comment "/* ----- */" in struct pci_dev, at the bottom 2- We move the basic enable/disable/set_master stuff under that lock and move the corresponding fields in pci_dev below the line comment. 3- Progressively, as we untangle them and verify their locking, we move individual fields below the line so that we have a good visibility of what has been addressed/audited already and what not Note: I would very much like to keep most of the probing/resource allocation single threaded at least within a given domain, I know we have some attempts at locking in the hotplug code for that, I'm not ready to tackle that or change it at this stage. We might be able to also addrss the is_added issues (Hari Vyas stuff) using the same mutex, I haven't looked into the details. Not-signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> ---