mwifiex: resolve races between async FW init (failure) and device removal

It's possible for the FW init sequence to fail, which will trigger a
device cleanup sequence in mwifiex_fw_dpc(). This sequence can race with
device suspend() or remove() (e.g., reboot or unbind), and can trigger
use-after-free issues. Currently, this driver attempts (poorly) to
synchronize remove() using a semaphore, but it doesn't protect some of
the critical sections properly. Particularly, we grab a pointer to the
adapter struct (card->adapter) without checking if it's being freed or
not. We later do a NULL check on the adapter, but that doesn't work if
the adapter was freed.

Also note that the PCIe interface driver doesn't ever set card->adapter
to NULL, so even if we get the synchronization right, we still might try
to redo the cleanup in ->remove(), even if the FW init failure sequence
already did it.

This patch replaces the static semaphore with a per-device completion
struct, and uses that completion to synchronize the remove() thread with
the mwifiex_fw_dpc(). A future patch will utilize this completion to
synchronize the suspend() thread as well.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 9e7acb48f7546ea4c8dc39793b3f2b3f4ae8354d..eac44fe9c4168826536eeafb84ecaff6886d8aae 100644 (file)
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -521,9 +521,9 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context)
        struct mwifiex_private *priv;
        struct mwifiex_adapter *adapter = context;
        struct mwifiex_fw_image fw;
-       struct semaphore *sem = adapter->card_sem;
        bool init_failed = false;
        struct wireless_dev *wdev;
+       struct completion *fw_done = adapter->fw_done;
 
        if (!firmware) {
                mwifiex_dbg(adapter, ERROR,
@@ -670,7 +670,8 @@ done:
        }
        if (init_failed)
                mwifiex_free_adapter(adapter);
-       up(sem);
+       /* Tell all current and future waiters we're finished */
+       complete_all(fw_done);
        return;
 }
 
@@ -1365,7 +1366,7 @@ static void mwifiex_main_work_queue(struct work_struct *work)
  * code is extracted from mwifiex_remove_card()
  */
 static int
-mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem)
+mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
 {
        struct mwifiex_private *priv;
        int i;
@@ -1373,8 +1374,9 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem)
        if (!adapter)
                goto exit_return;
 
-       if (down_interruptible(sem))
-               goto exit_sem_err;
+       wait_for_completion(adapter->fw_done);
+       /* Caller should ensure we aren't suspending while this happens */
+       reinit_completion(adapter->fw_done);
 
        priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
        mwifiex_deauthenticate(priv, NULL);
@@ -1431,8 +1433,6 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem)
                rtnl_unlock();
        }
 
-       up(sem);
-exit_sem_err:
        mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
 exit_return:
        return 0;
@@ -1442,21 +1442,18 @@ exit_return:
  * code is extracted from mwifiex_add_card()
  */
 static int
-mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct semaphore *sem,
+mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct completion *fw_done,
                  struct mwifiex_if_ops *if_ops, u8 iface_type)
 {
        char fw_name[32];
        struct pcie_service_card *card = adapter->card;
 
-       if (down_interruptible(sem))
-               goto exit_sem_err;
-
        mwifiex_init_lock_list(adapter);
        if (adapter->if_ops.up_dev)
                adapter->if_ops.up_dev(adapter);
 
        adapter->iface_type = iface_type;
-       adapter->card_sem = sem;
+       adapter->fw_done = fw_done;
 
        adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;
        adapter->surprise_removed = false;
@@ -1507,7 +1504,8 @@ mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct semaphore *sem,
        }
        strcpy(adapter->fw_name, fw_name);
        mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
-       up(sem);
+
+       complete_all(adapter->fw_done);
        return 0;
 
 err_init_fw:
@@ -1527,8 +1525,7 @@ err_init_fw:
 err_kmalloc:
        mwifiex_terminate_workqueue(adapter);
        adapter->surprise_removed = true;
-       up(sem);
-exit_sem_err:
+       complete_all(adapter->fw_done);
        mwifiex_dbg(adapter, INFO, "%s, error\n", __func__);
 
        return -1;
@@ -1543,12 +1540,12 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
        struct mwifiex_if_ops if_ops;
 
        if (!prepare) {
-               mwifiex_reinit_sw(adapter, adapter->card_sem, &if_ops,
+               mwifiex_reinit_sw(adapter, adapter->fw_done, &if_ops,
                                  adapter->iface_type);
        } else {
                memcpy(&if_ops, &adapter->if_ops,
                       sizeof(struct mwifiex_if_ops));
-               mwifiex_shutdown_sw(adapter, adapter->card_sem);
+               mwifiex_shutdown_sw(adapter);
        }
 }
 EXPORT_SYMBOL_GPL(mwifiex_do_flr);
