Hi, On 26 May 2016 at 15:48, Felipe Balbi <balbi@xxxxxxxxxx> wrote: > > Hi, > > Baolin Wang <baolin.wang@xxxxxxxxxx> writes: >> Hi Felipe, >> >> On 26 May 2016 at 14:22, Felipe Balbi <balbi@xxxxxxxxxx> wrote: >>> >>> Hi, >>> >>> Baolin Wang <baolin.wang@xxxxxxxxxx> writes: >>>> When handling the endpoint interrupt handler, it maybe disable the endpoint >>>> from another core user to set the USB endpoint descriptor pointor to be NULL >>>> while issuing usb_gadget_giveback_request() function to release lock. So it >>>> will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() APIs with >>>> one NULL USB endpoint descriptor. >>> >>> too complex, Baolin :-) Can you see if this helps: >>> >>> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=88bf752cfb55e57a78e27c931c9fef63240c739a >>> >>> The only situation when that can happen is while we drop our lock on >>> dwc3_gadget_giveback(). >> >> OK, But line 1974 and line 2025 as below may be at risk? So I think >> can we have a common place to solve the problem in case >> usb_endpoint_xfer_xxx() APIs are issued at this risk? What do you >> think? Thanks. >> >> 1956 static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep, >> 1957 const struct dwc3_event_depevt *event, int status) >> 1958 { >> 1959 struct dwc3_request *req; >> 1960 struct dwc3_trb *trb; >> 1961 unsigned int slot; >> 1962 unsigned int i; >> 1963 int ret; >> 1964 >> 1965 do { >> 1966 req = next_request(&dep->req_queued); >> 1967 if (WARN_ON_ONCE(!req)) >> 1968 return 1; >> 1969 >> 1970 i = 0; >> 1971 do { >> 1972 slot = req->start_slot + i; >> 1973 if ((slot == DWC3_TRB_NUM - 1) && >> 1974 usb_endpoint_xfer_isoc(dep->endpoint.desc)) > > this is executed still with locks held. Yeah, that's right. > >> 1975 slot++; >> 1976 slot %= DWC3_TRB_NUM; >> 1977 trb = &dep->trb_pool[slot]; >> 1978 >> 1979 ret = __dwc3_cleanup_done_trbs(dwc, dep, req, trb, >> 1980 event, status); >> 1981 if (ret) >> 1982 break; >> 1983 } while (++i < req->request.num_mapped_sgs); >> 1984 >> 1985 dwc3_gadget_giveback(dep, req, status); > > the problem can only show up after this call > >> 1986 >> 1987 if (ret) >> 1988 break; >> 1989 } while (1); >> ....... >> >> 2011 static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc, >> 2012 struct dwc3_ep *dep, const struct dwc3_event_depevt *event) >> 2013 { >> 2014 unsigned status = 0; >> 2015 int clean_busy; >> 2016 u32 is_xfer_complete; >> 2017 >> 2018 is_xfer_complete = (event->endpoint_event == >> DWC3_DEPEVT_XFERCOMPLETE); >> 2019 >> 2020 if (event->status & DEPEVT_STATUS_BUSERR) >> 2021 status = -ECONNRESET; >> 2022 >> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status); >> 2024 if (clean_busy && (is_xfer_complete || >> 2025 > > note the patch I linked you. This is where I added a bail out if the > descriptor is NULL. Here's the patch for reference: > > commit 4d100e870616ccd30cf29abf21d437ec746e1f54 > Author: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> > Date: Wed May 18 12:37:21 2016 +0300 > > usb: dwc3: gadget: fix for possible endpoint disable race > > when we call dwc3_gadget_giveback(), we end up > releasing our controller's lock. Another thread > could get scheduled and disable the endpoint, > subsequently setting dep->endpoint.desc to NULL. > > In that case, we would end up dereferencing a NULL > pointer which would result in a Kernel Oops. Let's > avoid the problem by simply returning early if we > have a NULL descriptor. > > Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index f31a59cd5162..3d0573c74b13 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2019,6 +2019,14 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep, > break; > } while (1); > > + /* > + * Our endpoint might get disabled by another thread during > + * dwc3_gadget_giveback(). If that happens, we're just gonna return 1 > + * early on so DWC3_EP_BUSY flag gets cleared > + */ > + if (!dep->endpoint.desc) > + return 1; > + > if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && > list_empty(&dep->started_list)) { > if (list_empty(&dep->pending_list)) { > @@ -2085,6 +2093,14 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc, > dwc->u1u2 = 0; > } > > + /* > + * Our endpoint might get disabled by another thread during > + * dwc3_gadget_giveback(). If that happens, we're just gonna return 1 > + * early on so DWC3_EP_BUSY flag gets cleared > + */ > + if (!dep->endpoint.desc) > + return; > + > if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) { > int ret; > > Also note that the usb_endpoint_xfer_isoc() call on line 2067 of > gadget.c (as in my testing/next from today) won't even get executed, so > we're safe there. Never will be executed? then we can remove the usb_endpoint_xfer_isoc() (line 2025) at risk? 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status); 2024 if (clean_busy && (is_xfer_complete || 2025 usb_endpoint_xfer_isoc(dep->endpoint.desc))) 2026 dep->flags &= ~DWC3_EP_BUSY; > > -- > balbi -- Baolin.wang Best Regards -- 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
