[PATCH v5 1/4] mwifiex: reset card->adapter during device unregister

[PATCH v5 1/4] mwifiex: reset card->adapter during device unregister

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

card->adapter gets initialized in mwifiex_register_dev(). As it's not
cleared in mwifiex_unregister_dev(), we may end up accessing the memory
which is already free in below scenario.

Scenario: Driver initialization is failed due to incorrect firmware or
some other reason. Meanwhile device reboot/unload occurs.

Please note that we have 'add_remove_card_sem' semaphore. So if there
is a race betweem init and remove threads, they will execute one after
another. This patch ensures that "card->adapter" is set to NULL when
all cleanup is performed in init failure thread. Later remove thread
can return immediately if "card->adapter" is NULL

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v4: Same as v1, v2, v3
v5: Patch description is updated to get clear picture. There is no race
for init and remove threads as per design. This patch just adds missing
"card->adapter= NULL" change to avoid accessing already freed memory which
leads to a crash.
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
 drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 063c707..302ffd1 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3021,6 +3021,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 			if (card->msi_enable)
 				pci_disable_msi(pdev);
 	       }
+		card->adapter = NULL;
 	}
 }
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 8718950..4cad1c2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 	struct sdio_mmc_card *card = adapter->card;
 
 	if (adapter->card) {
+		card->adapter = NULL;
 		sdio_claim_host(card->func);
 		sdio_disable_func(card->func);
 		sdio_release_host(card->func);
-- 
1.9.1

[PATCH v5 2/4] mwifiex: remove redundant pdev check in suspend/resume handlers

[Amitkumar Karwar]

to_pci_dev() would just do struct offset arithmetic on struct
device to get 'pdev' pointer. We never get NULL pdev pointer

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
New patch introduced in v3 as per inputs from Brian Norris.
v5: Same as v3, v4
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 302ffd1..dd4006e 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -103,14 +103,9 @@ static int mwifiex_pcie_suspend(struct device *dev)
 	struct pcie_service_card *card;
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (pdev) {
-		card = pci_get_drvdata(pdev);
-		if (!card || !card->adapter) {
-			pr_err("Card or adapter structure is not valid\n");
-			return 0;
-		}
-	} else {
-		pr_err("PCIE device is not specified\n");
+	card = pci_get_drvdata(pdev);
+	if (!card || !card->adapter) {
+		pr_err("Card or adapter structure is not valid\n");
 		return 0;
 	}
 
@@ -147,14 +142,9 @@ static int mwifiex_pcie_resume(struct device *dev)
 	struct pcie_service_card *card;
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (pdev) {
-		card = pci_get_drvdata(pdev);
-		if (!card || !card->adapter) {
-			pr_err("Card or adapter structure is not valid\n");
-			return 0;
-		}
-	} else {
-		pr_err("PCIE device is not specified\n");
+	card = pci_get_drvdata(pdev);
+	if (!card || !card->adapter) {
+		pr_err("Card or adapter structure is not valid\n");
 		return 0;
 	}
 
-- 
1.9.1

[PATCH v5 3/4] mwifiex: check hw_status in suspend and resume handlers

[Amitkumar Karwar]

We have observed a kernel crash when system immediately suspends
after booting. There is a race between suspend and driver initialization
paths. This patch adds hw_status checks in suspend/resume to fix this issue
and other corner cases.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Return failure in suspend/resume handler in this scenario.
v3: Handle "hw_status" not READY cases carefully. Return 0 if
init or shutdown is in progress. Return -EBUSY if firmware download
failed or command timeout ocurred(non-recoverable). (Brian Norris)
v4: In resume, we need not return failure.
v5: Same as v4
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index dd4006e..94f75be 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -104,13 +104,22 @@ static int mwifiex_pcie_suspend(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 
 	card = pci_get_drvdata(pdev);
-	if (!card || !card->adapter) {
-		pr_err("Card or adapter structure is not valid\n");
+	if (!card || !card->adapter ||
+	    card->adapter->hw_status == MWIFIEX_HW_STATUS_RESET ||
+	    card->adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY) {
+		pr_err("Card or adapter structure is not valid or hw_status shows failure\n");
 		return 0;
 	}
 
 	adapter = card->adapter;
 
+	if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING ||
+	    adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE ||
+	    adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) {
+		pr_err("We are in the middle of initialzaion or closing\n");
+		return -EBUSY;
+	}
+
 	/* Enable the Host Sleep */
 	if (!mwifiex_enable_hs(adapter)) {
 		mwifiex_dbg(adapter, ERROR,
@@ -143,8 +152,10 @@ static int mwifiex_pcie_resume(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 
 	card = pci_get_drvdata(pdev);
-	if (!card || !card->adapter) {
-		pr_err("Card or adapter structure is not valid\n");
+
+	if (!card || !card->adapter ||
+	    card->adapter->hw_status != MWIFIEX_HW_STATUS_READY) {
+		pr_err("Card or adapter structure is not valid or hw_status is not ready\n");
 		return 0;
 	}
 
-- 
1.9.1

[PATCH v5 4/4] mwifiex: fix race for card->adapter

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

card->adapter is set/reset in register/unregister calls. Firmware gets
downloaded asynchronously in a separate thread during init. We have a
unregister call when firmware initialization fails. It may have a race
with suspend thread where "card->adapter" is being read.

This patch adds spinlock to fix the problem.

Signed-off-by: Cathy Luo <cluo@marvell.com>
Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
New patch introduced in v5 to address race between driver init and suspend
threads.
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 38 +++++++++++++++++++++++------
 drivers/net/wireless/marvell/mwifiex/pcie.h |  2 ++
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 94f75be..9147e6a 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -102,23 +102,32 @@ static int mwifiex_pcie_suspend(struct device *dev)
 	struct mwifiex_adapter *adapter;
 	struct pcie_service_card *card;
 	struct pci_dev *pdev = to_pci_dev(dev);
+	unsigned long flags;
 
 	card = pci_get_drvdata(pdev);
-	if (!card || !card->adapter ||
-	    card->adapter->hw_status == MWIFIEX_HW_STATUS_RESET ||
-	    card->adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY) {
-		pr_err("Card or adapter structure is not valid or hw_status shows failure\n");
+	if (!card) {
+		pr_err("Card is not valid\n");
 		return 0;
 	}
 
+	spin_lock_irqsave(&card->adapter_lock, flags);
 	adapter = card->adapter;
+	if (!adapter ||
+	    adapter->hw_status == MWIFIEX_HW_STATUS_RESET ||
+	    adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY) {
+		spin_unlock_irqrestore(&card->adapter_lock, flags);
+		pr_err("Adapter structure is not valid or hw_status shows failure\n");
+		return 0;
+	}
 
 	if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING ||
 	    adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE ||
 	    adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) {
+		spin_unlock_irqrestore(&card->adapter_lock, flags);
 		pr_err("We are in the middle of initialzaion or closing\n");
 		return -EBUSY;
 	}
+	spin_unlock_irqrestore(&card->adapter_lock, flags);
 
 	/* Enable the Host Sleep */
 	if (!mwifiex_enable_hs(adapter)) {
@@ -150,16 +159,22 @@ static int mwifiex_pcie_resume(struct device *dev)
 	struct mwifiex_adapter *adapter;
 	struct pcie_service_card *card;
 	struct pci_dev *pdev = to_pci_dev(dev);
+	unsigned long flags;
 
 	card = pci_get_drvdata(pdev);
-
-	if (!card || !card->adapter ||
-	    card->adapter->hw_status != MWIFIEX_HW_STATUS_READY) {
-		pr_err("Card or adapter structure is not valid or hw_status is not ready\n");
+	if (!card) {
+		pr_err("Card is not valid\n");
 		return 0;
 	}
 
+	spin_lock_irqsave(&card->adapter_lock, flags);
 	adapter = card->adapter;
+	if (!adapter || adapter->hw_status != MWIFIEX_HW_STATUS_READY) {
+		spin_unlock_irqrestore(&card->adapter_lock, flags);
+		pr_err("adapter structure is not valid or hw_status is not ready\n");
+		return 0;
+	}
+	spin_unlock_irqrestore(&card->adapter_lock, flags);
 
 	if (!adapter->is_suspended) {
 		mwifiex_dbg(adapter, WARN,
@@ -195,6 +210,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 		return -ENOMEM;
 
 	card->dev = pdev;
+	spin_lock_init(&card->adapter_lock);
 
 	if (ent->driver_data) {
 		struct mwifiex_pcie_device *data = (void *)ent->driver_data;
@@ -2973,9 +2989,12 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 {
 	struct pcie_service_card *card = adapter->card;
 	struct pci_dev *pdev = card->dev;
+	unsigned long flags;
 
+	spin_lock_irqsave(&card->adapter_lock, flags);
 	/* save adapter pointer in card */
 	card->adapter = adapter;
+	spin_unlock_irqrestore(&card->adapter_lock, flags);
 	adapter->dev = &pdev->dev;
 
 	if (mwifiex_pcie_request_irq(adapter))
@@ -3001,6 +3020,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 	struct pcie_service_card *card = adapter->card;
 	struct pci_dev *pdev;
 	int i;
+	unsigned long flags;
 
 	if (card) {
 		pdev = card->dev;
@@ -3022,7 +3042,9 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 			if (card->msi_enable)
 				pci_disable_msi(pdev);
 	       }
+		spin_lock_irqsave(&card->adapter_lock, flags);
 		card->adapter = NULL;
+		spin_unlock_irqrestore(&card->adapter_lock, flags);
 	}
 }
 
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h
index 46f99ca..f6e20ea 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -344,6 +344,8 @@ struct mwifiex_msix_context {
 struct pcie_service_card {
 	struct pci_dev *dev;
 	struct mwifiex_adapter *adapter;
+	/* spin lock for card->adapter */
+	spinlock_t adapter_lock;
 	struct mwifiex_pcie_device pcie;
 
 	u8 txbd_flush;
-- 
1.9.1

Re: [PATCH v5 1/4] mwifiex: reset card->adapter during device unregister

[Brian Norris]

On Thu, Oct 20, 2016 at 06:56:16PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> card->adapter gets initialized in mwifiex_register_dev(). As it's not
> cleared in mwifiex_unregister_dev(), we may end up accessing the memory
> which is already free in below scenario.
> 
> Scenario: Driver initialization is failed due to incorrect firmware or
> some other reason. Meanwhile device reboot/unload occurs.
> 
> Please note that we have 'add_remove_card_sem' semaphore. So if there
> is a race betweem init and remove threads, they will execute one after
> another.

I argued in v4 [1] that this is false, and therefore this patch isn't
really correct. Carrying the NACK here.

Brian

[1] https://patchwork.kernel.org/patch/9365209/

> This patch ensures that "card->adapter" is set to NULL when
> all cleanup is performed in init failure thread. Later remove thread
> can return immediately if "card->adapter" is NULL
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v4: Same as v1, v2, v3
> v5: Patch description is updated to get clear picture. There is no race
> for init and remove threads as per design. This patch just adds missing
> "card->adapter= NULL" change to avoid accessing already freed memory which
> leads to a crash.
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 063c707..302ffd1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -3021,6 +3021,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
>  			if (card->msi_enable)
>  				pci_disable_msi(pdev);
>  	       }
> +		card->adapter = NULL;
>  	}
>  }
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 8718950..4cad1c2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
>  	struct sdio_mmc_card *card = adapter->card;
>  
>  	if (adapter->card) {
> +		card->adapter = NULL;
>  		sdio_claim_host(card->func);
>  		sdio_disable_func(card->func);
>  		sdio_release_host(card->func);
> -- 
> 1.9.1
>