On 12/11/2014 08:34 PM, Paul Zimmerman wrote: >> From: Robert Baldyga [mailto:r.baldyga@xxxxxxxxxxx] >> Sent: Tuesday, December 09, 2014 5:42 AM >> >> This makes us sure that all requests are completed before we unbind >> gadget. There are assumptions in gadget API that all requests have to >> be completed and leak of complete can break some usb function drivers. >> >> For example unbind of ECM function can cause NULL pointer dereference: >> >> [ 26.396595] configfs-gadget gadget: unbind function >> 'cdc_ethernet'/e79c4c00 >> [ 26.414999] Unable to handle kernel NULL pointer dereference at >> virtual address 00000000 >> (...) >> [ 26.452223] PC is at ecm_unbind+0x6c/0x9c >> [ 26.456209] LR is at ecm_unbind+0x68/0x9c >> (...) >> [ 26.603696] [<c033fdb4>] (ecm_unbind) from [<c033661c>] >> (purge_configs_funcs+0x94/0xd8) >> [ 26.611674] [<c033661c>] (purge_configs_funcs) from [<c0336674>] >> (configfs_composite_unbind+0x14/0x34) >> [ 26.620961] [<c0336674>] (configfs_composite_unbind) from >> [<c0337124>] (usb_gadget_remove_driver+0x68/0x9c) >> [ 26.630683] [<c0337124>] (usb_gadget_remove_driver) from [<c03376c8>] >> (usb_gadget_unregister_driver+0x64/0x94) >> [ 26.640664] [<c03376c8>] (usb_gadget_unregister_driver) from >> [<c0336be8>] (unregister_gadget+0x20/0x3c) >> [ 26.650038] [<c0336be8>] (unregister_gadget) from [<c0336c84>] >> (gadget_dev_desc_UDC_store+0x80/0xb8) >> [ 26.659152] [<c0336c84>] (gadget_dev_desc_UDC_store) from >> [<c0335120>] (gadget_info_attr_store+0x1c/0x28) >> [ 26.668703] [<c0335120>] (gadget_info_attr_store) from [<c012135c>] >> (configfs_write_file+0xe8/0x148) >> [ 26.677818] [<c012135c>] (configfs_write_file) from [<c00c8dd4>] >> (vfs_write+0xb0/0x1a0) >> [ 26.685801] [<c00c8dd4>] (vfs_write) from [<c00c91b8>] >> (SyS_write+0x44/0x84) >> [ 26.692834] [<c00c91b8>] (SyS_write) from [<c000e560>] >> (ret_fast_syscall+0x0/0x30) >> [ 26.700381] Code: e30409f8 e34c0069 eb07b88d e59430a8 (e5930000) >> [ 26.706485] ---[ end trace f62a082b323838a2 ]--- >> >> It's because in some cases request is still running on endpoint during >> unbind and kill_all_requests() called from s3c_hsotg_udc_stop() function >> doesn't cause call of complete() of request. Missing complete() call >> causes ecm->notify_req equals NULL in ecm_unbind() function, and this >> is reason of this bug. >> >> Similar breaks can be observed in another usb function drivers. >> >> This patch fixes this bug forcing usb request completion in when >> s3c_hsotg_ep_disable() is called from s3c_hsotg_udc_stop(). >> >> Signed-off-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx> >> --- >> drivers/usb/dwc2/gadget.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 8b5c079..cb4c925 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -2590,7 +2590,7 @@ error: >> * s3c_hsotg_ep_disable - disable given endpoint >> * @ep: The endpoint to disable. >> */ >> -static int s3c_hsotg_ep_disable(struct usb_ep *ep) >> +static int s3c_hsotg_ep_disable_force(struct usb_ep *ep, bool force) >> { >> struct s3c_hsotg_ep *hs_ep = our_ep(ep); >> struct s3c_hsotg *hsotg = hs_ep->parent; >> @@ -2611,7 +2611,7 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep) >> >> spin_lock_irqsave(&hsotg->lock, flags); >> /* terminate all requests with shutdown */ >> - kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, false); >> + kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, force); >> >> hsotg->fifo_map &= ~(1<<hs_ep->fifo_index); >> hs_ep->fifo_index = 0; >> @@ -2632,6 +2632,10 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep) >> return 0; >> } >> >> +static int s3c_hsotg_ep_disable(struct usb_ep *ep) >> +{ >> + return s3c_hsotg_ep_disable_force(ep, false); >> +} >> /** >> * on_list - check request is on the given endpoint >> * @ep: The endpoint to check. >> @@ -2933,7 +2937,7 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget, >> >> /* all endpoints should be shutdown */ >> for (ep = 1; ep < hsotg->num_of_eps; ep++) >> - s3c_hsotg_ep_disable(&hsotg->eps[ep].ep); >> + s3c_hsotg_ep_disable_force(&hsotg->eps[ep].ep, true); >> >> spin_lock_irqsave(&hsotg->lock, flags); >> > > Acked-by: Paul Zimmerman <paulz@xxxxxxxxxxxx> > One more thing which I have forgot to write: This patch makes sense only with my patch to udc-core [1] which modifies order of calls in usb_gadget_remove_driver() function. Hence this patch shouldn't be applied before [1] will be accepted. [1] https://lkml.org/lkml/2014/12/12/360 Best regards, Robert Baldyga -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
