linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Amitkumar Karwar <akarwar@marvell.com>,
	Nishant Sarmukadam <nishants@marvell.com>
Cc: <linux-kernel@vger.kernel.org>, Kalle Valo <kvalo@codeaurora.org>,
	linux-wireless@vger.kernel.org, Cathy Luo <cluo@marvell.com>,
	Rajat Jain <rajatja@google.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Brian Norris <briannorris@chromium.org>
Subject: [PATCH] mwifiex: don't do unbalanced free()'ing in cleanup_if()
Date: Wed, 26 Oct 2016 16:29:20 -0700	[thread overview]
Message-ID: <1477524560-49226-1-git-send-email-briannorris@chromium.org> (raw)

The cleanup_if() callback is the inverse of init_if(). We allocate our
'card' interface structure in the probe() function, but we free it in
cleanup_if(). That gives a few problems:
(a) we leak this memory if probe() fails before we reach init_if()
(b) we can't safely utilize 'card' after cleanup_if() -- namely, in
    remove() or suspend(), both of which might race with the cleanup
    paths in our asynchronous FW initialization path

Solution: just use devm_kzalloc(), which will free this structure
properly when the device is removed -- and drop the set_drvdata(...,
NULL), since the driver core does this for us. This also removes the
temptation to use drvdata == NULL as a hack for checking if the device
has been "cleaned up."

I *do* leave the set_drvdata(..., NULL) for the hacky SDIO
mwifiex_recreate_adapter(), since the device core won't be able to clear
that one for us.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c |  5 +----
 drivers/net/wireless/marvell/mwifiex/sdio.c | 16 ++++++++++------
 drivers/net/wireless/marvell/mwifiex/usb.c  |  7 +------
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 063c707844d3..3047c1ab944a 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -189,7 +189,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 	pr_debug("info: vendor=0x%4.04X device=0x%4.04X rev=%d\n",
 		 pdev->vendor, pdev->device, pdev->revision);
 
-	card = kzalloc(sizeof(struct pcie_service_card), GFP_KERNEL);
+	card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL);
 	if (!card)
 		return -ENOMEM;
 
@@ -2815,7 +2815,6 @@ static int mwifiex_pcie_init(struct mwifiex_adapter *adapter)
 err_set_dma_mask:
 	pci_disable_device(pdev);
 err_enable_dev:
-	pci_set_drvdata(pdev, NULL);
 	return ret;
 }
 
@@ -2849,9 +2848,7 @@ static void mwifiex_pcie_cleanup(struct mwifiex_adapter *adapter)
 		pci_disable_device(pdev);
 		pci_release_region(pdev, 2);
 		pci_release_region(pdev, 0);
-		pci_set_drvdata(pdev, NULL);
 	}
-	kfree(card);
 }
 
 static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 8718950004f3..f04cf5a551b3 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -152,7 +152,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
 	pr_debug("info: vendor=0x%4.04X device=0x%4.04X class=%d function=%d\n",
 		 func->vendor, func->device, func->class, func->num);
 
-	card = kzalloc(sizeof(struct sdio_mmc_card), GFP_KERNEL);
+	card = devm_kzalloc(&func->dev, sizeof(*card), GFP_KERNEL);
 	if (!card)
 		return -ENOMEM;
 
@@ -185,7 +185,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
 
 	if (ret) {
 		dev_err(&func->dev, "failed to enable function\n");
-		goto err_free;
+		return ret;
 	}
 
 	/* device tree node parsing and platform specific configuration*/
@@ -210,8 +210,6 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
 	sdio_claim_host(func);
 	sdio_disable_func(func);
 	sdio_release_host(func);
-err_free:
-	kfree(card);
 
 	return ret;
 }
@@ -2240,8 +2238,6 @@ static void mwifiex_cleanup_sdio(struct mwifiex_adapter *adapter)
 	kfree(card->mpa_rx.len_arr);
 	kfree(card->mpa_tx.buf);
 	kfree(card->mpa_rx.buf);
-	sdio_set_drvdata(card->func, NULL);
-	kfree(card);
 }
 
 /*
@@ -2291,6 +2287,14 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
 
 	mwifiex_sdio_remove(func);
 
+	/*
+	 * Normally, we would let the driver core take care of releasing these.
+	 * But we're not letting the driver core handle this one. See above
+	 * TODO.
+	 */
+	sdio_set_drvdata(func, NULL);
+	devm_kfree(&func->dev, card);
+
 	/* power cycle the adapter */
 	sdio_claim_host(func);
 	mmc_hw_reset(func->card->host);
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 73eb0846db21..57ed834ba296 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -382,7 +382,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
 	struct usb_card_rec *card;
 	u16 id_vendor, id_product, bcd_device, bcd_usb;
 
-	card = kzalloc(sizeof(struct usb_card_rec), GFP_KERNEL);
+	card = devm_kzalloc(&intf->dev, sizeof(*card), GFP_KERNEL);
 	if (!card)
 		return -ENOMEM;
 
@@ -480,7 +480,6 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
 	if (ret) {
 		pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
 		usb_reset_device(udev);
-		kfree(card);
 		return ret;
 	}
 
@@ -630,11 +629,7 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
 		    "%s: removing card\n", __func__);
 	mwifiex_remove_card(adapter, &add_remove_card_sem);
 
-	usb_set_intfdata(intf, NULL);
 	usb_put_dev(interface_to_usbdev(intf));
-	kfree(card);
-
-	return;
 }
 
 static struct usb_driver mwifiex_usb_driver = {
-- 
2.8.0.rc3.226.g39d4020

             reply	other threads:[~2016-10-26 23:30 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-26 23:29 Brian Norris [this message]
2016-10-26 23:35 ` [PATCH] mwifiex: don't do unbalanced free()'ing in cleanup_if() Dmitry Torokhov
2016-10-26 23:43   ` Brian Norris
2016-10-26 23:52     ` Dmitry Torokhov
2016-11-18 11:25 ` Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1477524560-49226-1-git-send-email-briannorris@chromium.org \
    --to=briannorris@chromium.org \
    --cc=akarwar@marvell.com \
    --cc=cluo@marvell.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=nishants@marvell.com \
    --cc=rajatja@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).