Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type

[Felipe Balbi]

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.

> 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.

-- 
balbi

Attachment: signature.asc
Description: PGP signature