* [PATCH RFC 3/4] USB: UDC: net2280: Fix memory leaks
@ 2020-07-29 20:23 Alan Stern
2020-08-07 3:35 ` Peter Chen
0 siblings, 1 reply; 3+ messages in thread
From: Alan Stern @ 2020-07-29 20:23 UTC (permalink / raw)
To: Felipe Balbi
Cc: Anton Vasilyev, Evgeny Novikov, Benjamin Herrenschmidt, USB mailing list
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.
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-By: Anton Vasilyev <vasilyev@ispras.ru>
Reported-By: Evgeny Novikov <novikov@ispras.ru>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
drivers/usb/gadget/udc/net2280.c | 10 +++++++---
drivers/usb/gadget/udc/net2280.h | 1 +
2 files changed, 8 insertions(+), 3 deletions(-)
Index: usb-devel/drivers/usb/gadget/udc/net2280.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/net2280.c
+++ usb-devel/drivers/usb/gadget/udc/net2280.c
@@ -3572,7 +3572,8 @@ static void net2280_remove(struct pci_de
{
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 +3604,7 @@ static void net2280_remove(struct pci_de
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 +3626,8 @@ static int net2280_probe(struct pci_dev
}
pci_set_drvdata(pdev, dev);
+ dev_set_drvdata(&dev->gadget.dev, 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 +3778,10 @@ static int net2280_probe(struct pci_dev
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:
Index: usb-devel/drivers/usb/gadget/udc/net2280.h
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/net2280.h
+++ usb-devel/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,
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH RFC 3/4] USB: UDC: net2280: Fix memory leaks
2020-07-29 20:23 [PATCH RFC 3/4] USB: UDC: net2280: Fix memory leaks Alan Stern
@ 2020-08-07 3:35 ` Peter Chen
2020-08-07 9:44 ` Peter Chen
0 siblings, 1 reply; 3+ messages in thread
From: Peter Chen @ 2020-08-07 3:35 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Anton Vasilyev, Evgeny Novikov,
Benjamin Herrenschmidt, USB mailing list
On 20-07-29 16:23:28, Alan Stern wrote:
> 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.
>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-By: Anton Vasilyev <vasilyev@ispras.ru>
> Reported-By: Evgeny Novikov <novikov@ispras.ru>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> ---
>
> drivers/usb/gadget/udc/net2280.c | 10 +++++++---
> drivers/usb/gadget/udc/net2280.h | 1 +
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> Index: usb-devel/drivers/usb/gadget/udc/net2280.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/udc/net2280.c
> +++ usb-devel/drivers/usb/gadget/udc/net2280.c
> @@ -3572,7 +3572,8 @@ static void net2280_remove(struct pci_de
> {
> 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 +3604,7 @@ static void net2280_remove(struct pci_de
> 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 +3626,8 @@ static int net2280_probe(struct pci_dev
> }
>
> pci_set_drvdata(pdev, dev);
> + dev_set_drvdata(&dev->gadget.dev, dev);
The gadget device's drvdata will be written by usb_composite_dev
structure pointer at composite_bind, composite_bind is called after
loading any gadget class drivers, so you can't depend on it at
gadget_release.
Peter
> + usb_initialize_gadget(&pdev->dev, &dev->gadget, gadget_release);
> spin_lock_init(&dev->lock);
> dev->quirks = id->driver_data;
> dev->pdev = pdev;
> @@ -3774,10 +3778,10 @@ static int net2280_probe(struct pci_dev
> 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:
> Index: usb-devel/drivers/usb/gadget/udc/net2280.h
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/udc/net2280.h
> +++ usb-devel/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,
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH RFC 3/4] USB: UDC: net2280: Fix memory leaks
2020-08-07 3:35 ` Peter Chen
@ 2020-08-07 9:44 ` Peter Chen
0 siblings, 0 replies; 3+ messages in thread
From: Peter Chen @ 2020-08-07 9:44 UTC (permalink / raw)
To: Alan Stern
Cc: Felipe Balbi, Anton Vasilyev, Evgeny Novikov,
Benjamin Herrenschmidt, USB mailing list
On 20-08-07 03:35:17, Peter Chen wrote:
> On 20-07-29 16:23:28, Alan Stern wrote:
> > 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.
> >
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Reported-By: Anton Vasilyev <vasilyev@ispras.ru>
> > Reported-By: Evgeny Novikov <novikov@ispras.ru>
> > CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >
> > ---
> >
> > drivers/usb/gadget/udc/net2280.c | 10 +++++++---
> > drivers/usb/gadget/udc/net2280.h | 1 +
> > 2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > Index: usb-devel/drivers/usb/gadget/udc/net2280.c
> > ===================================================================
> > --- usb-devel.orig/drivers/usb/gadget/udc/net2280.c
> > +++ usb-devel/drivers/usb/gadget/udc/net2280.c
> > @@ -3572,7 +3572,8 @@ static void net2280_remove(struct pci_de
> > {
> > 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 +3604,7 @@ static void net2280_remove(struct pci_de
> > 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 +3626,8 @@ static int net2280_probe(struct pci_dev
> > }
> >
> > pci_set_drvdata(pdev, dev);
> > + dev_set_drvdata(&dev->gadget.dev, dev);
>
> The gadget device's drvdata will be written by usb_composite_dev
> structure pointer at composite_bind, composite_bind is called after
> loading any gadget class drivers, so you can't depend on it at
> gadget_release.
>
I do some fixes about it for the new series, if anything is not suitable,
please commit.
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-07 9:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29 20:23 [PATCH RFC 3/4] USB: UDC: net2280: Fix memory leaks Alan Stern
2020-08-07 3:35 ` Peter Chen
2020-08-07 9:44 ` Peter Chen
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).