From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753284AbcEZHwL (ORCPT ); Thu, 26 May 2016 03:52:11 -0400 Received: from mga02.intel.com ([134.134.136.20]:3665 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753205AbcEZHwJ (ORCPT ); Thu, 26 May 2016 03:52:09 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,367,1459839600"; d="asc'?scan'208";a="110783959" From: Felipe Balbi To: Baolin Wang Cc: Greg KH , Mark Brown , USB , LKML Subject: Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type In-Reply-To: References: <7eb2c730f54f117438b6111bb63799a5d8c7249c.1464238593.git.baolin.wang@linaro.org> <87eg8pqqwx.fsf@linux.intel.com> User-Agent: Notmuch/0.22+11~g124a67e (http://notmuchmail.org) Emacs/25.0.93.2 (x86_64-pc-linux-gnu) Date: Thu, 26 May 2016 10:48:54 +0300 Message-ID: <8760u1qmxl.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Baolin Wang writes: > Hi Felipe, > > On 26 May 2016 at 14:22, Felipe Balbi wrote: >> >> Hi, >> >> Baolin Wang writes: >>> When handling the endpoint interrupt handler, it maybe disable the endp= oint >>> from another core user to set the USB endpoint descriptor pointor to be= NULL >>> while issuing usb_gadget_giveback_request() function to release lock. S= o it >>> will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() A= PIs 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=3D= 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 =3D next_request(&dep->req_queued); > 1967 if (WARN_ON_ONCE(!req)) > 1968 return 1; > 1969 > 1970 i =3D 0; > 1971 do { > 1972 slot =3D req->start_slot + i; > 1973 if ((slot =3D=3D DWC3_TRB_NUM - 1) && > 1974 usb_endpoint_xfer_isoc(dep->endpoint= .desc)) this is executed still with locks held. > 1975 slot++; > 1976 slot %=3D DWC3_TRB_NUM; > 1977 trb =3D &dep->trb_pool[slot]; > 1978 > 1979 ret =3D __dwc3_cleanup_done_trbs(dwc, dep, r= eq, 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 =3D 0; > 2015 int clean_busy; > 2016 u32 is_xfer_complete; > 2017 > 2018 is_xfer_complete =3D (event->endpoint_event =3D=3D > DWC3_DEPEVT_XFERCOMPLETE); > 2019 > 2020 if (event->status & DEPEVT_STATUS_BUSERR) > 2021 status =3D -ECONNRESET; > 2022 > 2023 clean_busy =3D dwc3_cleanup_done_reqs(dwc, dep, event, statu= s); > 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 Date: Wed May 18 12:37:21 2016 +0300 usb: dwc3: gadget: fix for possible endpoint disable race =20=20=20=20 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. =20=20=20=20 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. =20=20=20=20 Signed-off-by: Felipe Balbi diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index f31a59cd5162..3d0573c74b13 100644 =2D-- 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); =20 + /* + * 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 d= wc3 *dwc, dwc->u1u2 =3D 0; } =20 + /* + * 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. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXRqpnAAoJEIaOsuA1yqREM4YP/jMWeFgfFUR6byg2LAFBXtUs KzeztwftjYbZEME/03U2LqdYbT72MzMKMPV4ts2NZMPgwBCTKOsDpyw63j7xH6Jf bCaVu0+q9ksJHXc6P7Ydnc/SJMjlpb/81gNuMdbR/f3jXDOx684kGn4h6en1qd+c Se95Ti3DredbRYwFVMOiHfDBeIZH3Bwpob2gEpexoo5guI+S2xdM8CuSvnJ2se3r X9QwYx6EfJRoUYcNn3N6WJ609PJIXyn+gQdWwzC6nlGmlQmlw17sy2b/1EiOT6uS XWFDFdbcwWok3HDV1kQ320mgn8gnfouNgeeYvI/IkNS+NS9P0/H7EPyEzQPWQ3jw 6jLFmLavqPvckZTdT1oYRxadP74sObO9t0OwEnlCU//s8J46TaPGULWpICIy6H24 L2lUahPgbnTDY2pHzFIlT6qQ5xCeTM13fvOnKdPmzk5Irlwbj9IriONRuA3xL5ek Schr/puQkooZrtXyBppqCXJlBtvcsLnSHzfHllMtOQ9LEOPZv1FpRIi35xOyy/NS QGlqLUl3S+ArsD7y80g7LEGADWk63p4HHqDlqaA2E3/K59ZtFE4LxkA7v8JuOPgl VySJW/vBCfLG1Q6UTW7y1xW4bmaoB86t20E/jeW46i+/F7mC/APUs8/lPSM5Btp7 Hho9JXAqzUzNC2nVddlJ =x4ty -----END PGP SIGNATURE----- --=-=-=--