@@ -1618,15 +1615,12 @@ err_exit:
  *      - Add logical interfaces
  */
 int
-mwifiex_add_card(void *card, struct semaphore *sem,
+mwifiex_add_card(void *card, struct completion *fw_done,
                 struct mwifiex_if_ops *if_ops, u8 iface_type,
                 struct device *dev)
 {
        struct mwifiex_adapter *adapter;
 
-       if (down_interruptible(sem))
-               goto exit_sem_err;
-
        if (mwifiex_register(card, if_ops, (void **)&adapter)) {
                pr_err("%s: software init failed\n", __func__);
                goto err_init_sw;
@@ -1636,7 +1630,7 @@ mwifiex_add_card(void *card, struct semaphore *sem,
        mwifiex_probe_of(adapter);
 
        adapter->iface_type = iface_type;
-       adapter->card_sem = sem;
+       adapter->fw_done = fw_done;
 
        adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;
        adapter->surprise_removed = false;
@@ -1705,9 +1699,7 @@ err_kmalloc:
        mwifiex_free_adapter(adapter);
 
 err_init_sw:
-       up(sem);
 
-exit_sem_err:
        return -1;
 }
 EXPORT_SYMBOL_GPL(mwifiex_add_card);
@@ -1723,14 +1715,11 @@ EXPORT_SYMBOL_GPL(mwifiex_add_card);
  *      - Unregister the device
  *      - Free the adapter structure
  */
-int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem)
+int mwifiex_remove_card(struct mwifiex_adapter *adapter)
 {
        struct mwifiex_private *priv = NULL;
        int i;
 
-       if (down_trylock(sem))
-               goto exit_sem_err;
-
        if (!adapter)
                goto exit_remove;
 
@@ -1800,8 +1789,6 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem)
        mwifiex_free_adapter(adapter);
 
 exit_remove:
-       up(sem);
-exit_sem_err:
        return 0;
 }
 EXPORT_SYMBOL_GPL(mwifiex_remove_card);

diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 904a2edefc06440fa8fb02b8e9b41fb6f08efb1a..cf4c78080d94c115e95bfe365ec2e826f044537e 100644 (file)
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -20,6 +20,7 @@
 #ifndef _MWIFIEX_MAIN_H_
 #define _MWIFIEX_MAIN_H_
 
+#include <linux/completion.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sched.h>
@@ -985,7 +986,10 @@ struct mwifiex_adapter {
        u32 usr_dot_11ac_mcs_support;
 
        atomic_t pending_bridged_pkts;
-       struct semaphore *card_sem;
+
+       /* For synchronizing FW initialization with device lifecycle. */
+       struct completion *fw_done;
+
        bool ext_scan;
        u8 fw_api_ver;
        u8 key_api_major_ver, key_api_minor_ver;
@@ -1438,10 +1442,11 @@ static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
 
 int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
                             u32 func_init_shutdown);
-int mwifiex_add_card(void *card, struct semaphore *sem,
+
+int mwifiex_add_card(void *card, struct completion *fw_done,
                     struct mwifiex_if_ops *if_ops, u8 iface_type,
                     struct device *dev);
-int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
+int mwifiex_remove_card(struct mwifiex_adapter *adapter);
 
 void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
                         int maxlen);

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index cfb45ef9f85fea662402df0fffab381305634a7b..547806f8f9233bec39c29baac5b48989dc1571bd 100644 (file)
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -35,8 +35,6 @@ static u8 user_rmmod;
 
 static struct mwifiex_if_ops pcie_ops;
 
-static struct semaphore add_remove_card_sem;
-
 static const struct of_device_id mwifiex_pcie_of_match_table[] = {
        { .compatible = "pci11ab,2b42" },
        { .compatible = "pci1b4b,2b42" },
@@ -212,6 +210,8 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
        if (!card)
                return -ENOMEM;
 
+       init_completion(&card->fw_done);
+
        card->dev = pdev;
 
        if (ent->driver_data) {
@@ -232,7 +232,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
                        return ret;
        }
 
-       if (mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
+       if (mwifiex_add_card(card, &card->fw_done, &pcie_ops,
                             MWIFIEX_PCIE, &pdev->dev)) {
                pr_err("%s failed\n", __func__);
                return -1;
@@ -254,6 +254,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
        if (!card)
                return;
 
+       wait_for_completion(&card->fw_done);
+
        adapter = card->adapter;
        if (!adapter || !adapter->priv_num)
                return;
@@ -273,7 +275,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
                mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
        }
 
-       mwifiex_remove_card(card->adapter, &add_remove_card_sem);
+       mwifiex_remove_card(adapter);
 }
 
 static void mwifiex_pcie_shutdown(struct pci_dev *pdev)
@@ -3175,8 +3177,7 @@ static struct mwifiex_if_ops pcie_ops = {
 /*
  * This function initializes the PCIE driver module.
  *
- * This initiates the semaphore and registers the device with
- * PCIE bus.
+ * This registers the device with PCIE bus.
  */
 static int mwifiex_pcie_init_module(void)
 {
@@ -3184,8 +3185,6 @@ static int mwifiex_pcie_init_module(void)
 
        pr_debug("Marvell PCIe Driver\n");
 
-       sema_init(&add_remove_card_sem, 1);
-
        /* Clear the flag in case user removes the card. */
        user_rmmod = 0;
 
@@ -3209,9 +3208,6 @@ static int mwifiex_pcie_init_module(void)
  */
 static void mwifiex_pcie_cleanup_module(void)
 {
-       if (!down_interruptible(&add_remove_card_sem))
-               up(&add_remove_card_sem);
-
        /* Set the flag as user is removing this module. */
        user_rmmod = 1;
 

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h
index 46f99cae9399bd7e946b545cf9e51f76f8bd1121..ae3365d1c34e8acc8d882c56b8972c9c17dd5a85 100644 (file)
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -22,6 +22,7 @@
 #ifndef        _MWIFIEX_PCIE_H
 #define        _MWIFIEX_PCIE_H
 
+#include    <linux/completion.h>
 #include    <linux/pci.h>
 #include    <linux/interrupt.h>
 
@@ -345,6 +346,7 @@ struct pcie_service_card {
        struct pci_dev *dev;
        struct mwifiex_adapter *adapter;
        struct mwifiex_pcie_device pcie;
+       struct completion fw_done;
 
        u8 txbd_flush;
        u32 txbd_wrptr;

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index f410cf5c04e4d353d51f25a6e51ef76bb9cea208..5312ffb019c8b9970eb87359856924f97d34aa01 100644 (file)
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -49,8 +49,6 @@ static u8 user_rmmod;
 static struct mwifiex_if_ops sdio_ops;
 static unsigned long iface_work_flags;
 
-static struct semaphore add_remove_card_sem;
-
 static struct memory_type_mapping generic_mem_type_map[] = {
        {"DUMP", NULL, 0, 0xDD},
 };
@@ -115,6 +113,8 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
        if (!card)
                return -ENOMEM;
 
+       init_completion(&card->fw_done);
+
        card->func = func;
        card->device_id = id;
 
@@ -154,7 +154,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
                        goto err_disable;
        }
 
-       ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
+       ret = mwifiex_add_card(card, &card->fw_done, &sdio_ops,
                               MWIFIEX_SDIO, &func->dev);
        if (ret) {
                dev_err(&func->dev, "add card failed\n");
@@ -235,6 +235,8 @@ mwifiex_sdio_remove(struct sdio_func *func)
        if (!card)
                return;
 
+       wait_for_completion(&card->fw_done);
+
        adapter = card->adapter;
        if (!adapter || !adapter->priv_num)
                return;
@@ -252,7 +254,7 @@ mwifiex_sdio_remove(struct sdio_func *func)
                mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
        }
 
-       mwifiex_remove_card(card->adapter, &add_remove_card_sem);
+       mwifiex_remove_card(adapter);
 }
 
 /*
@@ -2714,14 +2716,11 @@ static struct mwifiex_if_ops sdio_ops = {
 /*
  * This function initializes the SDIO driver.
  *
- * This initiates the semaphore and registers the device with
- * SDIO bus.
+ * This registers the device with SDIO bus.
  */
 static int
 mwifiex_sdio_init_module(void)
 {
-       sema_init(&add_remove_card_sem, 1);
-
        /* Clear the flag in case user removes the card. */
        user_rmmod = 0;
 
@@ -2740,9 +2739,6 @@ mwifiex_sdio_init_module(void)
 static void
 mwifiex_sdio_cleanup_module(void)
 {
-       if (!down_interruptible(&add_remove_card_sem))
-               up(&add_remove_card_sem);
-
        /* Set the flag as user is removing this module. */
        user_rmmod = 1;
        cancel_work_sync(&sdio_work);

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
index b9fbc5cf6262d8647d6064ddde51211acf749f72..cdbf3a3ac7f994fbb56ced7df60b501199976202 100644 (file)
--- a/drivers/net/wireless/marvell/mwifiex/sdio.h
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
@@ -21,6 +21,7 @@
 #define        _MWIFIEX_SDIO_H
 
 
+#include <linux/completion.h>
 #include <linux/mmc/sdio.h>
 #include <linux/mmc/sdio_ids.h>
 #include <linux/mmc/sdio_func.h>
@@ -238,6 +239,7 @@ struct sdio_mmc_card {
        struct sdio_func *func;
        struct mwifiex_adapter *adapter;
 
+       struct completion fw_done;
        const char *firmware;
        const struct mwifiex_sdio_card_reg *reg;
        u8 max_ports;

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 9cc2a0c0b6700b9968a243e0dcfdb0cf83cd5d9a..63a755c1e38ca958c32b139a72c5d6d19cfd9bc2 100644 (file)
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -24,7 +24,6 @@
 
 static u8 user_rmmod;
 static struct mwifiex_if_ops usb_ops;
-static struct semaphore add_remove_card_sem;
 
 static struct usb_device_id mwifiex_usb_table[] = {
        /* 8766 */
@@ -386,6 +385,8 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
        if (!card)
                return -ENOMEM;
 
+       init_completion(&card->fw_done);
+
        id_vendor = le16_to_cpu(udev->descriptor.idVendor);
        id_product = le16_to_cpu(udev->descriptor.idProduct);
        bcd_device = le16_to_cpu(udev->descriptor.bcdDevice);
@@ -475,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
 
        usb_set_intfdata(intf, card);
 
-       ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
+       ret = mwifiex_add_card(card, &card->fw_done, &usb_ops,
                               MWIFIEX_USB, &card->udev->dev);
        if (ret) {
                pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
@@ -601,13 +602,15 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
        struct usb_card_rec *card = usb_get_intfdata(intf);
        struct mwifiex_adapter *adapter;
 
-       if (!card || !card->adapter) {
-               pr_err("%s: card or card->adapter is NULL\n", __func__);
+       if (!card) {
+               dev_err(&intf->dev, "%s: card is NULL\n", __func__);
                return;
        }
 
+       wait_for_completion(&card->fw_done);
+
        adapter = card->adapter;
-       if (!adapter->priv_num)
+       if (!adapter || !adapter->priv_num)
                return;
 
        if (user_rmmod && !adapter->mfg_mode) {
@@ -627,7 +630,7 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
 
        mwifiex_dbg(adapter, FATAL,
                    "%s: removing card\n", __func__);
-       mwifiex_remove_card(adapter, &add_remove_card_sem);
+       mwifiex_remove_card(adapter);
 
        usb_put_dev(interface_to_usbdev(intf));
 }
@@ -1200,8 +1203,7 @@ static struct mwifiex_if_ops usb_ops = {
 
 /* This function initializes the USB driver module.
  *
- * This initiates the semaphore and registers the device with
- * USB bus.
+ * This registers the device with USB bus.
  */
 static int mwifiex_usb_init_module(void)
 {
@@ -1209,8 +1211,6 @@ static int mwifiex_usb_init_module(void)
 
        pr_debug("Marvell USB8797 Driver\n");
 
-       sema_init(&add_remove_card_sem, 1);
-
        ret = usb_register(&mwifiex_usb_driver);
        if (ret)
                pr_err("Driver register failed!\n");
@@ -1230,9 +1230,6 @@ static int mwifiex_usb_init_module(void)
  */
 static void mwifiex_usb_cleanup_module(void)
 {
-       if (!down_interruptible(&add_remove_card_sem))
-               up(&add_remove_card_sem);
-
        /* set the flag as user is removing this module */
        user_rmmod = 1;
 

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.h b/drivers/net/wireless/marvell/mwifiex/usb.h
index 30e8eb8c259d1586b7d29393b875db70826d94f9..e5f204ea018bd8022b5da4f71f8416fadff6d410 100644 (file)
--- a/drivers/net/wireless/marvell/mwifiex/usb.h
+++ b/drivers/net/wireless/marvell/mwifiex/usb.h
@@ -20,6 +20,7 @@
 #ifndef _MWIFIEX_USB_H
 #define _MWIFIEX_USB_H
 
+#include <linux/completion.h>
 #include <linux/usb.h>
 
 #define USB8XXX_VID            0x1286
@@ -75,6 +76,7 @@ struct usb_card_rec {
        struct mwifiex_adapter *adapter;
        struct usb_device *udev;
        struct usb_interface *intf;
+       struct completion fw_done;
        u8 rx_cmd_ep;
        struct urb_context rx_cmd;
        atomic_t rx_cmd_urb_pending;