avoid null pointer rereference during FLR V2

avoid null pointer rereference during FLR V2

[Christoph Hellwig]

Hi all,

Rakesh reported a bug where a FLR can trivially crash his system.
The reason for that is that NVMe unbinds the driver from the PCI device
on an unrecoverable error, and that races with the reset_notify method.

This is fairly easily fixable by taking the device lock for a slightly
longer period.  Note that the other PCI error handling methods actually
have the same issue, but with them not taking the lock yet and me having
no good way to reproducibly call them I'm a little reluctant to touch
them, but it would be great if we could fix those issues as well.

Patches 2 and 3 are cleanups in the same area and not 4.12 material,
but given that they depend on the first one I thought I'd send them
along.

Changes since V1:
 - lock over all calls to ->reset_notify

avoid null pointer rereference during FLR V2

[Christoph Hellwig]

Hi all,

Rakesh reported a bug where a FLR can trivially crash his system.
The reason for that is that NVMe unbinds the driver from the PCI device
on an unrecoverable error, and that races with the reset_notify method.

This is fairly easily fixable by taking the device lock for a slightly
longer period.  Note that the other PCI error handling methods actually
have the same issue, but with them not taking the lock yet and me having
no good way to reproducibly call them I'm a little reluctant to touch
them, but it would be great if we could fix those issues as well.

Patches 2 and 3 are cleanups in the same area and not 4.12 material,
but given that they depend on the first one I thought I'd send them
along.

Changes since V1:
 - lock over all calls to ->reset_notify

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Christoph Hellwig]

Without this ->notify_reset instance may race with ->remove calls,
which can be easily triggered in NVMe.

Reported-by: Rakesh Pandit <rakesh@tuxera.com>
Tested-by: Rakesh Pandit <rakesh@tuxera.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/pci.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 563901cd9c06..92f7e5ae2e5e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4276,11 +4276,13 @@ int pci_reset_function(struct pci_dev *dev)
 	if (rc)
 		return rc;
 
+	pci_dev_lock(dev);
 	pci_dev_save_and_disable(dev);
 
-	rc = pci_dev_reset(dev, 0);
+	rc = __pci_dev_reset(dev, 0);
 
 	pci_dev_restore(dev);
+	pci_dev_unlock(dev);
 
 	return rc;
 }
@@ -4300,16 +4302,14 @@ int pci_try_reset_function(struct pci_dev *dev)
 	if (rc)
 		return rc;
 
-	pci_dev_save_and_disable(dev);
+	if (pci_dev_trylock(dev))
+		return -EAGAIN;
 
-	if (pci_dev_trylock(dev)) {
-		rc = __pci_dev_reset(dev, 0);
-		pci_dev_unlock(dev);
-	} else
-		rc = -EAGAIN;
+	pci_dev_save_and_disable(dev);
+	rc = __pci_dev_reset(dev, 0);
+	pci_dev_unlock(dev);
 
 	pci_dev_restore(dev);
-
 	return rc;
 }
 EXPORT_SYMBOL_GPL(pci_try_reset_function);
@@ -4459,7 +4459,9 @@ static void pci_bus_save_and_disable(struct pci_bus *bus)
 	struct pci_dev *dev;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
+		pci_dev_lock(dev);
 		pci_dev_save_and_disable(dev);
+		pci_dev_unlock(dev);
 		if (dev->subordinate)
 			pci_bus_save_and_disable(dev->subordinate);
 	}
@@ -4474,7 +4476,9 @@ static void pci_bus_restore(struct pci_bus *bus)
 	struct pci_dev *dev;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
+		pci_dev_lock(dev);
 		pci_dev_restore(dev);
+		pci_dev_unlock(dev);
 		if (dev->subordinate)
 			pci_bus_restore(dev->subordinate);
 	}
-- 
2.11.0

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Christoph Hellwig]

Without this ->notify_reset instance may race with ->remove calls,
which can be easily triggered in NVMe.

Reported-by: Rakesh Pandit <rakesh at tuxera.com>
Tested-by: Rakesh Pandit <rakesh at tuxera.com>
Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/pci/pci.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 563901cd9c06..92f7e5ae2e5e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4276,11 +4276,13 @@ int pci_reset_function(struct pci_dev *dev)
 	if (rc)
 		return rc;
 
+	pci_dev_lock(dev);
 	pci_dev_save_and_disable(dev);
 
-	rc = pci_dev_reset(dev, 0);
+	rc = __pci_dev_reset(dev, 0);
 
 	pci_dev_restore(dev);
+	pci_dev_unlock(dev);
 
 	return rc;
 }
@@ -4300,16 +4302,14 @@ int pci_try_reset_function(struct pci_dev *dev)
 	if (rc)
 		return rc;
 
-	pci_dev_save_and_disable(dev);
+	if (pci_dev_trylock(dev))
+		return -EAGAIN;
 
-	if (pci_dev_trylock(dev)) {
-		rc = __pci_dev_reset(dev, 0);
-		pci_dev_unlock(dev);
-	} else
-		rc = -EAGAIN;
+	pci_dev_save_and_disable(dev);
+	rc = __pci_dev_reset(dev, 0);
+	pci_dev_unlock(dev);
 
 	pci_dev_restore(dev);
-
 	return rc;
 }
 EXPORT_SYMBOL_GPL(pci_try_reset_function);
@@ -4459,7 +4459,9 @@ static void pci_bus_save_and_disable(struct pci_bus *bus)
 	struct pci_dev *dev;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
+		pci_dev_lock(dev);
 		pci_dev_save_and_disable(dev);
+		pci_dev_unlock(dev);
 		if (dev->subordinate)
 			pci_bus_save_and_disable(dev->subordinate);
 	}
@@ -4474,7 +4476,9 @@ static void pci_bus_restore(struct pci_bus *bus)
 	struct pci_dev *dev;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
+		pci_dev_lock(dev);
 		pci_dev_restore(dev);
+		pci_dev_unlock(dev);
 		if (dev->subordinate)
 			pci_bus_restore(dev->subordinate);
 	}
-- 
2.11.0

[PATCH 2/3] PCI: split reset_notify method

[Christoph Hellwig]

The prepare and done cases have no shared functionality whatsoever, so
split them into separate methods.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 36 +++++--------
 drivers/net/wireless/marvell/mwifiex/pcie.c  | 75 ++++++++++++++++------------
 drivers/nvme/host/pci.c                      | 15 +++---
 drivers/pci/pci.c                            | 25 +++-------
 include/linux/pci.h                          |  3 +-
 5 files changed, 75 insertions(+), 79 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index 3e26d27ad213..63784576ae8b 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -2348,30 +2348,19 @@ static void fm10k_io_resume(struct pci_dev *pdev)
 		netif_device_attach(netdev);
 }
 
