| Message ID | 20200813031953.13676-3-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: UDC: Fix memory leaks by expanding the API | expand |
On Thu, Aug 13, 2020 at 11:19:49AM +0800, Peter Chen wrote: > From: Alan Stern <stern@rowland.harvard.edu> > > As Anton and Evgeny have noted, the net2280 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(). > > Previous attempts to fix the problems have failed because of lack of > support in the UDC core for separately initializing and adding > gadgets, or for separately deleting and freeing gadgets. The previous > 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 net2280 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 point. > > 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: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Reported-by: Anton Vasilyev <vasilyev@ispras.ru> > Reported-by: Evgeny Novikov <novikov@ispras.ru> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > --- > drivers/usb/gadget/udc/net2280.c | 13 +++++++++---- > drivers/usb/gadget/udc/net2280.h | 1 + > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c > index 7530bd9a08c4..31e49cc34316 100644 > --- a/drivers/usb/gadget/udc/net2280.c > +++ b/drivers/usb/gadget/udc/net2280.c > @@ -3561,7 +3561,9 @@ static irqreturn_t net2280_irq(int irq, void *_dev) > > static void gadget_release(struct device *_dev) > { > - struct net2280 *dev = dev_get_drvdata(_dev); > + struct usb_gadget *gadget = container_of(_dev, > + struct usb_gadget, dev); > + struct net2280 *dev = container_of(gadget, struct net2280, gadget); Please change this to struct net2280 *dev = container_of(_dev, struct net2280, gadget,dev); And do the same for the net2272 patch. Alan Stern
On 20-08-13 09:57:13, Alan Stern wrote: > On Thu, Aug 13, 2020 at 11:19:49AM +0800, Peter Chen wrote: > > From: Alan Stern <stern@rowland.harvard.edu> > > > > As Anton and Evgeny have noted, the net2280 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(). > > > > Previous attempts to fix the problems have failed because of lack of > > support in the UDC core for separately initializing and adding > > gadgets, or for separately deleting and freeing gadgets. The previous > > 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 net2280 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 point. > > > > 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: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Reported-by: Anton Vasilyev <vasilyev@ispras.ru> > > Reported-by: Evgeny Novikov <novikov@ispras.ru> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > --- > > drivers/usb/gadget/udc/net2280.c | 13 +++++++++---- > > drivers/usb/gadget/udc/net2280.h | 1 + > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c > > index 7530bd9a08c4..31e49cc34316 100644 > > --- a/drivers/usb/gadget/udc/net2280.c > > +++ b/drivers/usb/gadget/udc/net2280.c > > @@ -3561,7 +3561,9 @@ static irqreturn_t net2280_irq(int irq, void *_dev) > > > > static void gadget_release(struct device *_dev) > > { > > - struct net2280 *dev = dev_get_drvdata(_dev); > > + struct usb_gadget *gadget = container_of(_dev, > > + struct usb_gadget, dev); > > + struct net2280 *dev = container_of(gadget, struct net2280, gadget); > > Please change this to > > struct net2280 *dev = container_of(_dev, struct net2280, gadget,dev); > > And do the same for the net2272 patch. > Sorry, my oops. Please skip this patch set.
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index 7530bd9a08c4..31e49cc34316 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -3561,7 +3561,9 @@ static irqreturn_t net2280_irq(int irq, void *_dev) static void gadget_release(struct device *_dev) { - struct net2280 *dev = dev_get_drvdata(_dev); + struct usb_gadget *gadget = container_of(_dev, + struct usb_gadget, dev); + struct net2280 *dev = container_of(gadget, struct net2280, gadget); kfree(dev); } @@ -3572,7 +3574,8 @@ static void net2280_remove(struct pci_dev *pdev) { struct net2280 *dev = pci_get_drvdata(pdev); - usb_del_gadget_udc(&dev->gadget); + if (dev->added) + usb_del_gadget(&dev->gadget); BUG_ON(dev->driver); @@ -3603,6 +3606,7 @@ static void net2280_remove(struct pci_dev *pdev) device_remove_file(&pdev->dev, &dev_attr_registers); ep_info(dev, "unbind\n"); + usb_put_gadget(&dev->gadget); } /* wrap this driver around the specified device, but @@ -3624,6 +3628,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) } pci_set_drvdata(pdev, dev); + usb_initialize_gadget(&pdev->dev, &dev->gadget, gadget_release); spin_lock_init(&dev->lock); dev->quirks = id->driver_data; dev->pdev = pdev; @@ -3774,10 +3779,10 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (retval) goto done; - retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget, - gadget_release); + retval = usb_add_gadget(&dev->gadget); if (retval) goto done; + dev->added = 1; return 0; done: diff --git a/drivers/usb/gadget/udc/net2280.h b/drivers/usb/gadget/udc/net2280.h index 85d3ca1698ba..7da3dc1e9729 100644 --- a/drivers/usb/gadget/udc/net2280.h +++ b/drivers/usb/gadget/udc/net2280.h @@ -156,6 +156,7 @@ struct net2280 { softconnect : 1, got_irq : 1, region:1, + added:1, u1_enable:1, u2_enable:1, ltm_enable:1,