[PATCH 0/4] wifi: brcmfmac: Fix bugs when the device is removed before firmware is loaded

[PATCH 0/4] wifi: brcmfmac: Fix bugs when the device is removed before firmware is loaded

[Sean Anderson]

I was working on a different bug, and I noticed that brcmfmac tends to
crash quite spectacularly when the device gets removed before the
firmware request completes. This is because brcmfmac does quite a lot of
initialization that would be normally be done in probe() only when the
firmware is loaded. I found two general classes of bugs:

- Some of the remove paths try to clean up things that the firmware
  request callback sets up, which doesn't work too well if the firmware
  isn't loaded (patches 1 and 2).
- The firmware request callback generally assumes that the driver is
  still alive and kicking. So if it runs after the driver is removed it
  will procede to access all sorts of memory after it's been free'd.

A fairly-reliable way to trigger these bugs is to edit really_probe in
drivers/base/dd.c and replace

    IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE)

with

    !strcmp("brcmfmac", drv->name)

Alternatively, you can use the name of the bus's driver.

I have only tested these fixes on SDIO. I would really appreciate if
someone could test this series on PCIe with the above snippet in their
kernel (preferably with KASAN).

Right now if the firmware cannot be loaded for whatever reason then the
firmware request callback will unbind the driver. This is incompatible
with canceling the firmware request and waiting for it to complete in
the driver's remove() callback. As such, I removed this behavior so the
device now sticks around even if we can't load the firmware. If this
behavior is really, truly desired then we can drop device_lock while
waiting for the firmware request to complete and attempt to recover from
the consequences.


Sean Anderson (4):
  wifi: brcmfmac: Fix canceling uninitialized datawork
  wifi: brcmfmac: Fix brcmf_pno_detach NULL-pointer deference
  firmware_loader: Return status from request_firmware_nowait_cancel
  wifi: brcmfmac: Fix firmware requests racing against SDIO removal

 drivers/base/firmware_loader/main.c           | 11 +++++---
 .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  7 ++++-
 .../broadcom/brcm80211/brcmfmac/bus.h         |  2 ++
 .../broadcom/brcm80211/brcmfmac/core.c        |  3 +--
 .../broadcom/brcm80211/brcmfmac/firmware.c    | 26 ++++++++++++++++---
 .../broadcom/brcm80211/brcmfmac/firmware.h    | 16 +++++++++++-
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 11 +++++---
 .../broadcom/brcm80211/brcmfmac/pno.c         |  2 ++
 .../broadcom/brcm80211/brcmfmac/sdio.c        |  8 +++---
 .../broadcom/brcm80211/brcmfmac/usb.c         |  6 +++--
 include/linux/firmware.h                      |  2 +-
 11 files changed, 75 insertions(+), 19 deletions(-)

---
base-commit: 587858367581b9c55c3690f4e63382ad622719d4
branch: brcmfmac_firmware_cancel

-- 
2.53.0

[PATCH 1/4] wifi: brcmfmac: Fix canceling uninitialized datawork

[Sean Anderson]

datawork is currently initialized in brcmf_attach, but bus_if->drvr is
created before this in brcmf_alloc. Both of these functions are called
on firmware load, which may race with device removal. If this happens,
cancel_work_sync may be called on an uninitialized datawork. Fix this by
always initializing datawork before we set bus_if->drvr, as this matches
the condition in brcmf_bus_cancel_reset_work.

Fixes: 43b25879f004c ("wifi: brcmfmac: drain bus_reset work on device removal")
Signed-off-by: Sean Anderson <sanderson@brivo.com>
---

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index dad6f4563d146..a3163120154dd 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -1298,8 +1298,6 @@ static int brcmf_bus_started(struct brcmf_pub *drvr, struct cfg80211_ops *ops)
 #endif
 #endif /* CONFIG_INET */
 
