@@ -20,7 +20,6 @@
#include <linux/pci_hotplug.h>
#include <linux/delay.h>
#include <linux/mutex.h>
-#include <linux/rwsem.h>
#include <linux/workqueue.h>
#include "../pcie/portdrv.h"
@@ -69,9 +68,6 @@
* @button_work: work item to turn the slot on or off after 5 seconds
* in response to an Attention Button press
* @hotplug_slot: structure registered with the PCI hotplug core
- * @reset_lock: prevents access to the Data Link Layer Link Active bit in the
- * Link Status register and to the Presence Detect State bit in the Slot
- * Status register during a slot reset which may cause them to flap
* @ist_running: flag to keep user request waiting while IRQ thread is running
* @request_result: result of last user request submitted to the IRQ thread
* @requester: wait queue to wake up on completion of user request,
@@ -102,7 +98,6 @@ struct controller {
struct delayed_work button_work;
struct hotplug_slot hotplug_slot; /* hotplug core interface */
- struct rw_semaphore reset_lock;
unsigned int ist_running;
int request_result;
wait_queue_head_t requester;
@@ -165,7 +165,7 @@ static void pciehp_check_presence(struct controller *ctrl)
{
int occupied;
- down_read(&ctrl->reset_lock);
+ down_read(&ctrl->hotplug_slot.pci_slot->reset_lock);
mutex_lock(&ctrl->state_lock);
occupied = pciehp_card_present_or_link_active(ctrl);
@@ -176,7 +176,7 @@ static void pciehp_check_presence(struct controller *ctrl)
pciehp_request(ctrl, PCI_EXP_SLTSTA_PDC);
mutex_unlock(&ctrl->state_lock);
- up_read(&ctrl->reset_lock);
+ up_read(&ctrl->hotplug_slot.pci_slot->reset_lock);
}
static int pciehp_probe(struct pcie_device *dev)
@@ -706,13 +706,17 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
/*
* Disable requests have higher priority than Presence Detect Changed
* or Data Link Layer State Changed events.
+ *
+ * A slot reset may cause flaps of the Presence Detect State bit in the
+ * Slot Status register and the Data Link Layer Link Active bit in the
+ * Link Status register. Prevent by holding the reset lock.
*/
- down_read(&ctrl->reset_lock);
+ down_read(&ctrl->hotplug_slot.pci_slot->reset_lock);
if (events & DISABLE_SLOT)
pciehp_handle_disable_request(ctrl);
else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
pciehp_handle_presence_or_link_change(ctrl, events);
- up_read(&ctrl->reset_lock);
+ up_read(&ctrl->hotplug_slot.pci_slot->reset_lock);
ret = IRQ_HANDLED;
out:
@@ -841,8 +845,6 @@ int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe)
if (probe)
return 0;
- down_write(&ctrl->reset_lock);
-
if (!ATTN_BUTTN(ctrl)) {
ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
stat_mask |= PCI_EXP_SLTSTA_PDC;
@@ -861,7 +863,6 @@ int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe)
ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, ctrl_mask);
- up_write(&ctrl->reset_lock);
return rc;
}
@@ -925,7 +926,6 @@ struct controller *pcie_init(struct pcie_device *dev)
ctrl->slot_cap = slot_cap;
mutex_init(&ctrl->ctrl_lock);
mutex_init(&ctrl->state_lock);
- init_rwsem(&ctrl->reset_lock);
init_waitqueue_head(&ctrl->requester);
init_waitqueue_head(&ctrl->queue);
INIT_DELAYED_WORK(&ctrl->button_work, pciehp_queue_pushbutton_work);
@@ -4902,6 +4902,8 @@ static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, int probe)
if (!hotplug || !try_module_get(hotplug->owner))
return rc;
+ lockdep_assert_held_write(&hotplug->pci_slot->reset_lock);
+
if (hotplug->ops->reset_slot)
rc = hotplug->ops->reset_slot(hotplug, probe);
@@ -5110,6 +5112,8 @@ int pci_reset_function(struct pci_dev *dev)
if (!dev->reset_fn)
return -ENOTTY;
+ if (dev->slot)
+ down_write(&dev->slot->reset_lock);
pci_dev_lock(dev);
pci_dev_save_and_disable(dev);
@@ -5117,6 +5121,8 @@ int pci_reset_function(struct pci_dev *dev)
pci_dev_restore(dev);
pci_dev_unlock(dev);
+ if (dev->slot)
+ up_write(&dev->slot->reset_lock);
return rc;
}
@@ -5169,6 +5175,9 @@ int pci_try_reset_function(struct pci_dev *dev)
if (!dev->reset_fn)
return -ENOTTY;
+ if (dev->slot && !down_write_trylock(&dev->slot->reset_lock))
+ return -EAGAIN;
+
if (!pci_dev_trylock(dev))
return -EAGAIN;
@@ -5176,6 +5185,8 @@ int pci_try_reset_function(struct pci_dev *dev)
rc = __pci_reset_function_locked(dev);
pci_dev_restore(dev);
pci_dev_unlock(dev);
+ if (dev->slot)
+ up_write(&dev->slot->reset_lock);
return rc;
}
@@ -5274,6 +5285,7 @@ static void pci_slot_lock(struct pci_slot *slot)
{
struct pci_dev *dev;
+ down_write(&slot->reset_lock);
list_for_each_entry(dev, &slot->bus->devices, bus_list) {
if (!dev->slot || dev->slot != slot)
continue;
@@ -5295,6 +5307,7 @@ static void pci_slot_unlock(struct pci_slot *slot)
pci_bus_unlock(dev->subordinate);
pci_dev_unlock(dev);
}
+ up_write(&slot->reset_lock);
}
/* Return 1 on successful lock, 0 on contention */
@@ -5302,6 +5315,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
{
struct pci_dev *dev;
+ if (!down_write_trylock(&slot->reset_lock))
+ return 0;
+
list_for_each_entry(dev, &slot->bus->devices, bus_list) {
if (!dev->slot || dev->slot != slot)
continue;
@@ -5325,6 +5341,7 @@ static int pci_slot_trylock(struct pci_slot *slot)
pci_bus_unlock(dev->subordinate);
pci_dev_unlock(dev);
}
+ up_write(&slot->reset_lock);
return 0;
}
@@ -279,6 +279,8 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
INIT_LIST_HEAD(&slot->list);
list_add(&slot->list, &parent->slots);
+ init_rwsem(&slot->reset_lock);
+
down_read(&pci_bus_sem);
list_for_each_entry(dev, &parent->devices, bus_list)
if (PCI_SLOT(dev->devfn) == slot_nr)
@@ -454,13 +454,20 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
* We can not use the "try" reset interface here, which will
* overwrite the previously restored configuration information.
*/
- if (vdev->reset_works && pci_cfg_access_trylock(pdev)) {
- if (device_trylock(&pdev->dev)) {
- if (!__pci_reset_function_locked(pdev))
- vdev->needs_reset = false;
- device_unlock(&pdev->dev);
+ if (vdev->reset_works) {
+ if (!pdev->slot ||
+ down_write_trylock(&pdev->slot->reset_lock)) {
+ if (pci_cfg_access_trylock(pdev)) {
+ if (device_trylock(&pdev->dev)) {
+ if (!__pci_reset_function_locked(pdev))
+ vdev->needs_reset = false;
+ device_unlock(&pdev->dev);
+ }
+ pci_cfg_access_unlock(pdev);
+ }
+ if (pdev->slot)
+ up_write(&pdev->slot->reset_lock);
}
- pci_cfg_access_unlock(pdev);
}
pci_restore_state(pdev);
@@ -89,11 +89,17 @@ static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,
mutex_unlock(&dev_data->lock);
if (found_dev) {
- if (lock)
+ if (lock) {
+ if (found_dev->slot)
+ down_write(&found_dev->slot->reset_lock);
device_lock(&found_dev->dev);
+ }
pcistub_put_pci_dev(found_dev);
- if (lock)
+ if (lock) {
device_unlock(&found_dev->dev);
+ if (found_dev->slot)
+ up_write(&found_dev->slot->reset_lock);
+ }
}
}
@@ -164,9 +170,13 @@ static void __xen_pcibk_release_devices(struct xen_pcibk_device *pdev)
list_for_each_entry_safe(dev_entry, t, &dev_data->dev_list, list) {
struct pci_dev *dev = dev_entry->dev;
list_del(&dev_entry->list);
+ if (dev->slot)
+ down_write(&dev->slot->reset_lock);
device_lock(&dev->dev);
pcistub_put_pci_dev(dev);
device_unlock(&dev->dev);
+ if (dev->slot)
+ up_write(&dev->slot->reset_lock);
kfree(dev_entry);
}
@@ -463,6 +463,9 @@ static int __init pcistub_init_devices_late(void)
spin_unlock_irqrestore(&pcistub_devices_lock, flags);
+ if (psdev->dev->slot)
+ down_write(&psdev->dev->slot->reset_lock);
+ device_lock(&psdev->dev->dev);
err = pcistub_init_device(psdev->dev);
if (err) {
dev_err(&psdev->dev->dev,
@@ -470,6 +473,9 @@ static int __init pcistub_init_devices_late(void)
kfree(psdev);
psdev = NULL;
}
+ device_unlock(&psdev->dev->dev);
+ if (psdev->dev->slot)
+ up_write(&psdev->dev->slot->reset_lock);
spin_lock_irqsave(&pcistub_devices_lock, flags);
@@ -171,11 +171,17 @@ static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,
mutex_unlock(&vpci_dev->lock);
if (found_dev) {
- if (lock)
+ if (lock) {
+ if (found_dev->slot)
+ down_write(&found_dev->slot->reset_lock);
device_lock(&found_dev->dev);
+ }
pcistub_put_pci_dev(found_dev);
- if (lock)
+ if (lock) {
device_unlock(&found_dev->dev);
+ if (found_dev->slot)
+ up_write(&found_dev->slot->reset_lock);
+ }
}
}
@@ -216,9 +222,13 @@ static void __xen_pcibk_release_devices(struct xen_pcibk_device *pdev)
list) {
struct pci_dev *dev = e->dev;
list_del(&e->list);
+ if (dev->slot)
+ down_write(&dev->slot->reset_lock);
device_lock(&dev->dev);
pcistub_put_pci_dev(dev);
device_unlock(&dev->dev);
+ if (dev->slot)
+ up_write(&dev->slot->reset_lock);
kfree(e);
}
}
@@ -38,6 +38,7 @@
#include <linux/interrupt.h>
#include <linux/io.h>
#include <linux/resource_ext.h>
+#include <linux/rwsem.h>
#include <uapi/linux/pci.h>
#include <linux/pci_ids.h>
@@ -65,11 +66,16 @@
/* return bus from PCI devid = ((u16)bus_number) << 8) | devfn */
#define PCI_BUS_NUM(x) (((x) >> 8) & 0xff)
-/* pci_slot represents a physical slot */
+/**
+ * struct pci_slot - represents a physical slot
+ * @reset_lock: held for writing during a slot reset; acquire for reading to
+ * protect access to register bits which may flap upon a reset
+ */
struct pci_slot {
struct pci_bus *bus; /* Bus this slot is on */
struct list_head list; /* Node in list of slots */
struct hotplug_slot *hotplug; /* Hotplug info (move here) */
+ struct rw_semaphore reset_lock;
unsigned char number; /* PCI_SLOT(pci_dev->devfn) */
struct kobject kobj;
};
Back in 2013, commits 2e35afaefe64 ("PCI: pciehp: Add reset_slot() method") 608c388122c7 ("PCI: Add slot reset option to pci_dev_reset()") introduced the callback pciehp_reset_slot() to the PCIe hotplug driver and amended __pci_dev_reset() (today __pci_reset_function_locked()) to invoke it when resetting a hotplug port's child. The callback performs a Secondary Bus Reset and ensures that an ensuing link or presence flap is ignored by pciehp. However the commits did not perform any locking, in particular: * No precautions were taken to prevent concurrent execution of the new callback with pciehp's IRQ handler or a sysfs request to bring the slot up or down. These code paths may see flapping link or presence bits during a slot reset. * pciehp is not prevented from unbinding while the new callback accesses its struct controller. Commit 608c388122c7 did take a reference on pciehp's module, but that's not sufficient. It only keeps pciehp's code in memory, but doesn't prevent unbinding. * In pci_dev_reset_slot_function(), commit 608c388122c7 iterates over the devices on a bus without holding pci_bus_sem. In 2018, commit 5b3f7b7d062b ("PCI: pciehp: Avoid slot access during reset") sought to address the first of these locking issues: It introduced a reset_lock which serializes a slot reset with other parts of pciehp. But Michael Haeuptle reports that deadlocks now occur on simultaneous hot-removal and reset of vfio devices because pciehp acquires the reset_lock and the device_lock in a different order than pci_try_reset_function(): pciehp_ist() # down_read(reset_lock) pciehp_handle_presence_or_link_change() pciehp_disable_slot() __pciehp_disable_slot() remove_board() pciehp_unconfigure_device() pci_stop_and_remove_bus_device() pci_stop_bus_device() pci_stop_dev() device_release_driver() device_release_driver_internal() __device_driver_lock() # device_lock() SYS_munmap() vfio_device_fops_release() vfio_pci_release() vfio_pci_disable() pci_try_reset_function() # device_lock() __pci_reset_function_locked() pci_reset_hotplug_slot() pciehp_reset_slot() # down_write(reset_lock) Ian May reports the same deadlock on simultaneous hot-removal and AER reset: aer_recover_work_func() pcie_do_recovery() aer_root_reset() pci_bus_error_reset() pci_slot_reset() pci_slot_lock() # device_lock() pci_reset_hotplug_slot() pciehp_reset_slot() # down_write(reset_lock) Fix by pushing the reset_lock out of pciehp's struct controller and into struct pci_slot such that it can be taken by the PCI core before taking the device lock. There's a catch though: Some drivers call __pci_reset_function_locked() directly and the function expects that all necessary locks, including the reset_lock, have been acquired by the caller. There are callers which already hold the device_lock, so they can't acquire the reset_lock without re-introducing the AB-BA deadlock: * drivers/net/ethernet/cavium/liquidio/lio_main.c: octeon_pci_flr() * drivers/xen/xen-pciback/pci_stub.c: pcistub_device_release() * drivers/xen/xen-pciback/pci_stub.c: pcistub_init_device() (if called from pcistub_seize()) In the case of octeon_pci_flr(), the device is reset on driver unbind, which is why the device_lock is already held. A possible solution might be to add a flag in struct pci_dev with which drivers tell the PCI core that the device is handed back in an unclean state and needs a reset. The PCI core would then perform the reset on behalf of the driver after it has unbound and before any new driver is bound. As for xen, this patch (which was never applied) explains that a reset is performed on bind, unbind and on un-assigning a device from a guest: https://lore.kernel.org/patchwork/patch/848180/ The unbind code path could be solved by the same solution as for octeon and it may also be possible to make it work on bind, though it's unclear why a reset on bind is at all necessary. The un-assigning code path is fixed by the present commit I think. For now, the three functions do not acquire the reset_lock. I'm inserting a lockdep_assert_held_write() so that a lockdep splat is shown as a reminder that liquidio and xen require fixing. Fixes: 5b3f7b7d062b ("PCI: pciehp: Avoid slot access during reset") Link: https://lore.kernel.org/linux-pci/CS1PR8401MB0728FC6FDAB8A35C22BD90EC95F10@CS1PR8401MB0728.NAMPRD84.PROD.OUTLOOK.COM Link: https://lore.kernel.org/linux-pci/20200615143250.438252-1-ian.may@canonical.com Reported-and-tested-by: Michael Haeuptle <michael.haeuptle@hpe.com> Reported-and-tested-by: Ian May <ian.may@canonical.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: stable@vger.kernel.org # v4.19+ Cc: Alex Williamson <alex.williamson@redhat.com> --- drivers/pci/hotplug/pciehp.h | 5 ----- drivers/pci/hotplug/pciehp_core.c | 4 ++-- drivers/pci/hotplug/pciehp_hpc.c | 12 ++++++------ drivers/pci/pci.c | 17 +++++++++++++++++ drivers/pci/slot.c | 2 ++ drivers/vfio/pci/vfio_pci.c | 19 +++++++++++++------ drivers/xen/xen-pciback/passthrough.c | 14 ++++++++++++-- drivers/xen/xen-pciback/pci_stub.c | 6 ++++++ drivers/xen/xen-pciback/vpci.c | 14 ++++++++++++-- include/linux/pci.h | 8 +++++++- 10 files changed, 77 insertions(+), 24 deletions(-)