| Message ID | 20200729202231.GB1584059@rowland.harvard.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: UDC: Fix memory leaks by expanding the API | expand |
On 20-07-29 16:22:31, Alan Stern wrote: > Abusing the kernel's device model, some UDC drivers (including > dwc3 and cdns3) register and unregister their gadget structures > multiple times. This is strictly forbidden; device structures may not > be reused. Register and unregister gadget structures multiple times should be allowed if we pass a clean (zeroed) gadget device structure. I checked the cdns3 code (cdns3_gadget_start), it always zeroed struct usb_gadget before calling usb_add_gadget_udc when start device mode. > > Commit fac323471df6 ("usb: udc: allow adding and removing the same > gadget device") attempted to work around this restriction by zeroing > out the memory occupied by a gadget's embedded struct device when the > gadget is unregistered. However, it does so at the wrong time, > immediately following the call to device_unregister(). At that point > there may still be outstanding references to the device, and > overwriting its memory is likely to cause trouble. Even worse, if > there are no outstanding references then the gadget's memory may have > been deallocated, and so gadget must be considered to be a stale > pointer when it is passed to memset(). > > This patch fixes the problem by moving the offending memset to the > device's release routine, which gets called only when all references > have been dropped. (Actually the call gets moved to the default > release routine, renamed from "usb_udc_nop_release" to > "usb_udc_zero_release" to indicate that it now zeroes out the memory. > This routine is the one used by dwc3 and cdns3; other drivers may not > use it.) In fact, many new written UDC drivers uses usb_add_gadget_udc directly which uses default .release function defined at core.c. > > This doesn't fix the underlying problem. UDC drivers that register > their gadgets multiple times should be rewritten to allocate their > gadget structures dynamically, using a new one each time. Until that > is done, this will at least remove one potential source of errors. > > On the other hand, the patch may create new errors if the release > routine doesn't get called until after the gadget has been > re-registered. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > CC: Roger Quadros <rogerq@ti.com> > CC: Peter Chen <peter.chen@nxp.com> > CC: Anton Vasilyev <vasilyev@ispras.ru> > CC: Evgeny Novikov <novikov@ispras.ru> > CC: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > --- > > drivers/usb/gadget/udc/core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > Index: usb-devel/drivers/usb/gadget/udc/core.c > =================================================================== > --- usb-devel.orig/drivers/usb/gadget/udc/core.c > +++ usb-devel/drivers/usb/gadget/udc/core.c > @@ -1138,9 +1138,10 @@ static void usb_udc_release(struct devic > > static const struct attribute_group *usb_udc_attr_groups[]; > > -static void usb_udc_nop_release(struct device *dev) > +static void usb_udc_zero_release(struct device *dev) > { > dev_vdbg(dev, "%s\n", __func__); > + memset(dev, 0, sizeof(*dev)); > } > > /* should be called with udc_lock held */ > @@ -1184,7 +1185,7 @@ int usb_add_gadget_udc_release(struct de > if (release) > gadget->dev.release = release; > else > - gadget->dev.release = usb_udc_nop_release; > + gadget->dev.release = usb_udc_zero_release; > > device_initialize(&gadget->dev); According to kernel-doc for device_initialize * All fields in @dev must be initialized by the caller to 0, except * for those explicitly set to some other value. The simplest * approach is to use kzalloc() to allocate the structure containing * @dev. * Is it better to zeroed gadget->dev before calling device_initialize? > > @@ -1338,7 +1339,6 @@ void usb_del_gadget_udc(struct usb_gadge > flush_work(&gadget->work); > device_unregister(&udc->dev); > device_unregister(&gadget->dev); > - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > } > EXPORT_SYMBOL_GPL(usb_del_gadget_udc); >
On Thu, Jul 30, 2020 at 03:28:09AM +0000, Peter Chen wrote: > On 20-07-29 16:22:31, Alan Stern wrote: > > Abusing the kernel's device model, some UDC drivers (including > > dwc3 and cdns3) register and unregister their gadget structures > > multiple times. This is strictly forbidden; device structures may not > > be reused. > > Register and unregister gadget structures multiple times should be > allowed if we pass a clean (zeroed) gadget device structure. I checked > the cdns3 code (cdns3_gadget_start), it always zeroed struct usb_gadget > before calling usb_add_gadget_udc when start device mode. How do you "know" that the structure really was properly freed/released by the driver core at that point in time? That's the issue, even if you do unregister it, the driver core, or any other part of the kernel, can hold on to the memory for an unbounded amount of time, due to the fact that this is a reference counted pointer. So please, never "recycle" memory structures like this. The documentation for the kernel explicitly says "do not do this!" thanks, greg k-h
> On Thu, Jul 30, 2020 at 03:28:09AM +0000, Peter Chen wrote: > > On 20-07-29 16:22:31, Alan Stern wrote: > > > Abusing the kernel's device model, some UDC drivers (including > > > dwc3 and cdns3) register and unregister their gadget structures > > > multiple times. This is strictly forbidden; device structures may > > > not be reused. > > > > Register and unregister gadget structures multiple times should be > > allowed if we pass a clean (zeroed) gadget device structure. I checked > > the cdns3 code (cdns3_gadget_start), it always zeroed struct > > usb_gadget before calling usb_add_gadget_udc when start device mode. > > How do you "know" that the structure really was properly freed/released by the > driver core at that point in time? > > That's the issue, even if you do unregister it, the driver core, or any other part of > the kernel, can hold on to the memory for an unbounded amount of time, due to the > fact that this is a reference counted pointer. > Yes, I find many UDC drivers have issues for that. The UDC driver's .remove has no sync with device reference counter for gadget device, they free device private structure which contains gadget device unconditional without considering device reference counter for gadget device. The UDC driver may need to call get_device/put_device for &gadget->dev to sync with device core, and at gadget->dev .release callback, free the whole device's private structure. So, a common .release (usb_udc_nop_release) callback at core.c may not suitable for most of UDC driver. > So please, never "recycle" memory structures like this. The documentation for the > kernel explicitly says "do not do this!" Yes, "recycle" memory structures have issue, but at cdns3 driver(cdns3/gadget.c), it does not use the same memory, it always allocates a NEW memory region for device structure before adding to device core. Peter
On Thu, Jul 30, 2020 at 07:19:04AM +0200, Greg KH wrote: > On Thu, Jul 30, 2020 at 03:28:09AM +0000, Peter Chen wrote: > > On 20-07-29 16:22:31, Alan Stern wrote: > > > Abusing the kernel's device model, some UDC drivers (including > > > dwc3 and cdns3) register and unregister their gadget structures > > > multiple times. This is strictly forbidden; device structures may not > > > be reused. > > > > Register and unregister gadget structures multiple times should be > > allowed if we pass a clean (zeroed) gadget device structure. I checked > > the cdns3 code (cdns3_gadget_start), it always zeroed struct usb_gadget > > before calling usb_add_gadget_udc when start device mode. > > How do you "know" that the structure really was properly freed/released > by the driver core at that point in time? > > That's the issue, even if you do unregister it, the driver core, or any > other part of the kernel, can hold on to the memory for an unbounded > amount of time, due to the fact that this is a reference counted > pointer. In theory, you can know that the driver core is done using a structure if you wait for the release routine to run. But of course, that can mean you have to wait an indefinitely long time. Alan Stern > So please, never "recycle" memory structures like this. The > documentation for the kernel explicitly says "do not do this!" > > thanks, > > greg k-h
Index: usb-devel/drivers/usb/gadget/udc/core.c =================================================================== --- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c @@ -1138,9 +1138,10 @@ static void usb_udc_release(struct devic static const struct attribute_group *usb_udc_attr_groups[]; -static void usb_udc_nop_release(struct device *dev) +static void usb_udc_zero_release(struct device *dev) { dev_vdbg(dev, "%s\n", __func__); + memset(dev, 0, sizeof(*dev)); } /* should be called with udc_lock held */ @@ -1184,7 +1185,7 @@ int usb_add_gadget_udc_release(struct de if (release) gadget->dev.release = release; else - gadget->dev.release = usb_udc_nop_release; + gadget->dev.release = usb_udc_zero_release; device_initialize(&gadget->dev); @@ -1338,7 +1339,6 @@ void usb_del_gadget_udc(struct usb_gadge flush_work(&gadget->work); device_unregister(&udc->dev); device_unregister(&gadget->dev); - memset(&gadget->dev, 0x00, sizeof(gadget->dev)); } EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
Abusing the kernel's device model, some UDC drivers (including dwc3 and cdns3) register and unregister their gadget structures multiple times. This is strictly forbidden; device structures may not be reused. Commit fac323471df6 ("usb: udc: allow adding and removing the same gadget device") attempted to work around this restriction by zeroing out the memory occupied by a gadget's embedded struct device when the gadget is unregistered. However, it does so at the wrong time, immediately following the call to device_unregister(). At that point there may still be outstanding references to the device, and overwriting its memory is likely to cause trouble. Even worse, if there are no outstanding references then the gadget's memory may have been deallocated, and so gadget must be considered to be a stale pointer when it is passed to memset(). This patch fixes the problem by moving the offending memset to the device's release routine, which gets called only when all references have been dropped. (Actually the call gets moved to the default release routine, renamed from "usb_udc_nop_release" to "usb_udc_zero_release" to indicate that it now zeroes out the memory. This routine is the one used by dwc3 and cdns3; other drivers may not use it.) This doesn't fix the underlying problem. UDC drivers that register their gadgets multiple times should be rewritten to allocate their gadget structures dynamically, using a new one each time. Until that is done, this will at least remove one potential source of errors. On the other hand, the patch may create new errors if the release routine doesn't get called until after the gadget has been re-registered. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> CC: Roger Quadros <rogerq@ti.com> CC: Peter Chen <peter.chen@nxp.com> CC: Anton Vasilyev <vasilyev@ispras.ru> CC: Evgeny Novikov <novikov@ispras.ru> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- drivers/usb/gadget/udc/core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)