-	INIT_WORK(&drvr->bus_reset, brcmf_core_bus_reset);
-
 	/* populate debugfs */
 	brcmf_debugfs_add_entry(drvr, "revinfo", brcmf_revinfo_read);
 	debugfs_create_file("reset", 0600, brcmf_debugfs_get_devdir(drvr), drvr,
@@ -1349,6 +1347,7 @@ int brcmf_alloc(struct device *dev, struct brcmf_mp_device *settings)
 	drvr = wiphy_priv(wiphy);
 	drvr->wiphy = wiphy;
 	drvr->ops = ops;
+	INIT_WORK(&drvr->bus_reset, brcmf_core_bus_reset);
 	drvr->bus_if = dev_get_drvdata(dev);
 	drvr->bus_if->drvr = drvr;
 	drvr->settings = settings;
-- 
2.53.0

[PATCH 2/4] wifi: brcmfmac: Fix brcmf_pno_detach NULL-pointer deference

[Sean Anderson]

brcmf_pno_attach is called when firmware is loaded, which may not have
happened yet at removal time. Skip the rest of the cleanup if pi is
NULL.

Fixes: efc2c1fa8e14 ("brcmfmac: add support multi-scheduled scan")
Signed-off-by: Sean Anderson <sanderson@brivo.com>
---

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
index d9fc94076791d..07d195e4d2832 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pno.c
@@ -533,6 +533,8 @@ void brcmf_pno_detach(struct brcmf_cfg80211_info *cfg)
 	brcmf_dbg(TRACE, "enter\n");
 	pi = cfg->pno;
 	cfg->pno = NULL;
+	if (!pi)
+		return;
 
 	WARN_ON(pi->n_reqs);
 	mutex_destroy(&pi->req_lock);
-- 
2.53.0

[PATCH 3/4] firmware_loader: Return status from request_firmware_nowait_cancel

[Sean Anderson]

request_firmware_nowait_cancel only cancels the most-recent matching
firmware request. If the callback creates additional requests then there
can be multiple requests in-flight (the original request and the
(pending) additional request). If we try to cancel the request at that
point, the original request may still be running. Add a status return
from request_firmware_nowait_cancel so we can tell if there may still be
additional requests we need to cancel.

Signed-off-by: Sean Anderson <sanderson@brivo.com>
---

 drivers/base/firmware_loader/main.c | 11 ++++++++---
 include/linux/firmware.h            |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 24213a0ea8317..ea475aa48a316 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -1290,11 +1290,15 @@ EXPORT_SYMBOL_GPL(firmware_request_nowait_nowarn);
  * Cancel a pending request_firmware_nowait() request for @device, @context
  * and @cont. If the associated work has already started, this function waits
  * until the callback has returned. If the callback has already completed, this
- * function does nothing.
+ * function does nothing. This function may need to be called multiple times if
+ * the callback makes additional firmware requests.
  *
  * This function may sleep.
+ *
+ * Return: %true if a request was canceled, or %false if no requests matched
+ *         @device and @context.
  */
-void request_firmware_nowait_cancel(struct device *device, void *context,
+bool request_firmware_nowait_cancel(struct device *device, void *context,
 				    void (*cont)(const struct firmware *fw,
 						 void *context))
 {
@@ -1313,9 +1317,10 @@ void request_firmware_nowait_cancel(struct device *device, void *context,
 	spin_unlock_irq(&firmware_work_lock);
 
 	if (!fw_work)
-		return;
+		return false;
 	cancel_work_sync(&fw_work->work);
 	firmware_work_free(fw_work);
+	return true;
 }
 EXPORT_SYMBOL_GPL(request_firmware_nowait_cancel);
 
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 0fa3b027f02f1..46c34ad0067e7 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -110,7 +110,7 @@ int request_firmware_nowait(
 	struct module *module, bool uevent,
 	const char *name, struct device *device, gfp_t gfp, void *context,
 	void (*cont)(const struct firmware *fw, void *context));
-void request_firmware_nowait_cancel(struct device *device, void *context,
+bool request_firmware_nowait_cancel(struct device *device, void *context,
 				    void (*cont)(const struct firmware *fw,
 						 void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
-- 
2.53.0

[PATCH 4/4] wifi: brcmfmac: Fix firmware requests racing against SDIO removal

[Sean Anderson]

brcmf_sdio_firmware_callback can race with device removal. If this
happens it can re-register IRQs, dereference NULL pointers, and cause
all sorts of havoc. Prevent this by canceling any outstanding firmware
request as the first step of the removal process.

When canceling the firmware request, we primarily need to ensure fwctx
remains valid for all our calls to request_firmware_nowait_cancel. If we
let it get free'd early then it could get re-used for some unrelated
firmware request. To avoid this, we follow the same pattern that
firmware_loader does. But while firmware_loader needs a spinlock, we can
get away with a single pointer:

- When ctxp is NULL, then we can't be canceled
- When *ctxp == fwctx, we're still alive
- When *ctxp == NULL, someone else has canceled the request (or we have
  run our natural course).
- Whoever clears ctxp is responsible for freeing fwctx.

There can be up to NR_CPUS requests in-flight at any given time, as each
(alt) firmware request can create a new firmware request. Eventually,
one of them will see that ctxp is cleared and stop spawning additional
requests.

If the firmware load fails while we are removing the device, we can no
longer attempt to call device_release_driver. This will deadlock. We
can't do this asynchronously either since we run the risk of releasing a
totally different driver/device combo. We could techincally do this by
dropping device_lock before waiting for the firmware to cancel, but that
seems like a major headache (we would need to make ctxp a separate
reference-counted allocation).

I also implemented this fix for PCIe but I have only build-tested it.
I didn't touch USB as it already uses a completion-based system to
determine when it's OK to remove the driver. I didn't go with this
approach because we could wait indefinitely for the firmware request to
complete (such as if the firmware is on a slow device or loaded by
userspace).

Fixes: bd0e1b1d380e ("brcmfmac: use asynchronous firmware request in SDIO")
Signed-off-by: Sean Anderson <sanderson@brivo.com>
---

 .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  7 ++++-
 .../broadcom/brcm80211/brcmfmac/bus.h         |  2 ++
 .../broadcom/brcm80211/brcmfmac/firmware.c    | 26 ++++++++++++++++---
 .../broadcom/brcm80211/brcmfmac/firmware.h    | 16 +++++++++++-
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 11 +++++---
 .../broadcom/brcm80211/brcmfmac/sdio.c        |  8 +++---
 .../broadcom/brcm80211/brcmfmac/usb.c         |  6 +++--
 7 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 71c2f99cdb711..39916f5a699dd 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1126,7 +1126,12 @@ static void brcmf_ops_sdio_remove(struct sdio_func *func)
 	if (bus_if) {
 		sdiodev = bus_if->bus_priv.sdio;
 
-		/* start by unregistering irqs */
+		/* Cancel any outstanding firmware request, as it may try to
+		 * call brcmf_sdiod_intr_register.
+		 */
+		brcmf_fw_cancel(sdiodev->dev, &bus_if->fwctx);
+
+		/* Now we can unregister irqs */
 		brcmf_sdiod_intr_unregister(sdiodev);
 
 		if (func->num != 1)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index 9371c1489948c..81c17a8c235cc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -157,6 +157,7 @@ struct brcmf_bus_stats {
  * @chip: device identifier of the dongle chip.
  * @chiprev: revision of the dongle chip.
  * @fwvid: firmware vendor-support identifier of the device.
+ * @fwctx: firmware request cancellation context
  * @always_use_fws_queue: bus wants use queue also when fwsignal is inactive.
  * @wowl_supported: is wowl supported by bus driver.
  * @ops: callbacks for this bus instance.
@@ -178,6 +179,7 @@ struct brcmf_bus {
 	u32 chip;
 	u32 chiprev;
 	enum brcmf_fwvendor fwvid;
+	void *fwctx;
 	bool always_use_fws_queue;
 	bool wowl_supported;
 	bool removing;		/* device removal in progress; quiesce async work */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index 22ff326f1924a..d1da65155e544 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -459,6 +459,7 @@ struct brcmf_fw {
 	u32 curpos;
 	unsigned int board_index;
 	void (*done)(struct device *dev, int err, struct brcmf_fw_request *req);
+	void **ctxp;
 };
 
 #ifdef CONFIG_EFI
@@ -693,7 +694,7 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx)
 		fwctx->req = NULL;
 	}
 	fwctx->done(fwctx->dev, ret, fwctx->req);
-	kfree(fwctx);
+	kfree(xchg(fwctx->ctxp, NULL));
 }
 
 static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx)
@@ -703,7 +704,7 @@ static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx)
 	const char *board_type, *alt_path;
 	int ret = 0;
 
-	if (fw) {
+	if (fw || !READ_ONCE(*fwctx->ctxp)) {
 		brcmf_fw_request_done(fw, ctx);
 		return;
 	}
@@ -755,7 +756,8 @@ static bool brcmf_fw_request_is_valid(struct brcmf_fw_request *req)
 
 int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
 			   void (*fw_cb)(struct device *dev, int err,
-					 struct brcmf_fw_request *req))
+					 struct brcmf_fw_request *req),
+			   void **ctxp)
 {
 	struct brcmf_fw_item *first = &req->items[0];
 	struct brcmf_fw *fwctx;
@@ -776,6 +778,8 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
 	fwctx->dev = dev;
 	fwctx->req = req;
 	fwctx->done = fw_cb;
+	fwctx->ctxp = ctxp;
+	WRITE_ONCE(*ctxp, fwctx);
 
 	/* First try alternative board-specific path if any */
 	if (fwctx->req->board_types[0])
@@ -799,6 +803,22 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
 	return 0;
 }
 
+void brcmf_fw_cancel(struct device *dev, void **ctxp)
+{
+	struct brcmf_fw *fwctx;
+
+	fwctx = xchg(ctxp, NULL);
+	if (!fwctx)
+		return;
+
+	/* Keep canceling requests until they see that we cleared ctxp */
+	while (request_firmware_nowait_cancel(dev, fwctx,
+					      brcmf_fw_request_done_alt_path))
+		;
+	request_firmware_nowait_cancel(dev, fwctx, brcmf_fw_request_done);
+	kfree(fwctx);
+}
+
 struct brcmf_fw_request *
 brcmf_fw_alloc_request(u32 chip, u32 chiprev,
 		       const struct brcmf_firmware_mapping mapping_table[],
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
index 4002d326fd21b..932899d4086b3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
@@ -87,9 +87,23 @@ brcmf_fw_alloc_request(u32 chip, u32 chiprev,
  * Request firmware(s) asynchronously. When the asynchronous request
  * fails it will not use the callback, but call device_release_driver()
  * instead which will call the driver .remove() callback.
+ *
+ * ctxp is a (pointer to an) opaque pointer that may be passed to
+ * brcmf_fw_cancel().
  */
 int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
 			   void (*fw_cb)(struct device *dev, int err,
-					 struct brcmf_fw_request *req));
+					 struct brcmf_fw_request *req),
+			   void **ctxp);
+
+/**
+ * brcmf_fw_cancel() - Cancel an outstanding firmware request
+ * @dev: Device requesting the firmware
+ * @ctxp: Opaque context pointer filled in by brcmf_fw_get_firmwares()
+ *
+ * Cancel an outstanding firmware request identified by @dev and @ctxp, which
+ * should be the same as passed to brcmf_fw_get_firmwares().
+ */
+void brcmf_fw_cancel(struct device *dev, void **ctxp);
 
 #endif /* BRCMFMAC_FIRMWARE_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 55f4d7b970f28..9eae712b0d98c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1555,6 +1555,8 @@ static int brcmf_pcie_reset(struct device *dev)
 	struct brcmf_fw_request *fwreq;
 	int err;
 
+	brcmf_fw_cancel(dev, &bus_if->fwctx);
+
 	brcmf_pcie_intr_disable(devinfo);
 
 	brcmf_pcie_bus_console_read(devinfo, true);
@@ -1572,7 +1574,8 @@ static int brcmf_pcie_reset(struct device *dev)
 		return -ENOMEM;
 	}
 