-/**
- * fm10k_io_reset_notify - called when PCI function is reset
- * @pdev: Pointer to PCI device
- *
- * This callback is called when the PCI function is reset such as from
- * /sys/class/net/<enpX>/device/reset or similar. When prepare is true, it
- * means we should prepare for a function reset. If prepare is false, it means
- * the function reset just occurred.
- */
-static void fm10k_io_reset_notify(struct pci_dev *pdev, bool prepare)
+static void fm10k_io_reset_prepare(struct pci_dev *pdev)
 {
-	struct fm10k_intfc *interface = pci_get_drvdata(pdev);
-	int err = 0;
-
-	if (prepare) {
-		/* warn incase we have any active VF devices */
-		if (pci_num_vf(pdev))
-			dev_warn(&pdev->dev,
-				 "PCIe FLR may cause issues for any active VF devices\n");
+	/* warn incase we have any active VF devices */
+	if (pci_num_vf(pdev))
+		dev_warn(&pdev->dev,
+			 "PCIe FLR may cause issues for any active VF devices\n");
+	fm10k_prepare_suspend(pci_get_drvdata(pdev));
+}
 
-		fm10k_prepare_suspend(interface);
-	} else {
-		err = fm10k_handle_resume(interface);
-	}
+static void fm10k_io_reset_done(struct pci_dev *pdev)
+{
+	struct fm10k_intfc *interface = pci_get_drvdata(pdev);
+	int err = fm10k_handle_resume(interface);
 
 	if (err) {
 		dev_warn(&pdev->dev,
@@ -2384,7 +2373,8 @@ static const struct pci_error_handlers fm10k_err_handler = {
 	.error_detected = fm10k_io_error_detected,
 	.slot_reset = fm10k_io_slot_reset,
 	.resume = fm10k_io_resume,
-	.reset_notify = fm10k_io_reset_notify,
+	.reset_prepare = fm10k_io_reset_prepare,
+	.reset_done = fm10k_io_reset_done,
 };
 
 static struct pci_driver fm10k_driver = {
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index ac62bce50e96..279adf124fc9 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -346,11 +346,13 @@ static const struct pci_device_id mwifiex_ids[] = {
 
 MODULE_DEVICE_TABLE(pci, mwifiex_ids);
 
-static void mwifiex_pcie_reset_notify(struct pci_dev *pdev, bool prepare)
+/*
+ * Cleanup all software without cleaning anything related to PCIe and HW.
+ */
+static void mwifiex_pcie_reset_prepare(struct pci_dev *pdev)
 {
 	struct pcie_service_card *card = pci_get_drvdata(pdev);
 	struct mwifiex_adapter *adapter = card->adapter;
-	int ret;
 
 	if (!adapter) {
 		dev_err(&pdev->dev, "%s: adapter structure is not valid\n",
@@ -359,37 +361,48 @@ static void mwifiex_pcie_reset_notify(struct pci_dev *pdev, bool prepare)
 	}
 
 	mwifiex_dbg(adapter, INFO,
-		    "%s: vendor=0x%4.04x device=0x%4.04x rev=%d %s\n",
-		    __func__, pdev->vendor, pdev->device,
-		    pdev->revision,
-		    prepare ? "Pre-FLR" : "Post-FLR");
-
-	if (prepare) {
-		/* Kernel would be performing FLR after this notification.
-		 * Cleanup all software without cleaning anything related to
-		 * PCIe and HW.
-		 */
-		mwifiex_shutdown_sw(adapter);
-		adapter->surprise_removed = true;
-		clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
-		clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
-	} else {
-		/* Kernel stores and restores PCIe function context before and
-		 * after performing FLR respectively. Reconfigure the software
-		 * and firmware including firmware redownload
-		 */
-		adapter->surprise_removed = false;
-		ret = mwifiex_reinit_sw(adapter);
-		if (ret) {
-			dev_err(&pdev->dev, "reinit failed: %d\n", ret);
-			return;
-		}
-	}
+		    "%s: vendor=0x%4.04x device=0x%4.04x rev=%d Pre-FLR\n",
+		    __func__, pdev->vendor, pdev->device, pdev->revision);
+
+	mwifiex_shutdown_sw(adapter);
+	adapter->surprise_removed = true;
+	clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
+	clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
 	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
 }
 
-static const struct pci_error_handlers mwifiex_pcie_err_handler[] = {
-		{ .reset_notify = mwifiex_pcie_reset_notify, },
+/*
+ * Kernel stores and restores PCIe function context before and after performing
+ * FLR respectively. Reconfigure the software and firmware including firmware
+ * redownload.
+ */
+static void mwifiex_pcie_reset_done(struct pci_dev *pdev)
+{
+	struct pcie_service_card *card = pci_get_drvdata(pdev);
+	struct mwifiex_adapter *adapter = card->adapter;
+	int ret;
+
+	if (!adapter) {
+		dev_err(&pdev->dev, "%s: adapter structure is not valid\n",
+			__func__);
+		return;
+	}
+
+	mwifiex_dbg(adapter, INFO,
+		    "%s: vendor=0x%4.04x device=0x%4.04x rev=%d Post-FLR\n",
+		    __func__, pdev->vendor, pdev->device, pdev->revision);
+
+	adapter->surprise_removed = false;
+	ret = mwifiex_reinit_sw(adapter);
+	if (ret)
+		dev_err(&pdev->dev, "reinit failed: %d\n", ret);
+	else
+		mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
+}
+
+static const struct pci_error_handlers mwifiex_pcie_err_handler = {
+	.reset_prepare		= mwifiex_pcie_reset_prepare,
+	.reset_done		= mwifiex_pcie_reset_done,
 };
 
 #ifdef CONFIG_PM_SLEEP
@@ -410,7 +423,7 @@ static struct pci_driver __refdata mwifiex_pcie = {
 	},
 #endif
 	.shutdown = mwifiex_pcie_shutdown,
-	.err_handler = mwifiex_pcie_err_handler,
+	.err_handler = &mwifiex_pcie_err_handler,
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d52701df7245..5059f0f983b5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2152,14 +2152,14 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return result;
 }
 
-static void nvme_reset_notify(struct pci_dev *pdev, bool prepare)
+static void nvme_reset_prepare(struct pci_dev *pdev)
 {
-	struct nvme_dev *dev = pci_get_drvdata(pdev);
+	nvme_dev_disable(pci_get_drvdata(pdev), false);
+}
 
-	if (prepare)
-		nvme_dev_disable(dev, false);
-	else
-		nvme_reset(dev);
+static void nvme_reset_done(struct pci_dev *pdev)
+{
+	nvme_reset(pci_get_drvdata(pdev));
 }
 
 static void nvme_shutdown(struct pci_dev *pdev)
@@ -2281,7 +2281,8 @@ static const struct pci_error_handlers nvme_err_handler = {
 	.error_detected	= nvme_error_detected,
 	.slot_reset	= nvme_slot_reset,
 	.resume		= nvme_error_resume,
-	.reset_notify	= nvme_reset_notify,
+	.reset_prepare	= nvme_reset_prepare,
+	.reset_done	= nvme_reset_done,
 };
 
 static const struct pci_device_id nvme_id_table[] = {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 92f7e5ae2e5e..b3f9ae7a4e21 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4128,26 +4128,13 @@ static void pci_dev_unlock(struct pci_dev *dev)
 	pci_cfg_access_unlock(dev);
 }
 
-/**
- * pci_reset_notify - notify device driver of reset
- * @dev: device to be notified of reset
- * @prepare: 'true' if device is about to be reset; 'false' if reset attempt
- *           completed
- *
- * Must be called prior to device access being disabled and after device
- * access is restored.
- */
-static void pci_reset_notify(struct pci_dev *dev, bool prepare)
+static void pci_dev_save_and_disable(struct pci_dev *dev)
 {
 	const struct pci_error_handlers *err_handler =
 			dev->driver ? dev->driver->err_handler : NULL;
-	if (err_handler && err_handler->reset_notify)
-		err_handler->reset_notify(dev, prepare);
-}
 
-static void pci_dev_save_and_disable(struct pci_dev *dev)
-{
-	pci_reset_notify(dev, true);
+	if (err_handler && err_handler->reset_prepare)
+		err_handler->reset_prepare(dev);
 
 	/*
 	 * Wake-up device prior to save.  PM registers default to D0 after
@@ -4169,8 +4156,12 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
 
 static void pci_dev_restore(struct pci_dev *dev)
 {
+	const struct pci_error_handlers *err_handler =
+			dev->driver ? dev->driver->err_handler : NULL;
+
 	pci_restore_state(dev);
-	pci_reset_notify(dev, false);
+	if (err_handler && err_handler->reset_done)
+		err_handler->reset_done(dev);
 }
 
 static int pci_dev_reset(struct pci_dev *dev, int probe)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f0ca05..b87aec18e673 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -695,7 +695,8 @@ struct pci_error_handlers {
 	pci_ers_result_t (*slot_reset)(struct pci_dev *dev);
 
 	/* PCI function reset prepare or completed */
-	void (*reset_notify)(struct pci_dev *dev, bool prepare);
+	void (*reset_prepare)(struct pci_dev *dev);
+	void (*reset_done)(struct pci_dev *dev);
 
 	/* Device driver may resume normal operations */
 	void (*resume)(struct pci_dev *dev);
-- 
2.11.0

[PATCH 2/3] PCI: split reset_notify method

[Christoph Hellwig]

The prepare and done cases have no shared functionality whatsoever, so
split them into separate methods.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 36 +++++--------
 drivers/net/wireless/marvell/mwifiex/pcie.c  | 75 ++++++++++++++++------------
 drivers/nvme/host/pci.c                      | 15 +++---
 drivers/pci/pci.c                            | 25 +++-------
 include/linux/pci.h                          |  3 +-
 5 files changed, 75 insertions(+), 79 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index 3e26d27ad213..63784576ae8b 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -2348,30 +2348,19 @@ static void fm10k_io_resume(struct pci_dev *pdev)
 		netif_device_attach(netdev);
 }
 
-/**
- * fm10k_io_reset_notify - called when PCI function is reset
- * @pdev: Pointer to PCI device
- *
- * This callback is called when the PCI function is reset such as from
- * /sys/class/net/<enpX>/device/reset or similar. When prepare is true, it
- * means we should prepare for a function reset. If prepare is false, it means
- * the function reset just occurred.
- */
-static void fm10k_io_reset_notify(struct pci_dev *pdev, bool prepare)
+static void fm10k_io_reset_prepare(struct pci_dev *pdev)
 {
-	struct fm10k_intfc *interface = pci_get_drvdata(pdev);
-	int err = 0;
-
-	if (prepare) {
-		/* warn incase we have any active VF devices */
-		if (pci_num_vf(pdev))
-			dev_warn(&pdev->dev,
-				 "PCIe FLR may cause issues for any active VF devices\n");
+	/* warn incase we have any active VF devices */
+	if (pci_num_vf(pdev))
+		dev_warn(&pdev->dev,
+			 "PCIe FLR may cause issues for any active VF devices\n");
+	fm10k_prepare_suspend(pci_get_drvdata(pdev));
+}
 
-		fm10k_prepare_suspend(interface);
-	} else {
-		err = fm10k_handle_resume(interface);
-	}
+static void fm10k_io_reset_done(struct pci_dev *pdev)
+{
+	struct fm10k_intfc *interface = pci_get_drvdata(pdev);
+	int err = fm10k_handle_resume(interface);
 
 	if (err) {
 		dev_warn(&pdev->dev,
@@ -2384,7 +2373,8 @@ static const struct pci_error_handlers fm10k_err_handler = {
 	.error_detected = fm10k_io_error_detected,
 	.slot_reset = fm10k_io_slot_reset,
 	.resume = fm10k_io_resume,
-	.reset_notify = fm10k_io_reset_notify,
+	.reset_prepare = fm10k_io_reset_prepare,
+	.reset_done = fm10k_io_reset_done,
 };
 
 static struct pci_driver fm10k_driver = {
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index ac62bce50e96..279adf124fc9 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -346,11 +346,13 @@ static const struct pci_device_id mwifiex_ids[] = {
 
 MODULE_DEVICE_TABLE(pci, mwifiex_ids);
 
-static void mwifiex_pcie_reset_notify(struct pci_dev *pdev, bool prepare)
+/*
+ * Cleanup all software without cleaning anything related to PCIe and HW.
+ */
+static void mwifiex_pcie_reset_prepare(struct pci_dev *pdev)
 {
 	struct pcie_service_card *card = pci_get_drvdata(pdev);
 	struct mwifiex_adapter *adapter = card->adapter;
-	int ret;
 
 	if (!adapter) {
 		dev_err(&pdev->dev, "%s: adapter structure is not valid\n",
@@ -359,37 +361,48 @@ static void mwifiex_pcie_reset_notify(struct pci_dev *pdev, bool prepare)
 	}
 
 	mwifiex_dbg(adapter, INFO,
-		    "%s: vendor=0x%4.04x device=0x%4.04x rev=%d %s\n",
-		    __func__, pdev->vendor, pdev->device,
-		    pdev->revision,
-		    prepare ? "Pre-FLR" : "Post-FLR");
-
-	if (prepare) {
-		/* Kernel would be performing FLR after this notification.
-		 * Cleanup all software without cleaning anything related to
-		 * PCIe and HW.
-		 */
-		mwifiex_shutdown_sw(adapter);
-		adapter->surprise_removed = true;
-		clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
-		clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
-	} else {
-		/* Kernel stores and restores PCIe function context before and
-		 * after performing FLR respectively. Reconfigure the software
-		 * and firmware including firmware redownload
-		 */
-		adapter->surprise_removed = false;
-		ret = mwifiex_reinit_sw(adapter);
-		if (ret) {
-			dev_err(&pdev->dev, "reinit failed: %d\n", ret);
-			return;
-		}
-	}
+		    "%s: vendor=0x%4.04x device=0x%4.04x rev=%d Pre-FLR\n",
+		    __func__, pdev->vendor, pdev->device, pdev->revision);
+
+	mwifiex_shutdown_sw(adapter);
+	adapter->surprise_removed = true;
+	clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
+	clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
 	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
 }
 
-static const struct pci_error_handlers mwifiex_pcie_err_handler[] = {
-		{ .reset_notify = mwifiex_pcie_reset_notify, },
+/*
+ * Kernel stores and restores PCIe function context before and after performing
+ * FLR respectively. Reconfigure the software and firmware including firmware
+ * redownload.
+ */
+static void mwifiex_pcie_reset_done(struct pci_dev *pdev)
+{
+	struct pcie_service_card *card = pci_get_drvdata(pdev);
+	struct mwifiex_adapter *adapter = card->adapter;
+	int ret;
+
+	if (!adapter) {
+		dev_err(&pdev->dev, "%s: adapter structure is not valid\n",
+			__func__);
+		return;
+	}
+
+	mwifiex_dbg(adapter, INFO,
+		    "%s: vendor=0x%4.04x device=0x%4.04x rev=%d Post-FLR\n",
+		    __func__, pdev->vendor, pdev->device, pdev->revision);
+
+	adapter->surprise_removed = false;
+	ret = mwifiex_reinit_sw(adapter);
+	if (ret)
+		dev_err(&pdev->dev, "reinit failed: %d\n", ret);
+	else
+		mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
+}
+
+static const struct pci_error_handlers mwifiex_pcie_err_handler = {
+	.reset_prepare		= mwifiex_pcie_reset_prepare,
+	.reset_done		= mwifiex_pcie_reset_done,
 };
 
 #ifdef CONFIG_PM_SLEEP
@@ -410,7 +423,7 @@ static struct pci_driver __refdata mwifiex_pcie = {
 	},
 #endif
 	.shutdown = mwifiex_pcie_shutdown,
-	.err_handler = mwifiex_pcie_err_handler,
+	.err_handler = &mwifiex_pcie_err_handler,
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d52701df7245..5059f0f983b5 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2152,14 +2152,14 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return result;
 }
 
-static void nvme_reset_notify(struct pci_dev *pdev, bool prepare)
+static void nvme_reset_prepare(struct pci_dev *pdev)
 {
-	struct nvme_dev *dev = pci_get_drvdata(pdev);
+	nvme_dev_disable(pci_get_drvdata(pdev), false);
+}
 
-	if (prepare)
-		nvme_dev_disable(dev, false);
-	else
-		nvme_reset(dev);
+static void nvme_reset_done(struct pci_dev *pdev)
+{
+	nvme_reset(pci_get_drvdata(pdev));
 }
 
 static void nvme_shutdown(struct pci_dev *pdev)
@@ -2281,7 +2281,8 @@ static const struct pci_error_handlers nvme_err_handler = {
 	.error_detected	= nvme_error_detected,
 	.slot_reset	= nvme_slot_reset,
 	.resume		= nvme_error_resume,
-	.reset_notify	= nvme_reset_notify,
+	.reset_prepare	= nvme_reset_prepare,
+	.reset_done	= nvme_reset_done,
 };
 
 static const struct pci_device_id nvme_id_table[] = {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 92f7e5ae2e5e..b3f9ae7a4e21 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4128,26 +4128,13 @@ static void pci_dev_unlock(struct pci_dev *dev)
 	pci_cfg_access_unlock(dev);
 }
 
-/**
- * pci_reset_notify - notify device driver of reset
- * @dev: device to be notified of reset
- * @prepare: 'true' if device is about to be reset; 'false' if reset attempt
- *           completed
- *
- * Must be called prior to device access being disabled and after device
- * access is restored.
- */
-static void pci_reset_notify(struct pci_dev *dev, bool prepare)
+static void pci_dev_save_and_disable(struct pci_dev *dev)
 {
 	const struct pci_error_handlers *err_handler =
 			dev->driver ? dev->driver->err_handler : NULL;
-	if (err_handler && err_handler->reset_notify)
-		err_handler->reset_notify(dev, prepare);
-}
 
-static void pci_dev_save_and_disable(struct pci_dev *dev)
-{
-	pci_reset_notify(dev, true);
+	if (err_handler && err_handler->reset_prepare)
+		err_handler->reset_prepare(dev);
 
 	/*
 	 * Wake-up device prior to save.  PM registers default to D0 after
@@ -4169,8 +4156,12 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
 
 static void pci_dev_restore(struct pci_dev *dev)
 {
+	const struct pci_error_handlers *err_handler =
+			dev->driver ? dev->driver->err_handler : NULL;
+
 	pci_restore_state(dev);
-	pci_reset_notify(dev, false);
+	if (err_handler && err_handler->reset_done)
+		err_handler->reset_done(dev);
 }
 
 static int pci_dev_reset(struct pci_dev *dev, int probe)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f0ca05..b87aec18e673 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -695,7 +695,8 @@ struct pci_error_handlers {
 	pci_ers_result_t (*slot_reset)(struct pci_dev *dev);
 
 	/* PCI function reset prepare or completed */
-	void (*reset_notify)(struct pci_dev *dev, bool prepare);
+	void (*reset_prepare)(struct pci_dev *dev);
+	void (*reset_done)(struct pci_dev *dev);
 
 	/* Device driver may resume normal operations */
 	void (*resume)(struct pci_dev *dev);
-- 
2.11.0

[PATCH 3/3] PCI: remove __pci_dev_reset and pci_dev_reset

[Christoph Hellwig]

And instead implement the reset probing / reset chain in
__pci_probe_reset_function and __pci_reset_function_locked respectively.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/pci.c | 108 ++++++++++++++++++++++++++----------------------------
 1 file changed, 52 insertions(+), 56 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b3f9ae7a4e21..20be3b87124a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4069,40 +4069,6 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
 	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
 }
 
-static int __pci_dev_reset(struct pci_dev *dev, int probe)
-{
-	int rc;
-
-	might_sleep();
-
-	rc = pci_dev_specific_reset(dev, probe);
-	if (rc != -ENOTTY)
-		goto done;
-
-	if (pcie_has_flr(dev)) {
-		if (!probe)
-			pcie_flr(dev);
-		rc = 0;
-		goto done;
-	}
-
-	rc = pci_af_flr(dev, probe);
-	if (rc != -ENOTTY)
-		goto done;
-
-	rc = pci_pm_reset(dev, probe);
-	if (rc != -ENOTTY)
-		goto done;
-
-	rc = pci_dev_reset_slot_function(dev, probe);
-	if (rc != -ENOTTY)
-		goto done;
-
-	rc = pci_parent_bus_reset(dev, probe);
-done:
-	return rc;
-}
-
 static void pci_dev_lock(struct pci_dev *dev)
 {
 	pci_cfg_access_lock(dev);
@@ -4164,21 +4130,6 @@ static void pci_dev_restore(struct pci_dev *dev)
 		err_handler->reset_done(dev);
 }
 
-static int pci_dev_reset(struct pci_dev *dev, int probe)
-{
-	int rc;
-
-	if (!probe)
-		pci_dev_lock(dev);
-
-	rc = __pci_dev_reset(dev, probe);
-
-	if (!probe)
-		pci_dev_unlock(dev);
-
-	return rc;
-}
-
 /**
  * __pci_reset_function - reset a PCI device function
  * @dev: PCI device to reset
@@ -4198,7 +4149,13 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
  */
 int __pci_reset_function(struct pci_dev *dev)
 {
-	return pci_dev_reset(dev, 0);
+	int ret;
+
+	pci_dev_lock(dev);
+	ret = __pci_reset_function_locked(dev);
+	pci_dev_unlock(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__pci_reset_function);
 
@@ -4223,7 +4180,27 @@ EXPORT_SYMBOL_GPL(__pci_reset_function);
  */
 int __pci_reset_function_locked(struct pci_dev *dev)
 {
-	return __pci_dev_reset(dev, 0);
+	int rc;
+
+	might_sleep();
+
+	rc = pci_dev_specific_reset(dev, 0);
+	if (rc != -ENOTTY)
+		return rc;
+	if (pcie_has_flr(dev)) {
+		pcie_flr(dev);
+		return 0;
+	}
+	rc = pci_af_flr(dev, 0);
+	if (rc != -ENOTTY)
+		return rc;
+	rc = pci_pm_reset(dev, 0);
+	if (rc != -ENOTTY)
+		return rc;
+	rc = pci_dev_reset_slot_function(dev, 0);
+	if (rc != -ENOTTY)
+		return rc;
+	return pci_parent_bus_reset(dev, 0);
 }
 EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
 
@@ -4240,7 +4217,26 @@ EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
  */
 int pci_probe_reset_function(struct pci_dev *dev)
 {
-	return pci_dev_reset(dev, 1);
+	int rc;
+
+	might_sleep();
+
+	rc = pci_dev_specific_reset(dev, 1);
+	if (rc != -ENOTTY)
+		return rc;
+	if (pcie_has_flr(dev))
+		return 0;
+	rc = pci_af_flr(dev, 1);
+	if (rc != -ENOTTY)
+		return rc;
+	rc = pci_pm_reset(dev, 1);
+	if (rc != -ENOTTY)
+		return rc;
+	rc = pci_dev_reset_slot_function(dev, 1);
+	if (rc != -ENOTTY)
+		return rc;
+
+	return pci_parent_bus_reset(dev, 1);
 }
 
 /**
@@ -4263,14 +4259,14 @@ int pci_reset_function(struct pci_dev *dev)
 {
 	int rc;
 
-	rc = pci_dev_reset(dev, 1);
+	rc = pci_probe_reset_function(dev);
 	if (rc)
 		return rc;
 
 	pci_dev_lock(dev);
 	pci_dev_save_and_disable(dev);
 
-	rc = __pci_dev_reset(dev, 0);
+	rc = __pci_reset_function_locked(dev);
 
 	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
@@ -4289,7 +4285,7 @@ int pci_try_reset_function(struct pci_dev *dev)
 {
 	int rc;
 
-	rc = pci_dev_reset(dev, 1);
+	rc = pci_probe_reset_function(dev);
 	if (rc)
 		return rc;
 
@@ -4297,7 +4293,7 @@ int pci_try_reset_function(struct pci_dev *dev)
 		return -EAGAIN;
 
 	pci_dev_save_and_disable(dev);
-	rc = __pci_dev_reset(dev, 0);
+	rc = __pci_reset_function_locked(dev);
 	pci_dev_unlock(dev);
 
 	pci_dev_restore(dev);
-- 
2.11.0

[PATCH 3/3] PCI: remove __pci_dev_reset and pci_dev_reset

[Christoph Hellwig]

And instead implement the reset probing / reset chain in
__pci_probe_reset_function and __pci_reset_function_locked respectively.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/pci/pci.c | 108 ++++++++++++++++++++++++++----------------------------
 1 file changed, 52 insertions(+), 56 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b3f9ae7a4e21..20be3b87124a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4069,40 +4069,6 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
 	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
 }
 
-static int __pci_dev_reset(struct pci_dev *dev, int probe)
-{
-	int rc;
-
-	might_sleep();
-
-	rc = pci_dev_specific_reset(dev, probe);
-	if (rc != -ENOTTY)
-		goto done;
-
-	if (pcie_has_flr(dev)) {
-		if (!probe)
-			pcie_flr(dev);
-		rc = 0;
-		goto done;
-	}
-
-	rc = pci_af_flr(dev, probe);
-	if (rc != -ENOTTY)
-		goto done;
-
-	rc = pci_pm_reset(dev, probe);
-	if (rc != -ENOTTY)
-		goto done;
-
-	rc = pci_dev_reset_slot_function(dev, probe);
-	if (rc != -ENOTTY)
-		goto done;
-
-	rc = pci_parent_bus_reset(dev, probe);
-done:
-	return rc;
-}
-
 static void pci_dev_lock(struct pci_dev *dev)
 {
 	pci_cfg_access_lock(dev);
@@ -4164,21 +4130,6 @@ static void pci_dev_restore(struct pci_dev *dev)
 		err_handler->reset_done(dev);
 }
 
-static int pci_dev_reset(struct pci_dev *dev, int probe)
-{
-	int rc;
-
-	if (!probe)
-		pci_dev_lock(dev);
-
-	rc = __pci_dev_reset(dev, probe);
-
-	if (!probe)
-		pci_dev_unlock(dev);
-
-	return rc;
-}
-
 /**
  * __pci_reset_function - reset a PCI device function
  * @dev: PCI device to reset
@@ -4198,7 +4149,13 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
  */
 int __pci_reset_function(struct pci_dev *dev)
 {
-	return pci_dev_reset(dev, 0);
+	int ret;
+
+	pci_dev_lock(dev);
+	ret = __pci_reset_function_locked(dev);
+	pci_dev_unlock(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__pci_reset_function);
 
@@ -4223,7 +4180,27 @@ EXPORT_SYMBOL_GPL(__pci_reset_function);
  */
 int __pci_reset_function_locked(struct pci_dev *dev)
 {
-	return __pci_dev_reset(dev, 0);
+	int rc;
+
+	might_sleep();
+
+	rc = pci_dev_specific_reset(dev, 0);
+	if (rc != -ENOTTY)
+		return rc;
+	if (pcie_has_flr(dev)) {
+		pcie_flr(dev);
+		return 0;
+	}
+	rc = pci_af_flr(dev, 0);
+	if (rc != -ENOTTY)
+		return rc;
+	rc = pci_pm_reset(dev, 0);
+	if (rc != -ENOTTY)
+		return rc;
+	rc = pci_dev_reset_slot_function(dev, 0);
+	if (rc != -ENOTTY)
+		return rc;
+	return pci_parent_bus_reset(dev, 0);
 }
 EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
 
@@ -4240,7 +4217,26 @@ EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
  */
 int pci_probe_reset_function(struct pci_dev *dev)
 {
-	return pci_dev_reset(dev, 1);
+	int rc;
+
+	might_sleep();
+
+	rc = pci_dev_specific_reset(dev, 1);
+	if (rc != -ENOTTY)
+		return rc;
+	if (pcie_has_flr(dev))
+		return 0;
+	rc = pci_af_flr(dev, 1);
+	if (rc != -ENOTTY)
+		return rc;
+	rc = pci_pm_reset(dev, 1);
+	if (rc != -ENOTTY)
+		return rc;
+	rc = pci_dev_reset_slot_function(dev, 1);
+	if (rc != -ENOTTY)
+		return rc;
+
+	return pci_parent_bus_reset(dev, 1);
 }
 
 /**
@@ -4263,14 +4259,14 @@ int pci_reset_function(struct pci_dev *dev)
 {
 	int rc;
 
-	rc = pci_dev_reset(dev, 1);
+	rc = pci_probe_reset_function(dev);
 	if (rc)
 		return rc;
 
 	pci_dev_lock(dev);
 	pci_dev_save_and_disable(dev);
 
-	rc = __pci_dev_reset(dev, 0);
+	rc = __pci_reset_function_locked(dev);
 
 	pci_dev_restore(dev);
 	pci_dev_unlock(dev);
@@ -4289,7 +4285,7 @@ int pci_try_reset_function(struct pci_dev *dev)
 {
 	int rc;
 
-	rc = pci_dev_reset(dev, 1);
+	rc = pci_probe_reset_function(dev);
 	if (rc)
 		return rc;
 
@@ -4297,7 +4293,7 @@ int pci_try_reset_function(struct pci_dev *dev)
 		return -EAGAIN;
 
 	pci_dev_save_and_disable(dev);
-	rc = __pci_dev_reset(dev, 0);
+	rc = __pci_reset_function_locked(dev);
 	pci_dev_unlock(dev);
 
 	pci_dev_restore(dev);
-- 
2.11.0

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Bjorn Helgaas]

On Thu, Jun 01, 2017 at 01:10:37PM +0200, Christoph Hellwig wrote:
> Without this ->notify_reset instance may race with ->remove calls,

s/notify_reset/reset_notify/

> which can be easily triggered in NVMe.

OK, sorry to be dense; it's taking me a long time to work out the
details here.  It feels like there should be a general principle to
help figure out where we need locking, and it would be really awesome
if we could include that in the changelog.  But it's not obvious to me
what that principle would be.

I think the two racing paths are these:

PATH 1 (write to sysfs "reset" file):

  sysfs_kf_write                      # <-- A (sysfs write)
    reset_store
      pci_reset_function
        pci_dev_lock                  # <-- patch moves lock here
          device_lock
        pci_dev_save_and_disable
          pci_reset_notify(dev, true)
            err_handler->reset_notify
              nvme_reset_notify       # nvme_err_handler.reset_notify
                nvme_dev_disable      # prepare == true
                  # shutdown == false
                  nvme_pci_disable
          pci_save_state
        pci_dev_reset
          pci_dev_lock                # <-- lock was originally here
          __pci_dev_reset
            pcie_flr                  # <-- B (issue reset)
          pci_dev_unlock              # <-- unlock was originally here
        pci_dev_restore
          pci_restore_state
          pci_reset_notify(dev, false)
            err_handler->reset_notify
              nvme_reset_notify       # nvme_err_handler.reset_notify
                dev = pci_get_drvdata(pdev)   # <-- F (dev == NULL)
                nvme_reset            # prepare == false
                  queue_work(..., &dev->reset_work)   # nvme_reset_work
        pci_dev_unlock                # <-- patch moves unlock here

  ...
  nvme_reset_work
    nvme_remove_dead_ctrl
      nvme_dev_disable
        if (!schedule_work(&dev->remove_work)) # nvme_remove_dead_ctrl_work
          nvme_put_ctrl

  ...
  nvme_remove_dead_ctrl_work
    if (pci_get_drvdata(pdev))
      device_release_driver(&pdev->dev)
        ...
          __device_release_driver
            drv->remove
              nvme_remove
                pci_set_drvdata(pdev, NULL)

PATH 2 (AER recovery):

  do_recovery                         # <-- C (AER interrupt)
    if (severity == AER_FATAL)
      state = pci_channel_io_frozen
    status = broadcast_error_message(..., report_error_detected)
      pci_walk_bus
        report_error_detected
          err_handler->error_detected
            nvme_error_detected
              return PCI_ERS_RESULT_NEED_RESET        # frozen case
    # status == PCI_ERS_RESULT_NEED_RESET
    if (severity == AER_FATAL)
      reset_link
    if (status == PCI_ERS_RESULT_NEED_RESET)
      broadcast_error_message(..., report_slot_reset)
        pci_walk_bus
          report_slot_reset
            device_lock               # <-- D (acquire device lock)
            err_handler->slot_reset
              nvme_slot_reset
                nvme_reset
                  queue_work(..., &dev->reset_work)   # nvme_reset_work
            device_lock               # <-- unlock

  ...
  nvme_reset_work
    ...
      schedule_work(&dev->remove_work)  # nvme_remove_dead_ctrl_work

  ...
  nvme_remove_dead_ctrl_work
    ...
      drv->remove
        nvme_remove                     # <-- E (driver remove() method)
          pci_set_drvdata(pdev, NULL)

So the critical point is that with the current code, we can have this
sequence:

  A sysfs write occurs
  B sysfs write thread issues FLR to device
  C AER thread takes an interrupt as a result of the FLR
  D AER thread acquires device lock
  E AER thread calls the driver remove() method and clears drvdata
  F sysfs write thread reads drvdata which is now NULL

The AER thread acquires the device lock before calling
err_handler->slot_reset, and this patch makes the sysfs thread hold
the lock until after it has read drvdata, so the patch avoids the NULL
pointer dereference at F by changing the sequence to this:

  A sysfs write occurs
  B sysfs write thread issues FLR to device
  C AER thread takes an interrupt as a result of the FLR
  F sysfs write thread reads drvdata
  D AER thread acquires device lock
  E AER thread calls the driver remove() method and clears drvdata

But I'm still nervous because I think both threads will queue
nvme_reset_work() work items for the same device, and I'm not sure
they're prepared to run concurrently.

I don't really think it should be the driver's responsibility to
understand issues like this and worry about things like
nvme_reset_work() running concurrently.  So I'm thinking maybe the PCI
core needs to be a little stricter here, but I don't know exactly
*how*.

What do you think?

Bjorn

> Reported-by: Rakesh Pandit <rakesh@tuxera.com>
> Tested-by: Rakesh Pandit <rakesh@tuxera.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/pci.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 563901cd9c06..92f7e5ae2e5e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4276,11 +4276,13 @@ int pci_reset_function(struct pci_dev *dev)
>  	if (rc)
>  		return rc;
>  
> +	pci_dev_lock(dev);
>  	pci_dev_save_and_disable(dev);
>  
> -	rc = pci_dev_reset(dev, 0);
> +	rc = __pci_dev_reset(dev, 0);
>  
>  	pci_dev_restore(dev);
> +	pci_dev_unlock(dev);
>  
>  	return rc;
>  }
> @@ -4300,16 +4302,14 @@ int pci_try_reset_function(struct pci_dev *dev)
>  	if (rc)
>  		return rc;
>  
> -	pci_dev_save_and_disable(dev);
> +	if (pci_dev_trylock(dev))
> +		return -EAGAIN;
>  
> -	if (pci_dev_trylock(dev)) {
> -		rc = __pci_dev_reset(dev, 0);
> -		pci_dev_unlock(dev);
> -	} else
> -		rc = -EAGAIN;
> +	pci_dev_save_and_disable(dev);
> +	rc = __pci_dev_reset(dev, 0);
> +	pci_dev_unlock(dev);
>  
>  	pci_dev_restore(dev);
> -
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(pci_try_reset_function);
> @@ -4459,7 +4459,9 @@ static void pci_bus_save_and_disable(struct pci_bus *bus)
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		pci_dev_lock(dev);
>  		pci_dev_save_and_disable(dev);
> +		pci_dev_unlock(dev);
>  		if (dev->subordinate)
>  			pci_bus_save_and_disable(dev->subordinate);
>  	}
> @@ -4474,7 +4476,9 @@ static void pci_bus_restore(struct pci_bus *bus)
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		pci_dev_lock(dev);
>  		pci_dev_restore(dev);
> +		pci_dev_unlock(dev);
>  		if (dev->subordinate)
>  			pci_bus_restore(dev->subordinate);
>  	}
> -- 
> 2.11.0
> 

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Bjorn Helgaas]

On Thu, Jun 01, 2017@01:10:37PM +0200, Christoph Hellwig wrote:
> Without this ->notify_reset instance may race with ->remove calls,

s/notify_reset/reset_notify/

> which can be easily triggered in NVMe.

OK, sorry to be dense; it's taking me a long time to work out the
details here.  It feels like there should be a general principle to
help figure out where we need locking, and it would be really awesome
if we could include that in the changelog.  But it's not obvious to me
what that principle would be.

I think the two racing paths are these:

PATH 1 (write to sysfs "reset" file):

  sysfs_kf_write                      # <-- A (sysfs write)
    reset_store
      pci_reset_function
        pci_dev_lock                  # <-- patch moves lock here
          device_lock
        pci_dev_save_and_disable
          pci_reset_notify(dev, true)
            err_handler->reset_notify
              nvme_reset_notify       # nvme_err_handler.reset_notify
                nvme_dev_disable      # prepare == true
                  # shutdown == false
                  nvme_pci_disable
          pci_save_state
        pci_dev_reset
          pci_dev_lock                # <-- lock was originally here
          __pci_dev_reset
            pcie_flr                  # <-- B (issue reset)
          pci_dev_unlock              # <-- unlock was originally here
        pci_dev_restore
          pci_restore_state
          pci_reset_notify(dev, false)
            err_handler->reset_notify
              nvme_reset_notify       # nvme_err_handler.reset_notify
                dev = pci_get_drvdata(pdev)   # <-- F (dev == NULL)
                nvme_reset            # prepare == false
                  queue_work(..., &dev->reset_work)   # nvme_reset_work
        pci_dev_unlock                # <-- patch moves unlock here

  ...
  nvme_reset_work
    nvme_remove_dead_ctrl
      nvme_dev_disable
        if (!schedule_work(&dev->remove_work)) # nvme_remove_dead_ctrl_work
          nvme_put_ctrl

  ...
  nvme_remove_dead_ctrl_work
    if (pci_get_drvdata(pdev))
      device_release_driver(&pdev->dev)
        ...
          __device_release_driver
            drv->remove
              nvme_remove
                pci_set_drvdata(pdev, NULL)

PATH 2 (AER recovery):

  do_recovery                         # <-- C (AER interrupt)
    if (severity == AER_FATAL)
      state = pci_channel_io_frozen
    status = broadcast_error_message(..., report_error_detected)
      pci_walk_bus
        report_error_detected
          err_handler->error_detected
            nvme_error_detected
              return PCI_ERS_RESULT_NEED_RESET        # frozen case
    # status == PCI_ERS_RESULT_NEED_RESET
    if (severity == AER_FATAL)
      reset_link
    if (status == PCI_ERS_RESULT_NEED_RESET)
      broadcast_error_message(..., report_slot_reset)
        pci_walk_bus
          report_slot_reset
            device_lock               # <-- D (acquire device lock)
            err_handler->slot_reset
              nvme_slot_reset
                nvme_reset
                  queue_work(..., &dev->reset_work)   # nvme_reset_work
            device_lock               # <-- unlock

  ...
  nvme_reset_work
    ...
      schedule_work(&dev->remove_work)  # nvme_remove_dead_ctrl_work

  ...
  nvme_remove_dead_ctrl_work
    ...
      drv->remove
        nvme_remove                     # <-- E (driver remove() method)
          pci_set_drvdata(pdev, NULL)

So the critical point is that with the current code, we can have this
sequence:

  A sysfs write occurs
  B sysfs write thread issues FLR to device
  C AER thread takes an interrupt as a result of the FLR
  D AER thread acquires device lock
  E AER thread calls the driver remove() method and clears drvdata
  F sysfs write thread reads drvdata which is now NULL

The AER thread acquires the device lock before calling
err_handler->slot_reset, and this patch makes the sysfs thread hold
the lock until after it has read drvdata, so the patch avoids the NULL
pointer dereference at F by changing the sequence to this:

  A sysfs write occurs
  B sysfs write thread issues FLR to device
  C AER thread takes an interrupt as a result of the FLR
  F sysfs write thread reads drvdata
  D AER thread acquires device lock
  E AER thread calls the driver remove() method and clears drvdata

But I'm still nervous because I think both threads will queue
nvme_reset_work() work items for the same device, and I'm not sure
they're prepared to run concurrently.

I don't really think it should be the driver's responsibility to
understand issues like this and worry about things like
nvme_reset_work() running concurrently.  So I'm thinking maybe the PCI
core needs to be a little stricter here, but I don't know exactly
*how*.

What do you think?

Bjorn

> Reported-by: Rakesh Pandit <rakesh at tuxera.com>
> Tested-by: Rakesh Pandit <rakesh at tuxera.com>
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>  drivers/pci/pci.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 563901cd9c06..92f7e5ae2e5e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4276,11 +4276,13 @@ int pci_reset_function(struct pci_dev *dev)
>  	if (rc)
>  		return rc;
>  
> +	pci_dev_lock(dev);
>  	pci_dev_save_and_disable(dev);
>  
> -	rc = pci_dev_reset(dev, 0);
> +	rc = __pci_dev_reset(dev, 0);
>  
>  	pci_dev_restore(dev);
> +	pci_dev_unlock(dev);
>  
>  	return rc;
>  }
> @@ -4300,16 +4302,14 @@ int pci_try_reset_function(struct pci_dev *dev)
>  	if (rc)
>  		return rc;
>  
> -	pci_dev_save_and_disable(dev);
> +	if (pci_dev_trylock(dev))
> +		return -EAGAIN;
>  
> -	if (pci_dev_trylock(dev)) {
> -		rc = __pci_dev_reset(dev, 0);
> -		pci_dev_unlock(dev);
> -	} else
> -		rc = -EAGAIN;
> +	pci_dev_save_and_disable(dev);
> +	rc = __pci_dev_reset(dev, 0);
> +	pci_dev_unlock(dev);
>  
>  	pci_dev_restore(dev);
> -
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(pci_try_reset_function);
> @@ -4459,7 +4459,9 @@ static void pci_bus_save_and_disable(struct pci_bus *bus)
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		pci_dev_lock(dev);
>  		pci_dev_save_and_disable(dev);
> +		pci_dev_unlock(dev);
>  		if (dev->subordinate)
>  			pci_bus_save_and_disable(dev->subordinate);
>  	}
> @@ -4474,7 +4476,9 @@ static void pci_bus_restore(struct pci_bus *bus)
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		pci_dev_lock(dev);
>  		pci_dev_restore(dev);
> +		pci_dev_unlock(dev);
>  		if (dev->subordinate)
>  			pci_bus_restore(dev->subordinate);
>  	}
> -- 
> 2.11.0
> 

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Marta Rybczynska]

> 
> But I'm still nervous because I think both threads will queue
> nvme_reset_work() work items for the same device, and I'm not sure
> they're prepared to run concurrently.
> 
> I don't really think it should be the driver's responsibility to
> understand issues like this and worry about things like
> nvme_reset_work() running concurrently.  So I'm thinking maybe the PCI
> core needs to be a little stricter here, but I don't know exactly
> *how*.
> 
> What do you think?

>From what I can see the nvme_reset_work if run twice may disable the
controller (the out label) if run concurrently. If run twice it will
initialize twice what isn't best either.

I think that the double nvme_reset_work should be prevented. Maybe
a state information saying that the device is in the reset procedure
and after that run nvme_reset_work just once?

Marta

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Marta Rybczynska]

> 
> But I'm still nervous because I think both threads will queue
> nvme_reset_work() work items for the same device, and I'm not sure
> they're prepared to run concurrently.
> 
> I don't really think it should be the driver's responsibility to
> understand issues like this and worry about things like
> nvme_reset_work() running concurrently.  So I'm thinking maybe the PCI
> core needs to be a little stricter here, but I don't know exactly
> *how*.
> 
> What do you think?

>From what I can see the nvme_reset_work if run twice may disable the
controller (the out label) if run concurrently. If run twice it will
initialize twice what isn't best either.

I think that the double nvme_reset_work should be prevented. Maybe
a state information saying that the device is in the reset procedure
and after that run nvme_reset_work just once?

Marta

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Christoph Hellwig]

On Tue, Jun 06, 2017 at 12:31:42AM -0500, Bjorn Helgaas wrote:
> OK, sorry to be dense; it's taking me a long time to work out the
> details here.  It feels like there should be a general principle to
> help figure out where we need locking, and it would be really awesome
> if we could include that in the changelog.  But it's not obvious to me
> what that principle would be.

The principle is very simple: every method in struct device_driver
or structures derived from it like struct pci_driver MUST provide
exclusion vs ->remove.  Usuaull by using device_lock().

If we don't provide such an exclusion the method call can race with
a removal in one form or another.

> But I'm still nervous because I think both threads will queue
> nvme_reset_work() work items for the same device, and I'm not sure
> they're prepared to run concurrently.

We had another bug in that area, and the fix for that is hopefully
going to go into the next 4.12-rc.

> I don't really think it should be the driver's responsibility to
> understand issues like this and worry about things like
> nvme_reset_work() running concurrently.  So I'm thinking maybe the PCI
> core needs to be a little stricter here, but I don't know exactly
> *how*.
> 
> What do you think?

The driver core / bus driver must ensure that method calls don't
race with ->remove.  There is nothing the driver can do about it,
and the race is just as possible with explicit user removals or
hardware hotplug.

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Christoph Hellwig]

On Tue, Jun 06, 2017@12:31:42AM -0500, Bjorn Helgaas wrote:
> OK, sorry to be dense; it's taking me a long time to work out the
> details here.  It feels like there should be a general principle to
> help figure out where we need locking, and it would be really awesome
> if we could include that in the changelog.  But it's not obvious to me
> what that principle would be.

The principle is very simple: every method in struct device_driver
or structures derived from it like struct pci_driver MUST provide
exclusion vs ->remove.  Usuaull by using device_lock().

If we don't provide such an exclusion the method call can race with
a removal in one form or another.

> But I'm still nervous because I think both threads will queue
> nvme_reset_work() work items for the same device, and I'm not sure
> they're prepared to run concurrently.

We had another bug in that area, and the fix for that is hopefully
going to go into the next 4.12-rc.

> I don't really think it should be the driver's responsibility to
> understand issues like this and worry about things like
> nvme_reset_work() running concurrently.  So I'm thinking maybe the PCI
> core needs to be a little stricter here, but I don't know exactly
> *how*.
> 
> What do you think?

The driver core / bus driver must ensure that method calls don't
race with ->remove.  There is nothing the driver can do about it,
and the race is just as possible with explicit user removals or
hardware hotplug.

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Bjorn Helgaas]

On Tue, Jun 06, 2017 at 12:48:36PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 06, 2017 at 12:31:42AM -0500, Bjorn Helgaas wrote:
> > OK, sorry to be dense; it's taking me a long time to work out the
> > details here.  It feels like there should be a general principle to
> > help figure out where we need locking, and it would be really awesome
> > if we could include that in the changelog.  But it's not obvious to me
> > what that principle would be.
> 
> The principle is very simple: every method in struct device_driver
> or structures derived from it like struct pci_driver MUST provide
> exclusion vs ->remove.  Usuaull by using device_lock().
> 
> If we don't provide such an exclusion the method call can race with
> a removal in one form or another.

So I guess the method here is
dev->driver->err_handler->reset_notify(), and the PCI core should be
holding device_lock() while calling it?  That makes sense to me;
thanks a lot for articulating that!

1) The current patch protects the err_handler->reset_notify() uses by
adding or expanding device_lock regions in the paths that lead to
pci_reset_notify().  Could we simplify it by doing the locking
directly in pci_reset_notify()?  Then it would be easy to verify the
locking, and we would be less likely to add new callers without the
proper locking.

2) Stating the rule explicitly helps look for other problems, and I
think we have a similar problem in all the pcie_portdrv_err_handler
methods.  These are all called in the AER do_recovery() path, and the
functions there, e.g., report_error_detected() do hold device_lock().
But pcie_portdrv_error_detected() propagates this to all the children,
and we *don't* hold the lock for the children.

Bjorn

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Bjorn Helgaas]

On Tue, Jun 06, 2017@12:48:36PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 06, 2017@12:31:42AM -0500, Bjorn Helgaas wrote:
> > OK, sorry to be dense; it's taking me a long time to work out the
> > details here.  It feels like there should be a general principle to
> > help figure out where we need locking, and it would be really awesome
> > if we could include that in the changelog.  But it's not obvious to me
> > what that principle would be.
> 
> The principle is very simple: every method in struct device_driver
> or structures derived from it like struct pci_driver MUST provide
> exclusion vs ->remove.  Usuaull by using device_lock().
> 
> If we don't provide such an exclusion the method call can race with
> a removal in one form or another.

So I guess the method here is
dev->driver->err_handler->reset_notify(), and the PCI core should be
holding device_lock() while calling it?  That makes sense to me;
thanks a lot for articulating that!

1) The current patch protects the err_handler->reset_notify() uses by
adding or expanding device_lock regions in the paths that lead to
pci_reset_notify().  Could we simplify it by doing the locking
directly in pci_reset_notify()?  Then it would be easy to verify the
locking, and we would be less likely to add new callers without the
proper locking.

2) Stating the rule explicitly helps look for other problems, and I
think we have a similar problem in all the pcie_portdrv_err_handler
methods.  These are all called in the AER do_recovery() path, and the
functions there, e.g., report_error_detected() do hold device_lock().
But pcie_portdrv_error_detected() propagates this to all the children,
and we *don't* hold the lock for the children.

Bjorn

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Christoph Hellwig]

On Tue, Jun 06, 2017 at 04:14:43PM -0500, Bjorn Helgaas wrote:
> So I guess the method here is
> dev->driver->err_handler->reset_notify(), and the PCI core should be
> holding device_lock() while calling it?  That makes sense to me;
> thanks a lot for articulating that!

Yes.

> 1) The current patch protects the err_handler->reset_notify() uses by
> adding or expanding device_lock regions in the paths that lead to
> pci_reset_notify().  Could we simplify it by doing the locking
> directly in pci_reset_notify()?  Then it would be easy to verify the
> locking, and we would be less likely to add new callers without the
> proper locking.

We could do that, except that I'd rather hold the lock over a longer
period if we have many calls following each other.  I also have
a patch to actually kill pci_reset_notify() later in the series as
well, as the calling convention for it and ->reset_notify() are
awkward - depending on prepare parameter they do two entirely
different things.  That being said I could also add new
pci_reset_prepare() and pci_reset_done() helpers.

> 2) Stating the rule explicitly helps look for other problems, and I
> think we have a similar problem in all the pcie_portdrv_err_handler
> methods.

Yes, I mentioned this earlier, and I also vaguely remember we got
bug reports from IBM on power for this a while ago.  I just don't
feel confident enough to touch all these without a good test plan.

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Christoph Hellwig]

On Tue, Jun 06, 2017@04:14:43PM -0500, Bjorn Helgaas wrote:
> So I guess the method here is
> dev->driver->err_handler->reset_notify(), and the PCI core should be
> holding device_lock() while calling it?  That makes sense to me;
> thanks a lot for articulating that!

Yes.

> 1) The current patch protects the err_handler->reset_notify() uses by
> adding or expanding device_lock regions in the paths that lead to
> pci_reset_notify().  Could we simplify it by doing the locking
> directly in pci_reset_notify()?  Then it would be easy to verify the
> locking, and we would be less likely to add new callers without the
> proper locking.

We could do that, except that I'd rather hold the lock over a longer
period if we have many calls following each other.  I also have
a patch to actually kill pci_reset_notify() later in the series as
well, as the calling convention for it and ->reset_notify() are
awkward - depending on prepare parameter they do two entirely
different things.  That being said I could also add new
pci_reset_prepare() and pci_reset_done() helpers.

> 2) Stating the rule explicitly helps look for other problems, and I
> think we have a similar problem in all the pcie_portdrv_err_handler
> methods.

Yes, I mentioned this earlier, and I also vaguely remember we got
bug reports from IBM on power for this a while ago.  I just don't
feel confident enough to touch all these without a good test plan.

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Bjorn Helgaas]

On Wed, Jun 07, 2017 at 08:29:36PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 06, 2017 at 04:14:43PM -0500, Bjorn Helgaas wrote:
> > So I guess the method here is
> > dev->driver->err_handler->reset_notify(), and the PCI core should be
> > holding device_lock() while calling it?  That makes sense to me;
> > thanks a lot for articulating that!
> 
> Yes.
> 
> > 1) The current patch protects the err_handler->reset_notify() uses by
> > adding or expanding device_lock regions in the paths that lead to
> > pci_reset_notify().  Could we simplify it by doing the locking
> > directly in pci_reset_notify()?  Then it would be easy to verify the
> > locking, and we would be less likely to add new callers without the
> > proper locking.
> 
> We could do that, except that I'd rather hold the lock over a longer
> period if we have many calls following each other.  

My main concern is being able to verify the locking.  I think that is
much easier if the locking is adjacent to the method invocation.  But
if you just add a comment at the method invocation about where the
locking is, that should be sufficient.

> I also have
> a patch to actually kill pci_reset_notify() later in the series as
> well, as the calling convention for it and ->reset_notify() are
> awkward - depending on prepare parameter they do two entirely
> different things.  That being said I could also add new
> pci_reset_prepare() and pci_reset_done() helpers.

I like your pci_reset_notify() changes; they make that much clearer.
I don't think new helpers are necessary.

> > 2) Stating the rule explicitly helps look for other problems, and I
> > think we have a similar problem in all the pcie_portdrv_err_handler
> > methods.
> 
> Yes, I mentioned this earlier, and I also vaguely remember we got
> bug reports from IBM on power for this a while ago.  I just don't
> feel confident enough to touch all these without a good test plan.

Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
wonder if some enterprising soul could tickle this bug by injecting
errors while removing and rescanning devices below the bridge?

Bjorn

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Bjorn Helgaas]

On Wed, Jun 07, 2017@08:29:36PM +0200, Christoph Hellwig wrote:
> On Tue, Jun 06, 2017@04:14:43PM -0500, Bjorn Helgaas wrote:
> > So I guess the method here is
> > dev->driver->err_handler->reset_notify(), and the PCI core should be
> > holding device_lock() while calling it?  That makes sense to me;
> > thanks a lot for articulating that!
> 
> Yes.
> 
> > 1) The current patch protects the err_handler->reset_notify() uses by
> > adding or expanding device_lock regions in the paths that lead to
> > pci_reset_notify().  Could we simplify it by doing the locking
> > directly in pci_reset_notify()?  Then it would be easy to verify the
> > locking, and we would be less likely to add new callers without the
> > proper locking.
> 
> We could do that, except that I'd rather hold the lock over a longer
> period if we have many calls following each other.  

My main concern is being able to verify the locking.  I think that is
much easier if the locking is adjacent to the method invocation.  But
if you just add a comment at the method invocation about where the
locking is, that should be sufficient.

> I also have
> a patch to actually kill pci_reset_notify() later in the series as
> well, as the calling convention for it and ->reset_notify() are
> awkward - depending on prepare parameter they do two entirely
> different things.  That being said I could also add new
> pci_reset_prepare() and pci_reset_done() helpers.

I like your pci_reset_notify() changes; they make that much clearer.
I don't think new helpers are necessary.

> > 2) Stating the rule explicitly helps look for other problems, and I
> > think we have a similar problem in all the pcie_portdrv_err_handler
> > methods.
> 
> Yes, I mentioned this earlier, and I also vaguely remember we got
> bug reports from IBM on power for this a while ago.  I just don't
> feel confident enough to touch all these without a good test plan.

Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
wonder if some enterprising soul could tickle this bug by injecting
errors while removing and rescanning devices below the bridge?

Bjorn

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Christoph Hellwig]

On Mon, Jun 12, 2017 at 06:14:23PM -0500, Bjorn Helgaas wrote:
> My main concern is being able to verify the locking.  I think that is
> much easier if the locking is adjacent to the method invocation.  But
> if you just add a comment at the method invocation about where the
> locking is, that should be sufficient.

Ok.  I can add comments for all the methods as a separate patch,
similar to Documentation/vfs/Locking

> > Yes, I mentioned this earlier, and I also vaguely remember we got
> > bug reports from IBM on power for this a while ago.  I just don't
> > feel confident enough to touch all these without a good test plan.
> 
> Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
> wonder if some enterprising soul could tickle this bug by injecting
> errors while removing and rescanning devices below the bridge?

I'm completely loaded up at the moment, but this sounds like a good
idea.

In the meantime how do you want to proceed with this patch?

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Christoph Hellwig]

On Mon, Jun 12, 2017@06:14:23PM -0500, Bjorn Helgaas wrote:
> My main concern is being able to verify the locking.  I think that is
> much easier if the locking is adjacent to the method invocation.  But
> if you just add a comment at the method invocation about where the
> locking is, that should be sufficient.

Ok.  I can add comments for all the methods as a separate patch,
similar to Documentation/vfs/Locking

> > Yes, I mentioned this earlier, and I also vaguely remember we got
> > bug reports from IBM on power for this a while ago.  I just don't
> > feel confident enough to touch all these without a good test plan.
> 
> Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
> wonder if some enterprising soul could tickle this bug by injecting
> errors while removing and rescanning devices below the bridge?

I'm completely loaded up at the moment, but this sounds like a good
idea.

In the meantime how do you want to proceed with this patch?

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Bjorn Helgaas]

On Tue, Jun 13, 2017 at 09:08:10AM +0200, Christoph Hellwig wrote:
> On Mon, Jun 12, 2017 at 06:14:23PM -0500, Bjorn Helgaas wrote:
> > My main concern is being able to verify the locking.  I think that is
> > much easier if the locking is adjacent to the method invocation.  But
> > if you just add a comment at the method invocation about where the
> > locking is, that should be sufficient.
> 
> Ok.  I can add comments for all the methods as a separate patch,
> similar to Documentation/vfs/Locking
> 
> > > Yes, I mentioned this earlier, and I also vaguely remember we got
> > > bug reports from IBM on power for this a while ago.  I just don't
> > > feel confident enough to touch all these without a good test plan.
> > 
> > Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
> > wonder if some enterprising soul could tickle this bug by injecting
> > errors while removing and rescanning devices below the bridge?
> 
> I'm completely loaded up at the moment, but this sounds like a good
> idea.
> 
> In the meantime how do you want to proceed with this patch?

Can you just add comments about the locking?  I'd prefer that in the
same patch that adds the locking because that's what I had a hard time
reviewing.  I'm not thinking of anything fancy like
Documentation/filesystems/Locking; I'm just thinking of something
along the lines of "caller must hold pci_dev_lock() to protect
err_handler->reset_notify from racing ->remove()".  And the changelog
should contain the general principle about the locking strategy.

Bjorn

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Bjorn Helgaas]

On Tue, Jun 13, 2017@09:08:10AM +0200, Christoph Hellwig wrote:
> On Mon, Jun 12, 2017@06:14:23PM -0500, Bjorn Helgaas wrote:
> > My main concern is being able to verify the locking.  I think that is
> > much easier if the locking is adjacent to the method invocation.  But
> > if you just add a comment at the method invocation about where the
> > locking is, that should be sufficient.
> 
> Ok.  I can add comments for all the methods as a separate patch,
> similar to Documentation/vfs/Locking
> 
> > > Yes, I mentioned this earlier, and I also vaguely remember we got
> > > bug reports from IBM on power for this a while ago.  I just don't
> > > feel confident enough to touch all these without a good test plan.
> > 
> > Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
> > wonder if some enterprising soul could tickle this bug by injecting
> > errors while removing and rescanning devices below the bridge?
> 
> I'm completely loaded up at the moment, but this sounds like a good
> idea.
> 
> In the meantime how do you want to proceed with this patch?

Can you just add comments about the locking?  I'd prefer that in the
same patch that adds the locking because that's what I had a hard time
reviewing.  I'm not thinking of anything fancy like
Documentation/filesystems/Locking; I'm just thinking of something
along the lines of "caller must hold pci_dev_lock() to protect
err_handler->reset_notify from racing ->remove()".  And the changelog
should contain the general principle about the locking strategy.

Bjorn

Re: avoid null pointer rereference during FLR V2

[Bjorn Helgaas]

On Thu, Jun 01, 2017 at 01:10:36PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> Rakesh reported a bug where a FLR can trivially crash his system.
> The reason for that is that NVMe unbinds the driver from the PCI device
> on an unrecoverable error, and that races with the reset_notify method.
> 
> This is fairly easily fixable by taking the device lock for a slightly
> longer period.  Note that the other PCI error handling methods actually
> have the same issue, but with them not taking the lock yet and me having
> no good way to reproducibly call them I'm a little reluctant to touch
> them, but it would be great if we could fix those issues as well.
> 
> Patches 2 and 3 are cleanups in the same area and not 4.12 material,
> but given that they depend on the first one I thought I'd send them
> along.
> 
> Changes since V1:
>  - lock over all calls to ->reset_notify

Applied all three (with some updated changelogs and comments) to
pci/virtualization for v4.13, thanks!

avoid null pointer rereference during FLR V2

[Bjorn Helgaas]

On Thu, Jun 01, 2017@01:10:36PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> Rakesh reported a bug where a FLR can trivially crash his system.
> The reason for that is that NVMe unbinds the driver from the PCI device
> on an unrecoverable error, and that races with the reset_notify method.
> 
> This is fairly easily fixable by taking the device lock for a slightly
> longer period.  Note that the other PCI error handling methods actually
> have the same issue, but with them not taking the lock yet and me having
> no good way to reproducibly call them I'm a little reluctant to touch
> them, but it would be great if we could fix those issues as well.
> 
> Patches 2 and 3 are cleanups in the same area and not 4.12 material,
> but given that they depend on the first one I thought I'd send them
> along.
> 
> Changes since V1:
>  - lock over all calls to ->reset_notify

Applied all three (with some updated changelogs and comments) to
pci/virtualization for v4.13, thanks!

Re: [PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Guilherme G. Piccoli]

On 06/12/2017 08:14 PM, Bjorn Helgaas wrote:
> On Wed, Jun 07, 2017 at 08:29:36PM +0200, Christoph Hellwig wrote:
>> On Tue, Jun 06, 2017 at 04:14:43PM -0500, Bjorn Helgaas wrote:
>>> So I guess the method here is
>>> dev->driver->err_handler->reset_notify(), and the PCI core should be
>>> holding device_lock() while calling it?  That makes sense to me;
>>> thanks a lot for articulating that!
>>
>> Yes.
>>
>>> 1) The current patch protects the err_handler->reset_notify() uses by
>>> adding or expanding device_lock regions in the paths that lead to
>>> pci_reset_notify().  Could we simplify it by doing the locking
>>> directly in pci_reset_notify()?  Then it would be easy to verify the
>>> locking, and we would be less likely to add new callers without the
>>> proper locking.
>>
>> We could do that, except that I'd rather hold the lock over a longer
>> period if we have many calls following each other.  
> 
> My main concern is being able to verify the locking.  I think that is
> much easier if the locking is adjacent to the method invocation.  But
> if you just add a comment at the method invocation about where the
> locking is, that should be sufficient.
> 
>> I also have
>> a patch to actually kill pci_reset_notify() later in the series as
>> well, as the calling convention for it and ->reset_notify() are
>> awkward - depending on prepare parameter they do two entirely
>> different things.  That being said I could also add new
>> pci_reset_prepare() and pci_reset_done() helpers.
> 
> I like your pci_reset_notify() changes; they make that much clearer.
> I don't think new helpers are necessary.
> 
>>> 2) Stating the rule explicitly helps look for other problems, and I
>>> think we have a similar problem in all the pcie_portdrv_err_handler
>>> methods.
>>
>> Yes, I mentioned this earlier, and I also vaguely remember we got
>> bug reports from IBM on power for this a while ago.  I just don't
>> feel confident enough to touch all these without a good test plan.
> 
> Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
> wonder if some enterprising soul could tickle this bug by injecting
> errors while removing and rescanning devices below the bridge?

Well, although I don't consider myself an enterprising soul...heheh
I can test it, just CC me in next spin and provide some comment on how
to test (or point me the thread of original report).

I guess it was myself the reporter of the issue, I tried a simple fix
for our case and Christoph mentioned issue was more generic and needed a
proper fix..

Hopefully this one is that fix!
Thanks,


Guilherme

> 
> Bjorn
> 

[PATCH 1/3] PCI: ensure the PCI device is locked over ->reset_notify calls

[Guilherme G. Piccoli]

On 06/12/2017 08:14 PM, Bjorn Helgaas wrote:
> On Wed, Jun 07, 2017@08:29:36PM +0200, Christoph Hellwig wrote:
>> On Tue, Jun 06, 2017@04:14:43PM -0500, Bjorn Helgaas wrote:
>>> So I guess the method here is
>>> dev->driver->err_handler->reset_notify(), and the PCI core should be
>>> holding device_lock() while calling it?  That makes sense to me;
>>> thanks a lot for articulating that!
>>
>> Yes.
>>
>>> 1) The current patch protects the err_handler->reset_notify() uses by
>>> adding or expanding device_lock regions in the paths that lead to
>>> pci_reset_notify().  Could we simplify it by doing the locking
>>> directly in pci_reset_notify()?  Then it would be easy to verify the
>>> locking, and we would be less likely to add new callers without the
>>> proper locking.
>>
>> We could do that, except that I'd rather hold the lock over a longer
>> period if we have many calls following each other.  
> 
> My main concern is being able to verify the locking.  I think that is
> much easier if the locking is adjacent to the method invocation.  But
> if you just add a comment at the method invocation about where the
> locking is, that should be sufficient.
> 
>> I also have
>> a patch to actually kill pci_reset_notify() later in the series as
>> well, as the calling convention for it and ->reset_notify() are
>> awkward - depending on prepare parameter they do two entirely
>> different things.  That being said I could also add new
>> pci_reset_prepare() and pci_reset_done() helpers.
> 
> I like your pci_reset_notify() changes; they make that much clearer.
> I don't think new helpers are necessary.
> 
>>> 2) Stating the rule explicitly helps look for other problems, and I
>>> think we have a similar problem in all the pcie_portdrv_err_handler
>>> methods.
>>
>> Yes, I mentioned this earlier, and I also vaguely remember we got
>> bug reports from IBM on power for this a while ago.  I just don't
>> feel confident enough to touch all these without a good test plan.
> 
> Hmmm.  I see your point, but I hate leaving a known bug unfixed.  I
> wonder if some enterprising soul could tickle this bug by injecting
> errors while removing and rescanning devices below the bridge?

Well, although I don't consider myself an enterprising soul...heheh
I can test it, just CC me in next spin and provide some comment on how
to test (or point me the thread of original report).

I guess it was myself the reporter of the issue, I tried a simple fix
for our case and Christoph mentioned issue was more generic and needed a
proper fix..

Hopefully this one is that fix!
Thanks,


Guilherme

> 
> Bjorn
>