linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Peter Chen <peter.chen@nxp.com>
To: balbi@kernel.org
Cc: linux-usb@vger.kernel.org, linux-imx@nxp.com,
	gregkh@linuxfoundation.org, stern@rowland.harvard.edu,
	Anton Vasilyev <vasilyev@ispras.ru>,
	Evgeny Novikov <novikov@ispras.ru>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Peter Chen <peter.chen@nxp.com>
Subject: [PATCH v2 3/6] USB: UDC: net2272: Fix memory leaks
Date: Mon, 10 Aug 2020 10:25:07 +0800	[thread overview]
Message-ID: <20200810022510.6516-4-peter.chen@nxp.com> (raw)
In-Reply-To: <20200810022510.6516-1-peter.chen@nxp.com>

From: Alan Stern <stern@rowland.harvard.edu>

Like net2280 (on which it was based), the net2272 UDC driver has a
problem with leaking memory along some of its failure pathways.  It
also has another problem, not previously noted, in that some of the
failure pathways will call usb_del_gadget_udc() without first calling
usb_add_gadget_udc_release().  And it leaks memory by calling kfree()
when it should call put_device().

Until now it has been impossible to handle the memory leaks, because of
lack of support in the UDC core for separately initializing and adding
gadgets, or for separately deleting and freeing gadgets.  An earlier
patch in this series adds the necessary support, making it possible to
fix the outstanding problems properly.

This patch adds an "added" flag to the net2272 structure to indicate
whether or not the gadget has been registered (and thus whether or not
to call usb_del_gadget()), and it fixes the deallocation issues by
calling usb_put_gadget() at the appropriate places.

A similar memory leak issue, apparently never before recognized, stems
from the fact that the driver never initializes the drvdata field in
the gadget's embedded struct device!  Evidently this wasn't noticed
because the pointer is only ever used as an argument to kfree(), which
doesn't mind getting called with a NULL pointer. In fact, the drvdata
for gadget device will be written by usb_composite_dev structure if
any gadget class is loaded, so it needs to use usb_gadget structure
to get net2280 private data.

CC: Anton Vasilyev <vasilyev@ispras.ru>
CC: Evgeny Novikov <novikov@ispras.ru>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
Changes from v1:
- Simplify the method of get the net2272 private data pointer at .release function
- Add my Sob

Changes from RFC:
- Using usb_gadget structure to get net2272 private data, and
update commit log accoringly.

 drivers/usb/gadget/udc/net2272.c | 23 +++++++++++++----------
 drivers/usb/gadget/udc/net2272.h |  1 +
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c
index fbbe62513545..7046b7eda900 100644
--- a/drivers/usb/gadget/udc/net2272.c
+++ b/drivers/usb/gadget/udc/net2272.c
@@ -2196,7 +2196,8 @@ static int net2272_present(struct net2272 *dev)
 static void
 net2272_gadget_release(struct device *_dev)
 {
-	struct net2272 *dev = dev_get_drvdata(_dev);
+	struct net2272 *dev = container_of(_dev, struct net2272, gadget.dev);
+
 	kfree(dev);
 }
 
@@ -2205,7 +2206,8 @@ net2272_gadget_release(struct device *_dev)
 static void
 net2272_remove(struct net2272 *dev)
 {
-	usb_del_gadget_udc(&dev->gadget);
+	if (dev->added)
+		usb_del_gadget(&dev->gadget);
 	free_irq(dev->irq, dev);
 	iounmap(dev->base_addr);
 	device_remove_file(dev->dev, &dev_attr_registers);
@@ -2235,6 +2237,7 @@ static struct net2272 *net2272_probe_init(struct device *dev, unsigned int irq)
 
 	/* the "gadget" abstracts/virtualizes the controller */
 	ret->gadget.name = driver_name;
+	usb_initialize_gadget(dev, &ret->gadget, net2272_gadget_release);
 
 	return ret;
 }
@@ -2273,10 +2276,10 @@ net2272_probe_fin(struct net2272 *dev, unsigned int irqflags)
 	if (ret)
 		goto err_irq;
 