-	err = brcmf_fw_get_firmwares(dev, fwreq, brcmf_pcie_setup);
+	err = brcmf_fw_get_firmwares(dev, fwreq, brcmf_pcie_setup,
+				     &bus_if->fwctx);
 	if (err) {
 		dev_err(dev, "Failed to prepare FW request\n");
 		kfree(fwreq);
@@ -2231,7 +2234,6 @@ static void brcmf_pcie_setup(struct device *dev, int ret,
 	brcmf_err(bus, "Dongle setup failed\n");
 	brcmf_pcie_bus_console_read(devinfo, true);
 	brcmf_fw_crashed(dev);
-	device_release_driver(dev);
 }
 
 static struct brcmf_fw_request *
@@ -2554,7 +2556,8 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto fail_brcmf;
 	}
 
-	ret = brcmf_fw_get_firmwares(bus->dev, fwreq, brcmf_pcie_setup);
+	ret = brcmf_fw_get_firmwares(bus->dev, fwreq, brcmf_pcie_setup,
+				     &bus->fwctx);
 	if (ret < 0) {
 		kfree(fwreq);
 		goto fail_brcmf;
@@ -2595,6 +2598,8 @@ brcmf_pcie_remove(struct pci_dev *pdev)
 	brcmf_pcie_bus_console_read(devinfo, false);
 	brcmf_pcie_fwcon_timer(devinfo, false);
 
+	brcmf_fw_cancel(bus->dev, &bus->fwctx);
+
 	devinfo->state = BRCMFMAC_PCIE_STATE_DOWN;
 	if (devinfo->ci)
 		brcmf_pcie_intr_disable(devinfo);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 381801af3ac98..1c32fe5828a2f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4417,8 +4417,6 @@ static void brcmf_sdio_firmware_callback(struct device *dev, int err,
 	sdio_release_host(sdiod->func1);
 fail:
 	brcmf_dbg(TRACE, "failed: dev=%s, err=%d\n", dev_name(dev), err);
-	device_release_driver(&sdiod->func2->dev);
-	device_release_driver(dev);
 }
 
 static struct brcmf_fw_request *
@@ -4546,7 +4544,8 @@ int brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
 	}
 
 	ret = brcmf_fw_get_firmwares(sdiodev->dev, fwreq,
-				     brcmf_sdio_firmware_callback);
+				     brcmf_sdio_firmware_callback,
+				     &sdiodev->bus_if->fwctx);
 	if (ret != 0) {
 		brcmf_err("async firmware request failed: %d\n", ret);
 		kfree(fwreq);
@@ -4581,6 +4580,9 @@ void brcmf_sdio_remove(struct brcmf_sdio *bus)
 			bus->watchdog_tsk = NULL;
 		}
 
