From: Xinming Hu <[email protected]>
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 <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
---
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
On Thu, Oct 20, 2016 at 06:56:16PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Amitkumar Karwar <[email protected]>
> ---
> 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
>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
From: Xinming Hu <[email protected]>
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 <[email protected]>
Signed-off-by: Xinming Hu <[email protected]>
Signed-off-by: Amitkumar Karwar <[email protected]>
---
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