| Message ID | f37e289c25f9219112b866153f35679836ef12a4.1523870350.git.tovmasya@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Grigor, > Grigor Tovmasyan <Grigor.Tovmasyan@synopsys.com> hat am 16. April 2018 um 12:16 geschrieben: > > > In dwc2_gadget_init() we allocate EP0 request via > dwc2_hsotg_ep_alloc_request(). After that there are > usb_add_gadget_udc() call and if it failed, then > ctrl_req will not be freed and will cause memory leak. > > Reordered function calls in gadget_init: moved up usb_add_gadget_udc() > before dwc2_hsotg_ep_alloc_request(). i'm not sure, but doesn't this change introduce a race condition before EP0 request has been allocated? Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stefan, On 4/18/2018 1:11 AM, Stefan Wahren wrote: > Hi Grigor, > >> Grigor Tovmasyan <Grigor.Tovmasyan@synopsys.com> hat am 16. April 2018 um 12:16 geschrieben: >> >> >> In dwc2_gadget_init() we allocate EP0 request via >> dwc2_hsotg_ep_alloc_request(). After that there are >> usb_add_gadget_udc() call and if it failed, then >> ctrl_req will not be freed and will cause memory leak. >> >> Reordered function calls in gadget_init: moved up usb_add_gadget_udc() >> before dwc2_hsotg_ep_alloc_request(). > > i'm not sure, but doesn't this change introduce a race condition before EP0 request has been allocated? As far as I know the firt request to EP0 coming when the function driver first bind() to the device, which happens after dwc2 probe. So, in my opinion this patch is safe. Grigor > > Stefan > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 23.04.2018 um 09:05 schrieb Grigor Tovmasyan: > Hi Stefan, > > On 4/18/2018 1:11 AM, Stefan Wahren wrote: >> Hi Grigor, >> >>> Grigor Tovmasyan <Grigor.Tovmasyan@synopsys.com> hat am 16. April 2018 um 12:16 geschrieben: >>> >>> >>> In dwc2_gadget_init() we allocate EP0 request via >>> dwc2_hsotg_ep_alloc_request(). After that there are >>> usb_add_gadget_udc() call and if it failed, then >>> ctrl_req will not be freed and will cause memory leak. >>> >>> Reordered function calls in gadget_init: moved up usb_add_gadget_udc() >>> before dwc2_hsotg_ep_alloc_request(). >> i'm not sure, but doesn't this change introduce a race condition before EP0 request has been allocated? > As far as I know the firt request to EP0 coming when the function driver > first bind() to the device, which happens after dwc2 probe. > So, in my opinion this patch is safe. okay fine > > Grigor >> Stefan >> -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/16/2018 2:16 PM, Grigor Tovmasyan wrote: > In dwc2_gadget_init() we allocate EP0 request via > dwc2_hsotg_ep_alloc_request(). After that there are > usb_add_gadget_udc() call and if it failed, then > ctrl_req will not be freed and will cause memory leak. > > Reordered function calls in gadget_init: moved up usb_add_gadget_udc() > before dwc2_hsotg_ep_alloc_request(). > > Tested using kmemleak. > > Cc: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Grigor Tovmasyan <tovmasya@synopsys.com> > --- Acked-by: Minas Harutyunyan <hminas@synopsys.com> > drivers/usb/dwc2/gadget.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 6c32bf26e48e..24000bda5c20 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -4679,6 +4679,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) > INIT_LIST_HEAD(&hsotg->gadget.ep_list); > hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep; > > + ret = usb_add_gadget_udc(dev, &hsotg->gadget); > + if (ret) > + return ret; > + > /* allocate EP0 request */ > > hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep, > @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) > epnum, 0); > } > > - ret = usb_add_gadget_udc(dev, &hsotg->gadget); > - if (ret) > - return ret; > - > dwc2_hsotg_dump(hsotg); > > return 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi All, I've just noticed this patch has been applied to linux-next. It breaks DWC2 driver initialization on Samsung Exynos-based boards I've use for kernel daily tests. Here is an example of the call stack: Unable to handle kernel NULL pointer dereference at virtual address 0000000c pgd = (ptrval) [0000000c] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 25 Comm: kworker/0:1 Not tainted 4.17.0-rc5-next-20180517 #782 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) Workqueue: events deferred_probe_work_func PC is at usb_ep_alloc_request+0x1c/0x200 LR is at composite_dev_prepare+0x20/0xec pc : [<c06976a8>] lr : [<c0694744>] psr: 60000153 sp : d95f1d10 ip : 00000000 fp : 00000002 r10: c1072d58 r9 : d82443d0 r8 : 00000000 r7 : c1074180 r6 : c10ad460 r5 : c100746c r4 : d8253980 r3 : 00000000 r2 : d82539b8 r1 : 014000c0 r0 : d82443d0 Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 4000406a DAC: 00000051 Process kworker/0:1 (pid: 25, stack limit = 0x(ptrval)) Stack: (0xd95f1d10 to 0xd95f2000) ... [<c06976a8>] (usb_ep_alloc_request) from [<c0694744>] (composite_dev_prepare+0x20/0xec) [<c0694744>] (composite_dev_prepare) from [<c0694b4c>] (composite_bind+0x7c/0x1c8) [<c0694b4c>] (composite_bind) from [<c069a8e0>] (udc_bind_to_driver+0x74/0x134) [<c069a8e0>] (udc_bind_to_driver) from [<c069aa14>] (check_pending_gadget_drivers+0x74/0xac) [<c069aa14>] (check_pending_gadget_drivers) from [<c069abdc>] (usb_add_gadget_udc_release+0x190/0x1ec) [<c069abdc>] (usb_add_gadget_udc_release) from [<c0655a84>] (dwc2_gadget_init+0x274/0x464) [<c0655a84>] (dwc2_gadget_init) from [<c0642fb0>] (dwc2_driver_probe+0x488/0x50c) [<c0642fb0>] (dwc2_driver_probe) from [<c057a1e4>] (platform_drv_probe+0x48/0x9c) [<c057a1e4>] (platform_drv_probe) from [<c0577d54>] (driver_probe_device+0x2dc/0x4ac) [<c0577d54>] (driver_probe_device) from [<c0575d54>] (bus_for_each_drv+0x74/0xb8) [<c0575d54>] (bus_for_each_drv) from [<c057792c>] (__device_attach+0xd4/0x168) [<c057792c>] (__device_attach) from [<c0576bf8>] (bus_probe_device+0x88/0x90) [<c0576bf8>] (bus_probe_device) from [<c05771b0>] (deferred_probe_work_func+0x60/0x180) [<c05771b0>] (deferred_probe_work_func) from ad+0x28c/0x58c) [<c0147f2c>] (worker_thread) from [<c014dc5c>] (kthread+0x138/0x168) [<c014dc5c>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) A proper fix for the potential memory leak is to add free to error path instead of reordering usb_add_gadget_udc() call. The issue happens if DWC2 driver is initialized from deferred probe (in such case the gadget driver is already registered). Felipe: please drop or revert this patch in your -next branch. Best regards Marek Szyprowski, PhD Samsung R&D Institute Poland On 2018-04-16 12:16, Grigor Tovmasyan wrote: > In dwc2_gadget_init() we allocate EP0 request via > dwc2_hsotg_ep_alloc_request(). After that there are > usb_add_gadget_udc() call and if it failed, then > ctrl_req will not be freed and will cause memory leak. > > Reordered function calls in gadget_init: moved up usb_add_gadget_udc() > before dwc2_hsotg_ep_alloc_request(). > > Tested using kmemleak. > > Cc: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Grigor Tovmasyan <tovmasya@synopsys.com> > Acked-by: Minas Harutyunyan <hminas@synopsys.com> > --- > drivers/usb/dwc2/gadget.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 6c32bf26e48e..24000bda5c20 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -4679,6 +4679,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) > INIT_LIST_HEAD(&hsotg->gadget.ep_list); > hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep; > > + ret = usb_add_gadget_udc(dev, &hsotg->gadget); > + if (ret) > + return ret; > + > /* allocate EP0 request */ > > hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep, > @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) > epnum, 0); > } > > - ret = usb_add_gadget_udc(dev, &hsotg->gadget); > - if (ret) > - return ret; > - > dwc2_hsotg_dump(hsotg); > > return 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Felipe, Please drop this patch from your next branch. I will provide another fix based on Marek's suggestion. Thanks, Grigor. On 5/17/2018 4:18 PM, Marek Szyprowski wrote: > Hi All, > > I've just noticed this patch has been applied to linux-next. > > It breaks DWC2 driver initialization on Samsung Exynos-based boards > I've use for kernel daily tests. > > Here is an example of the call stack: > > Unable to handle kernel NULL pointer dereference at virtual address 0000000c > pgd = (ptrval) > [0000000c] *pgd=00000000 > Internal error: Oops: 5 [#1] PREEMPT SMP ARM > Modules linked in: > CPU: 0 PID: 25 Comm: kworker/0:1 Not tainted 4.17.0-rc5-next-20180517 #782 > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) > Workqueue: events deferred_probe_work_func > PC is at usb_ep_alloc_request+0x1c/0x200 > LR is at composite_dev_prepare+0x20/0xec > pc : [<c06976a8>] lr : [<c0694744>] psr: 60000153 > sp : d95f1d10 ip : 00000000 fp : 00000002 > r10: c1072d58 r9 : d82443d0 r8 : 00000000 > r7 : c1074180 r6 : c10ad460 r5 : c100746c r4 : d8253980 > r3 : 00000000 r2 : d82539b8 r1 : 014000c0 r0 : d82443d0 > Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: 4000406a DAC: 00000051 > Process kworker/0:1 (pid: 25, stack limit = 0x(ptrval)) > Stack: (0xd95f1d10 to 0xd95f2000) > ... > [<c06976a8>] (usb_ep_alloc_request) from [<c0694744>] > (composite_dev_prepare+0x20/0xec) > [<c0694744>] (composite_dev_prepare) from [<c0694b4c>] > (composite_bind+0x7c/0x1c8) > [<c0694b4c>] (composite_bind) from [<c069a8e0>] > (udc_bind_to_driver+0x74/0x134) > [<c069a8e0>] (udc_bind_to_driver) from [<c069aa14>] > (check_pending_gadget_drivers+0x74/0xac) > [<c069aa14>] (check_pending_gadget_drivers) from [<c069abdc>] > (usb_add_gadget_udc_release+0x190/0x1ec) > [<c069abdc>] (usb_add_gadget_udc_release) from [<c0655a84>] > (dwc2_gadget_init+0x274/0x464) > [<c0655a84>] (dwc2_gadget_init) from [<c0642fb0>] > (dwc2_driver_probe+0x488/0x50c) > [<c0642fb0>] (dwc2_driver_probe) from [<c057a1e4>] > (platform_drv_probe+0x48/0x9c) > [<c057a1e4>] (platform_drv_probe) from [<c0577d54>] > (driver_probe_device+0x2dc/0x4ac) > [<c0577d54>] (driver_probe_device) from [<c0575d54>] > (bus_for_each_drv+0x74/0xb8) > [<c0575d54>] (bus_for_each_drv) from [<c057792c>] > (__device_attach+0xd4/0x168) > [<c057792c>] (__device_attach) from [<c0576bf8>] > (bus_probe_device+0x88/0x90) > [<c0576bf8>] (bus_probe_device) from [<c05771b0>] > (deferred_probe_work_func+0x60/0x180) > [<c05771b0>] (deferred_probe_work_func) from ad+0x28c/0x58c) > [<c0147f2c>] (worker_thread) from [<c014dc5c>] (kthread+0x138/0x168) > [<c014dc5c>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > > A proper fix for the potential memory leak is to add free to error path > instead of reordering usb_add_gadget_udc() call. The issue happens if DWC2 > driver is initialized from deferred probe (in such case the gadget driver > is already registered). > > Felipe: please drop or revert this patch in your -next branch. > > > Best regards > Marek Szyprowski, PhD > Samsung R&D Institute Poland > > On 2018-04-16 12:16, Grigor Tovmasyan wrote: >> In dwc2_gadget_init() we allocate EP0 request via >> dwc2_hsotg_ep_alloc_request(). After that there are >> usb_add_gadget_udc() call and if it failed, then >> ctrl_req will not be freed and will cause memory leak. >> >> Reordered function calls in gadget_init: moved up usb_add_gadget_udc() >> before dwc2_hsotg_ep_alloc_request(). >> >> Tested using kmemleak. >> >> Cc: Stefan Wahren <stefan.wahren@i2se.com> >> Signed-off-by: Grigor Tovmasyan <tovmasya@synopsys.com> >> Acked-by: Minas Harutyunyan <hminas@synopsys.com> >> --- >> drivers/usb/dwc2/gadget.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 6c32bf26e48e..24000bda5c20 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -4679,6 +4679,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) >> INIT_LIST_HEAD(&hsotg->gadget.ep_list); >> hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep; >> >> + ret = usb_add_gadget_udc(dev, &hsotg->gadget); >> + if (ret) >> + return ret; >> + >> /* allocate EP0 request */ >> >> hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep, >> @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) >> epnum, 0); >> } >> >> - ret = usb_add_gadget_udc(dev, &hsotg->gadget); >> - if (ret) >> - return ret; >> - >> dwc2_hsotg_dump(hsotg); >> >> return 0; >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Grigor Tovmasyan <Grigor.Tovmasyan@synopsys.com> writes: > Hi Felipe, > > Please drop this patch from your next branch. > I will provide another fix based on Marek's suggestion. this time I'll rebase my 'next' branch. Next time, make sure this doesn't happen anymore as I like my 'next' to be immutable.
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 6c32bf26e48e..24000bda5c20 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -4679,6 +4679,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) INIT_LIST_HEAD(&hsotg->gadget.ep_list); hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep; + ret = usb_add_gadget_udc(dev, &hsotg->gadget); + if (ret) + return ret; + /* allocate EP0 request */ hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep, @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) epnum, 0); } - ret = usb_add_gadget_udc(dev, &hsotg->gadget); - if (ret) - return ret; - dwc2_hsotg_dump(hsotg); return 0;
In dwc2_gadget_init() we allocate EP0 request via dwc2_hsotg_ep_alloc_request(). After that there are usb_add_gadget_udc() call and if it failed, then ctrl_req will not be freed and will cause memory leak. Reordered function calls in gadget_init: moved up usb_add_gadget_udc() before dwc2_hsotg_ep_alloc_request(). Tested using kmemleak. Cc: Stefan Wahren <stefan.wahren@i2se.com> Signed-off-by: Grigor Tovmasyan <tovmasya@synopsys.com> --- drivers/usb/dwc2/gadget.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)