+		brcmf_fw_cancel(bus->sdiodev->dev,
+				&bus->sdiodev->bus_if->fwctx);
+
 		/* De-register interrupt handler */
 		brcmf_sdiod_intr_unregister(bus->sdiodev);
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index b41949a9bdc8e..e8c0db0c82689 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1306,7 +1306,8 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo,
 	}
 
 	/* request firmware here */
-	ret = brcmf_fw_get_firmwares(dev, fwreq, brcmf_usb_probe_phase2);
+	ret = brcmf_fw_get_firmwares(dev, fwreq, brcmf_usb_probe_phase2,
+				     &bus->fwctx);
 	if (ret) {
 		brcmf_err("firmware request failed: %d\n", ret);
 		kfree(fwreq);
@@ -1524,7 +1525,8 @@ static int brcmf_usb_reset_resume(struct usb_interface *intf)
 	if (!fwreq)
 		return -ENOMEM;
 
-	ret = brcmf_fw_get_firmwares(&usb->dev, fwreq, brcmf_usb_probe_phase2);
+	ret = brcmf_fw_get_firmwares(&usb->dev, fwreq, brcmf_usb_probe_phase2,
+				     &devinfo->bus_pub->bus->fwctx);
 	if (ret < 0)
 		kfree(fwreq);
 
-- 
2.53.0

Re: [PATCH 4/4] wifi: brcmfmac: Fix firmware requests racing against SDIO removal

[Sean Anderson]

On 9/21/26 5:18 PM, Sean Anderson wrote:
> brcmf_sdio_firmware_callback can race with device removal. If this
> happens it can re-register IRQs, dereference NULL pointers, and cause
> all sorts of havoc. Prevent this by canceling any outstanding firmware
> request as the first step of the removal process.
> 
> When canceling the firmware request, we primarily need to ensure fwctx
> remains valid for all our calls to request_firmware_nowait_cancel. If we
> let it get free'd early then it could get re-used for some unrelated
> firmware request. To avoid this, we follow the same pattern that
> firmware_loader does. But while firmware_loader needs a spinlock, we can
> get away with a single pointer:
> 
> - When ctxp is NULL, then we can't be canceled
> - When *ctxp == fwctx, we're still alive
> - When *ctxp == NULL, someone else has canceled the request (or we have
>    run our natural course).
> - Whoever clears ctxp is responsible for freeing fwctx.
> 
> There can be up to NR_CPUS requests in-flight at any given time, as each
> (alt) firmware request can create a new firmware request. Eventually,
> one of them will see that ctxp is cleared and stop spawning additional
> requests.
> 
> If the firmware load fails while we are removing the device, we can no
> longer attempt to call device_release_driver. This will deadlock. We
> can't do this asynchronously either since we run the risk of releasing a
> totally different driver/device combo. We could techincally do this by
> dropping device_lock before waiting for the firmware to cancel, but that
> seems like a major headache (we would need to make ctxp a separate
> reference-counted allocation).
> 
> I also implemented this fix for PCIe but I have only build-tested it.
> I didn't touch USB as it already uses a completion-based system to
> determine when it's OK to remove the driver. I didn't go with this
> approach because we could wait indefinitely for the firmware request to
> complete (such as if the firmware is on a slow device or loaded by
> userspace).
> 
> Fixes: bd0e1b1d380e ("brcmfmac: use asynchronous firmware request in SDIO")
> Signed-off-by: Sean Anderson <sanderson@brivo.com>
> ---
> 
>   .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  7 ++++-
>   .../broadcom/brcm80211/brcmfmac/bus.h         |  2 ++
>   .../broadcom/brcm80211/brcmfmac/firmware.c    | 26 ++++++++++++++++---
>   .../broadcom/brcm80211/brcmfmac/firmware.h    | 16 +++++++++++-
>   .../broadcom/brcm80211/brcmfmac/pcie.c        | 11 +++++---
>   .../broadcom/brcm80211/brcmfmac/sdio.c        |  8 +++---
>   .../broadcom/brcm80211/brcmfmac/usb.c         |  6 +++--
>   7 files changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 71c2f99cdb711..39916f5a699dd 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -1126,7 +1126,12 @@ static void brcmf_ops_sdio_remove(struct sdio_func *func)
>   	if (bus_if) {
>   		sdiodev = bus_if->bus_priv.sdio;
>   
> -		/* start by unregistering irqs */
> +		/* Cancel any outstanding firmware request, as it may try to
> +		 * call brcmf_sdiod_intr_register.
> +		 */
> +		brcmf_fw_cancel(sdiodev->dev, &bus_if->fwctx);
> +
> +		/* Now we can unregister irqs */
>   		brcmf_sdiod_intr_unregister(sdiodev);
>   
>   		if (func->num != 1)
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> index 9371c1489948c..81c17a8c235cc 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> @@ -157,6 +157,7 @@ struct brcmf_bus_stats {
>    * @chip: device identifier of the dongle chip.
>    * @chiprev: revision of the dongle chip.
>    * @fwvid: firmware vendor-support identifier of the device.
> + * @fwctx: firmware request cancellation context
>    * @always_use_fws_queue: bus wants use queue also when fwsignal is inactive.
>    * @wowl_supported: is wowl supported by bus driver.
>    * @ops: callbacks for this bus instance.
> @@ -178,6 +179,7 @@ struct brcmf_bus {
>   	u32 chip;
>   	u32 chiprev;
>   	enum brcmf_fwvendor fwvid;
> +	void *fwctx;
>   	bool always_use_fws_queue;
>   	bool wowl_supported;
>   	bool removing;		/* device removal in progress; quiesce async work */
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index 22ff326f1924a..d1da65155e544 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -459,6 +459,7 @@ struct brcmf_fw {
>   	u32 curpos;
>   	unsigned int board_index;
>   	void (*done)(struct device *dev, int err, struct brcmf_fw_request *req);
> +	void **ctxp;
>   };
>   
>   #ifdef CONFIG_EFI
> @@ -693,7 +694,7 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx)
>   		fwctx->req = NULL;
>   	}
>   	fwctx->done(fwctx->dev, ret, fwctx->req);
> -	kfree(fwctx);
> +	kfree(xchg(fwctx->ctxp, NULL));
>   }
>   
>   static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx)
> @@ -703,7 +704,7 @@ static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx)
>   	const char *board_type, *alt_path;
>   	int ret = 0;
>   
> -	if (fw) {
> +	if (fw || !READ_ONCE(*fwctx->ctxp)) {
>   		brcmf_fw_request_done(fw, ctx);
>   		return;
>   	}
> @@ -755,7 +756,8 @@ static bool brcmf_fw_request_is_valid(struct brcmf_fw_request *req)
>   
>   int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
>   			   void (*fw_cb)(struct device *dev, int err,
> -					 struct brcmf_fw_request *req))
> +					 struct brcmf_fw_request *req),
> +			   void **ctxp)
>   {
>   	struct brcmf_fw_item *first = &req->items[0];
>   	struct brcmf_fw *fwctx;
> @@ -776,6 +778,8 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
>   	fwctx->dev = dev;
>   	fwctx->req = req;
>   	fwctx->done = fw_cb;
> +	fwctx->ctxp = ctxp;
> +	WRITE_ONCE(*ctxp, fwctx);
>   
>   	/* First try alternative board-specific path if any */
>   	if (fwctx->req->board_types[0])
> @@ -799,6 +803,22 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
>   	return 0;
>   }
>   
> +void brcmf_fw_cancel(struct device *dev, void **ctxp)
> +{
> +	struct brcmf_fw *fwctx;
> +
> +	fwctx = xchg(ctxp, NULL);
> +	if (!fwctx)
> +		return;
> +
> +	/* Keep canceling requests until they see that we cleared ctxp */
> +	while (request_firmware_nowait_cancel(dev, fwctx,
> +					      brcmf_fw_request_done_alt_path))
> +		;
> +	request_firmware_nowait_cancel(dev, fwctx, brcmf_fw_request_done);
> +	kfree(fwctx);
> +}
> +
>   struct brcmf_fw_request *
>   brcmf_fw_alloc_request(u32 chip, u32 chiprev,
>   		       const struct brcmf_firmware_mapping mapping_table[],
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
> index 4002d326fd21b..932899d4086b3 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
> @@ -87,9 +87,23 @@ brcmf_fw_alloc_request(u32 chip, u32 chiprev,
>    * Request firmware(s) asynchronously. When the asynchronous request
>    * fails it will not use the callback, but call device_release_driver()
>    * instead which will call the driver .remove() callback.
> + *
> + * ctxp is a (pointer to an) opaque pointer that may be passed to
> + * brcmf_fw_cancel().
>    */
>   int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
>   			   void (*fw_cb)(struct device *dev, int err,
> -					 struct brcmf_fw_request *req));
> +					 struct brcmf_fw_request *req),
> +			   void **ctxp);
> +
> +/**
> + * brcmf_fw_cancel() - Cancel an outstanding firmware request
> + * @dev: Device requesting the firmware
> + * @ctxp: Opaque context pointer filled in by brcmf_fw_get_firmwares()
> + *
> + * Cancel an outstanding firmware request identified by @dev and @ctxp, which
> + * should be the same as passed to brcmf_fw_get_firmwares().
> + */
> +void brcmf_fw_cancel(struct device *dev, void **ctxp);
>   
>   #endif /* BRCMFMAC_FIRMWARE_H */
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index 55f4d7b970f28..9eae712b0d98c 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -1555,6 +1555,8 @@ static int brcmf_pcie_reset(struct device *dev)
>   	struct brcmf_fw_request *fwreq;
>   	int err;
>   
> +	brcmf_fw_cancel(dev, &bus_if->fwctx);
> +
>   	brcmf_pcie_intr_disable(devinfo);
>   
>   	brcmf_pcie_bus_console_read(devinfo, true);
> @@ -1572,7 +1574,8 @@ static int brcmf_pcie_reset(struct device *dev)
>   		return -ENOMEM;
>   	}
>   
> -	err = brcmf_fw_get_firmwares(dev, fwreq, brcmf_pcie_setup);
> +	err = brcmf_fw_get_firmwares(dev, fwreq, brcmf_pcie_setup,
> +				     &bus_if->fwctx);
>   	if (err) {
>   		dev_err(dev, "Failed to prepare FW request\n");
>   		kfree(fwreq);
> @@ -2231,7 +2234,6 @@ static void brcmf_pcie_setup(struct device *dev, int ret,
>   	brcmf_err(bus, "Dongle setup failed\n");
>   	brcmf_pcie_bus_console_read(devinfo, true);
>   	brcmf_fw_crashed(dev);
> -	device_release_driver(dev);
>   }
>   
>   static struct brcmf_fw_request *
> @@ -2554,7 +2556,8 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   		goto fail_brcmf;
>   	}
>   
> -	ret = brcmf_fw_get_firmwares(bus->dev, fwreq, brcmf_pcie_setup);
> +	ret = brcmf_fw_get_firmwares(bus->dev, fwreq, brcmf_pcie_setup,
> +				     &bus->fwctx);
>   	if (ret < 0) {
>   		kfree(fwreq);
>   		goto fail_brcmf;
> @@ -2595,6 +2598,8 @@ brcmf_pcie_remove(struct pci_dev *pdev)
>   	brcmf_pcie_bus_console_read(devinfo, false);
>   	brcmf_pcie_fwcon_timer(devinfo, false);
>   
> +	brcmf_fw_cancel(bus->dev, &bus->fwctx);
> +
>   	devinfo->state = BRCMFMAC_PCIE_STATE_DOWN;
>   	if (devinfo->ci)
>   		brcmf_pcie_intr_disable(devinfo);
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 381801af3ac98..1c32fe5828a2f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -4417,8 +4417,6 @@ static void brcmf_sdio_firmware_callback(struct device *dev, int err,
>   	sdio_release_host(sdiod->func1);
>   fail:
>   	brcmf_dbg(TRACE, "failed: dev=%s, err=%d\n", dev_name(dev), err);
> -	device_release_driver(&sdiod->func2->dev);
> -	device_release_driver(dev);
>   }
>   
>   static struct brcmf_fw_request *
> @@ -4546,7 +4544,8 @@ int brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
>   	}
>   
>   	ret = brcmf_fw_get_firmwares(sdiodev->dev, fwreq,
> -				     brcmf_sdio_firmware_callback);
> +				     brcmf_sdio_firmware_callback,
> +				     &sdiodev->bus_if->fwctx);
>   	if (ret != 0) {
>   		brcmf_err("async firmware request failed: %d\n", ret);
>   		kfree(fwreq);
> @@ -4581,6 +4580,9 @@ void brcmf_sdio_remove(struct brcmf_sdio *bus)
>   			bus->watchdog_tsk = NULL;
>   		}
>   
> +		brcmf_fw_cancel(bus->sdiodev->dev,
> +				&bus->sdiodev->bus_if->fwctx);
> +
>   		/* De-register interrupt handler */
>   		brcmf_sdiod_intr_unregister(bus->sdiodev);
>   
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> index b41949a9bdc8e..e8c0db0c82689 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> @@ -1306,7 +1306,8 @@ static int brcmf_usb_probe_cb(struct brcmf_usbdev_info *devinfo,
>   	}
>   
>   	/* request firmware here */
> -	ret = brcmf_fw_get_firmwares(dev, fwreq, brcmf_usb_probe_phase2);
> +	ret = brcmf_fw_get_firmwares(dev, fwreq, brcmf_usb_probe_phase2,
> +				     &bus->fwctx);
>   	if (ret) {
>   		brcmf_err("firmware request failed: %d\n", ret);
>   		kfree(fwreq);
> @@ -1524,7 +1525,8 @@ static int brcmf_usb_reset_resume(struct usb_interface *intf)
>   	if (!fwreq)
>   		return -ENOMEM;
>   
> -	ret = brcmf_fw_get_firmwares(&usb->dev, fwreq, brcmf_usb_probe_phase2);
> +	ret = brcmf_fw_get_firmwares(&usb->dev, fwreq, brcmf_usb_probe_phase2,
> +				     &devinfo->bus_pub->bus->fwctx);

This should be devinfo->bus_pub.bus->fwctx. Forgot to add the hunk before sending.

--Sean

>   	if (ret < 0)
>   		kfree(fwreq);
>