Hi, Baolin Wang <[email protected]> writes: > Hi Felipe, > > On 26 May 2016 at 14:22, Felipe Balbi <[email protected]> wrote: >> >> Hi, >> >> Baolin Wang <[email protected]> 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.
> 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 <[email protected]>
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 <[email protected]>
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.
--
balbi
signature.asc
Description: PGP signature