-	ret = usb_add_gadget_udc_release(dev->dev, &dev->gadget,
-			net2272_gadget_release);
+	ret = usb_add_gadget(&dev->gadget);
 	if (ret)
 		goto err_add_udc;
+	dev->added = 1;
 
 	return 0;
 
@@ -2449,7 +2452,7 @@ net2272_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	if (pci_enable_device(pdev) < 0) {
 		ret = -ENODEV;
-		goto err_free;
+		goto err_put;
 	}
 
 	pci_set_master(pdev);
@@ -2472,8 +2475,8 @@ net2272_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
  err_pci:
 	pci_disable_device(pdev);
- err_free:
-	kfree(dev);
+ err_put:
+	usb_put_gadget(&dev->gadget);
 
 	return ret;
 }
@@ -2534,7 +2537,7 @@ net2272_pci_remove(struct pci_dev *pdev)
 
 	pci_disable_device(pdev);
 
-	kfree(dev);
+	usb_put_gadget(&dev->gadget);
 }
 
 /* Table of matching PCI IDs */
@@ -2647,7 +2650,7 @@ net2272_plat_probe(struct platform_device *pdev)
  err_req:
 	release_mem_region(base, len);
  err:
-	kfree(dev);
+	usb_put_gadget(&dev->gadget);
 
 	return ret;
 }
@@ -2662,7 +2665,7 @@ net2272_plat_remove(struct platform_device *pdev)
 	release_mem_region(pdev->resource[0].start,
 		resource_size(&pdev->resource[0]));
 
-	kfree(dev);
+	usb_put_gadget(&dev->gadget);
 
 	return 0;
 }
diff --git a/drivers/usb/gadget/udc/net2272.h b/drivers/usb/gadget/udc/net2272.h
index 87d0ab9ffeeb..c669308111c2 100644
--- a/drivers/usb/gadget/udc/net2272.h
+++ b/drivers/usb/gadget/udc/net2272.h
@@ -441,6 +441,7 @@ struct net2272 {
 	unsigned protocol_stall:1,
 	         softconnect:1,
 	         wakeup:1,
+		 added:1,
 	         dma_eot_polarity:1,
 	         dma_dack_polarity:1,
 	         dma_dreq_polarity:1,
-- 
2.17.1


  parent reply	other threads:[~2020-08-10  2:26 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-10  2:25 [PATCH v2 0/6] USB: UDC: Fix memory leaks by expanding the API Peter Chen
2020-08-10  2:25 ` [PATCH v2 1/6] USB: UDC: Expand device model API interface Peter Chen
2020-08-10  2:25 ` [PATCH v2 2/6] USB: UDC: net2280: Fix memory leaks Peter Chen
2020-08-10  2:25 ` Peter Chen [this message]
2020-08-10  2:25 ` [PATCH v2 4/6] usb: cdns3: gadget: fix possible memory leak Peter Chen
2020-08-10  2:25 ` [PATCH v2 5/6] usb: dwc3: allocate gadget structure dynamically Peter Chen
2020-08-10  2:25 ` [PATCH v2 6/6] Revert "usb: udc: allow adding and removing the same gadget device" Peter Chen
2020-08-18  9:33   ` Greg KH
2020-08-18 10:05     ` Peter Chen
2020-08-18 14:46       ` stern
2020-08-19  1:31         ` Peter Chen
2020-08-19 14:52           ` stern
2020-08-20  3:40             ` Peter Chen
2020-08-20 14:25               ` stern
2020-08-18  9:33 ` [PATCH v2 0/6] USB: UDC: Fix memory leaks by expanding the API Greg KH
2020-08-18 10:07   ` Peter Chen

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=20200810022510.6516-4-peter.chen@nxp.com \
    --to=peter.chen@nxp.com \
    --cc=balbi@kernel.org \
    --cc=benh@kernel.crashing.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=novikov@ispras.ru \
    --cc=stern@rowland.harvard.edu \
    --cc=vasilyev@ispras.ru \
    /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).