Quantcast

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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

Thus this patch introduces safety dwc3_endpoint_xfer_xxx() APIs to check
endpoint type with checking if the endpoint flag 'DWC3_EP_ENABLED' is set or
not.

[ 4007.812527] c1 init(1) call enable_store(0)
[ 4007.812633] c0 dwc3 20500000.dwc3: ep1out disabled while handling ep event
[ 4007.812657] c0 Unable to handle kernel NULL pointer dereference at virtual address 00000003
[ 4007.812669] c0 pgd = ffffffc0260a8000
[ 4007.812681] c0 [00000003] *pgd=00000000b4ab4003, *pud=00000000b4ab4003, *pmd=0000000000000000
[ 4007.812709] c0 Internal error: Oops: 96000006 [#1] PREEMPT SMP
[ 4007.818441] c0 Modules linked in: bcmdhd
[ 4007.821854] c3 warning saudio_recv cache is empty!
[ 4007.821860] c3 aud_recv_cmd  ENODATA
[ 4007.821869] c3 aud_send_cmd in,cmd =11 id:10 ret-value:0
[ 4007.821876] c3 saudio_send: dst=1, channel=0, timeout=-1
[ 4007.840767] c0  mali_kbase(O)
[ 4007.843707] c0
[ 4007.843889] c0 CPU: 0 PID: 8126 Comm: adbd Tainted: G        W  O   3.18.12+ #1
[ 4007.851153] c0 Hardware name: Spreadtrum SP9860g Board (DT)
[ 4007.856693] c0 task: ffffffc02638a400 ti: ffffffc026148000 task.ti: ffffffc026148000
[ 4007.864404] c0 PC is at dwc3_interrupt_bh+0x710/0x112c
[ 4007.869503] c0 LR is at dwc3_interrupt_bh+0x70c/0x112c
[ 4007.874604] c0 pc : [<ffffffc0005bc48c>] lr : [<ffffffc0005bc488>] pstate: 200001c5
[ 4007.882218] c0 sp : ffffffc02614b970
[ 4007.885766] c0 x29: ffffffc02614b970 x28: ffffffc000c75f90
[ 4007.891044] c0 x27: 0000000000000008 x26: 0000000000000000
[ 4007.896323] c0 x25: 0000000000000000 x24: 000000000000030c
[ 4007.901602] c0 x23: ffffffc000ff1000 x22: 0000000000000004
[ 4007.906881] c0 x21: ffffffc03d9fb5d8 x20: ffffffc13e573600
[ 4007.912160] c0 x19: ffffffc13e133020 x18: 0000000000000007
[ 4007.917437] c0 x17: 000000000000000e x16: 0000000000000001
[ 4007.922717] c0 x15: 0000000000000007 x14: 0fffffffffffffff
[ 4007.927996] c0 x13: 0000000000000001 x12: 0101010101010101
[ 4007.933274] c0 x11: 00000000000c4f5d x10: 0000000000000002
[ 4007.938553] c0 x9 : 0000000000000000 x8 : 00000000000c4f5e
[ 4007.943833] c0 x7 : 696c646e61682065 x6 : ffffffc000fa6cdc
[ 4007.949111] c0 x5 : 0000000000000000 x4 : 0000000000000105
[ 4007.954390] c0 x3 : 0000000000000000 x2 : 0000000000000000
[ 4007.959669] c0 x1 : 000000005533250a x0 : 0000000000000000
......
[ 4009.150699] c0 Call trace:
[ 4009.153394] c0 [<ffffffc0005bc48c>] dwc3_interrupt_bh+0x710/0x112c
[ 4009.159532] c0 [<ffffffc0000c0370>] tasklet_action+0xb0/0x188
[ 4009.165243] c0 [<ffffffc0000bf598>] __do_softirq+0x114/0x3b4
[ 4009.170867] c0 [<ffffffc0000bfb9c>] irq_exit+0xa8/0xdc
[ 4009.175975] c0 [<ffffffc00010cc44>] __handle_domain_irq+0x90/0xf8
[ 4009.182030] c0 [<ffffffc00008249c>] gic_handle_irq+0x58/0x1b4
[ 4009.187739] c0 Exception stack(0xffffffc02614bb70 to 0xffffffc02614bc90)
[ 4009.194404] c0 bb60:                                     00000140 00000000 3e133108 ffffffc1
[ 4009.202802] c0 bb80: 2614bcd0 ffffffc0 009cff0c ffffffc0 80000145 00000000 00000018 00000000
[ 4009.211194] c0 bba0: 00000400 00000000 00459900 ffffffc0 00000000 00000000 00d1aa50 ffffffc0
[ 4009.219589] c0 bbc0: 2614bc00 ffffffc0 26148000 ffffffc0 00000001 00000000 3d2ce4e0 ffffffc1
[ 4009.227984] c0 bbe0: bdc00000 00000000 00000001 00000000 2638a980 ffffffc0 2614ba40 ffffffc0
[ 4009.236379] c0 bc00: 00000400 00000000 00000001 00000000 1d37bbb0 ffffffc0 00000013 00000000
[ 4009.244772] c0 bc20: 0000000e 00000000 00000007 00000000 00000001 00000000 0000000e 00000000
[ 4009.253167] c0 bc40: 00000007 00000000 00000140 00000000 3e133108 ffffffc1 3e133108 ffffffc1
[ 4009.261560] c0 bc60: 00000140 00000000 3eb833c0 ffffffc1 00000018 00000000 00000400 00000000
[ 4009.269951] c0 bc80: 1eb29b80 ffffffc1 3acd7c00 ffffffc0
[ 4009.275233] c0 [<ffffffc0000860e8>] el1_irq+0x68/0xdc
[ 4009.280255] c0 [<ffffffc0005b940c>] dwc3_gadget_ep_dequeue+0x180/0x1b8
[ 4009.286741] c0 [<ffffffc0005feb48>] ffs_epfile_io+0x330/0x374
[ 4009.292453] c0 [<ffffffc0005fec3c>] ffs_epfile_read+0x30/0x40
[ 4009.298169] c0 [<ffffffc0001ecb3c>] vfs_read+0xa0/0x1dc
[ 4009.303357] c0 [<ffffffc0001ed644>] SyS_read+0x50/0xb0

Signed-off-by: Baolin Wang <[hidden email]>
---
 drivers/usb/dwc3/gadget.c |   95 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 72 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8e4a1b1..5d095f2 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -36,6 +36,56 @@
 #include "io.h"
 
 /**
+* dwc3_endpoint_xfer_bulk - check if the endpoint has bulk transfer type
+* @ep: endpoint to be checked
+*
+* Returns true if the endpoint is of type bulk, otherwise it returns false.
+*/
+static inline int dwc3_endpoint_xfer_bulk(struct dwc3_ep *ep)
+{
+ return ((ep->flags & DWC3_EP_ENABLED) &&
+ ep->type == USB_ENDPOINT_XFER_BULK);
+}
+
+/**
+* dwc3_endpoint_xfer_control - check if the endpoint has control transfer type
+* @ep: endpoint to be checked
+*
+* Returns true if the endpoint is of type control, otherwise it returns false.
+*/
+static inline int dwc3_endpoint_xfer_control(struct dwc3_ep *ep)
+{
+ return ((ep->flags & DWC3_EP_ENABLED) &&
+ ep->type == USB_ENDPOINT_XFER_CONTROL);
+}
+
+/**
+* dwc3_endpoint_xfer_int - check if the endpoint has interrupt transfer type
+* @ep: endpoint to be checked
+*
+* Returns true if the endpoint is of type interrupt, otherwise it returns
+* false.
+*/
+static inline int dwc3_endpoint_xfer_int(struct dwc3_ep *ep)
+{
+ return ((ep->flags & DWC3_EP_ENABLED) &&
+ ep->type == USB_ENDPOINT_XFER_INT);
+}
+
+/**
+* dwc3_endpoint_xfer_isoc - check if the endpoint has isochronous transfer type
+* @ep: endpoint to be checked
+*
+* Returns true if the endpoint is of type isochronous, otherwise it returns
+* false.
+*/
+static inline int dwc3_endpoint_xfer_isoc(struct dwc3_ep *ep)
+{
+ return ((ep->flags & DWC3_EP_ENABLED) &&
+ ep->type == USB_ENDPOINT_XFER_ISOC);
+}
+
+/**
  * dwc3_gadget_set_test_mode - Enables USB2 Test Modes
  * @dwc: pointer to our context structure
  * @mode: the mode to set (J, K SE0 NAK, Force Enable)
@@ -198,8 +248,8 @@ int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc)
  if (!(dep->flags & DWC3_EP_ENABLED))
  continue;
 
- if (usb_endpoint_xfer_bulk(dep->endpoint.desc)
- || usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ if (dwc3_endpoint_xfer_bulk(dep) ||
+    dwc3_endpoint_xfer_isoc(dep))
  mult = 3;
 
  /*
@@ -248,7 +298,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
  */
  if (((dep->busy_slot & DWC3_TRB_MASK) ==
  DWC3_TRB_NUM- 1) &&
- usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ dwc3_endpoint_xfer_isoc(dep))
  dep->busy_slot++;
  } while(++i < req->request.num_mapped_sgs);
  req->queued = false;
@@ -567,7 +617,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
  reg |= DWC3_DALEPENA_EP(dep->number);
  dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
 
- if (!usb_endpoint_xfer_isoc(desc))
+ if (!dwc3_endpoint_xfer_isoc(dep))
  goto out;
 
  /* Link TRB for ISOC. The HWO bit is never reset */
@@ -795,7 +845,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
  dep->free_slot++;
  /* Skip the LINK-TRB on ISOC */
  if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
- usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ dwc3_endpoint_xfer_isoc(dep))
  dep->free_slot++;
 
  trb->size = DWC3_TRB_SIZE_LENGTH(length);
@@ -829,7 +879,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
  if (!req->request.no_interrupt && !chain)
  trb->ctrl |= DWC3_TRB_CTRL_IOC;
 
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
  trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
  trb->ctrl |= DWC3_TRB_CTRL_CSP;
  } else if (last) {
@@ -839,7 +889,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
  if (chain)
  trb->ctrl |= DWC3_TRB_CTRL_CHN;
 
- if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
+ if (dwc3_endpoint_xfer_bulk(dep) && dep->stream_capable)
  trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(req->request.stream_id);
 
  trb->ctrl |= DWC3_TRB_CTRL_HWO;
@@ -869,7 +919,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)
  trbs_left = (dep->busy_slot - dep->free_slot) & DWC3_TRB_MASK;
 
  /* Can't wrap around on a non-isoc EP since there's no link TRB */
- if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (!dwc3_endpoint_xfer_isoc(dep)) {
  max = DWC3_TRB_NUM - (dep->free_slot & DWC3_TRB_MASK);
  if (trbs_left > max)
  trbs_left = max;
@@ -895,7 +945,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)
  * processed from the first TRB until the last one. Since we
  * don't wrap around we have to start at the beginning.
  */
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
  dep->busy_slot = 1;
  dep->free_slot = 1;
  } else {
@@ -905,7 +955,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)
  }
 
  /* The last TRB is a link TRB, not used for xfer */
- if ((trbs_left <= 1) && usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ if ((trbs_left <= 1) && dwc3_endpoint_xfer_isoc(dep))
  return;
 
  list_for_each_entry_safe(req, n, &dep->request_list, list) {
@@ -1123,8 +1173,8 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
  * This will save one IRQ (XFER_NOT_READY) and possibly make it a
  * little bit faster.
  */
- if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
- !usb_endpoint_xfer_int(dep->endpoint.desc) &&
+ if (!dwc3_endpoint_xfer_isoc(dep) &&
+ !dwc3_endpoint_xfer_int(dep) &&
  !(dep->flags & DWC3_EP_BUSY)) {
  ret = __dwc3_gadget_kick_transfer(dep, 0, true);
  goto out;
@@ -1148,7 +1198,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
  * you can receive xfernotready again and can have
  * notion of current microframe.
  */
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
  if (list_empty(&dep->req_queued)) {
  dwc3_stop_active_transfer(dwc, dep->number, true);
  dep->flags = DWC3_EP_ENABLED;
@@ -1168,7 +1218,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
  *    kick the transfer here after queuing a request, otherwise the
  *    core may not see the modified TRB(s).
  */
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+ if (dwc3_endpoint_xfer_isoc(dep) &&
  (dep->flags & DWC3_EP_BUSY) &&
  !(dep->flags & DWC3_EP_MISSED_ISOC)) {
  WARN_ON_ONCE(!dep->resource_index);
@@ -1304,7 +1354,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
  struct dwc3 *dwc = dep->dwc;
  int ret;
 
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
  dev_err(dwc->dev, "%s is of Isochronous type\n", dep->name);
  return -EINVAL;
  }
@@ -1971,7 +2021,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
  do {
  slot = req->start_slot + i;
  if ((slot == DWC3_TRB_NUM - 1) &&
- usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ dwc3_endpoint_xfer_isoc(dep))
  slot++;
  slot %= DWC3_TRB_NUM;
  trb = &dep->trb_pool[slot];
@@ -1988,7 +2038,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
  break;
  } while (1);
 
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+ if (dwc3_endpoint_xfer_isoc(dep) &&
  list_empty(&dep->req_queued)) {
  if (list_empty(&dep->request_list)) {
  /*
@@ -2021,8 +2071,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
  status = -ECONNRESET;
 
  clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
- if (clean_busy && (is_xfer_complete ||
- usb_endpoint_xfer_isoc(dep->endpoint.desc)))
+ if (clean_busy && (is_xfer_complete || dwc3_endpoint_xfer_isoc(dep)))
  dep->flags &= ~DWC3_EP_BUSY;
 
  /*
@@ -2050,7 +2099,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
  dwc->u1u2 = 0;
  }
 
- if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (!dwc3_endpoint_xfer_isoc(dep)) {
  int ret;
 
  ret = __dwc3_gadget_kick_transfer(dep, 0, is_xfer_complete);
@@ -2079,7 +2128,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
  case DWC3_DEPEVT_XFERCOMPLETE:
  dep->resource_index = 0;
 
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
  dwc3_trace(trace_dwc3_gadget,
  "%s is an Isochronous endpoint\n",
  dep->name);
@@ -2092,7 +2141,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
  dwc3_endpoint_transfer_complete(dwc, dep, event);
  break;
  case DWC3_DEPEVT_XFERNOTREADY:
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
  dwc3_gadget_start_isoc(dwc, dep, event);
  } else {
  int active;
@@ -2115,7 +2164,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 
  break;
  case DWC3_DEPEVT_STREAMEVT:
- if (!usb_endpoint_xfer_bulk(dep->endpoint.desc)) {
+ if (!dwc3_endpoint_xfer_bulk(dep)) {
  dev_err(dwc->dev, "Stream event for non-Bulk %s\n",
  dep->name);
  return;
--
1.7.9.5

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Felipe Balbi-2

Hi,

Baolin Wang <[hidden email]> 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().

--
balbi

signature.asc (834 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Baolin Wang
Hi Felipe,

On 26 May 2016 at 14:22, Felipe Balbi <[hidden email]> wrote:

>
> Hi,
>
> Baolin Wang <[hidden email]> 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))
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);
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
usb_endpoint_xfer_isoc(dep->endpoint.desc)))
2026                 dep->flags &= ~DWC3_EP_BUSY;

>
> --
> balbi



--
Baolin.wang
Best Regards
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Felipe Balbi-2

Hi,

Baolin Wang <[hidden email]> writes:

> Hi Felipe,
>
> On 26 May 2016 at 14:22, Felipe Balbi <[hidden email]> wrote:
>>
>> Hi,
>>
>> Baolin Wang <[hidden email]> 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 <[hidden email]>
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 <[hidden email]>

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 (834 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Baolin Wang
Hi,

On 26 May 2016 at 15:48, Felipe Balbi <[hidden email]> wrote:

>
> Hi,
>
> Baolin Wang <[hidden email]> writes:
>> Hi Felipe,
>>
>> On 26 May 2016 at 14:22, Felipe Balbi <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]>
>
> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Felipe Balbi-2

Hi,

Baolin Wang <[hidden email]> writes:

<trim>

>> 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;
hmm, now that I look at this again, in case of XferInProgress, we could
still have a problem.

I'll fix it up in that commit I pointed you to.

--
balbi

signature.asc (834 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Baolin Wang
On 26 May 2016 at 17:45, Felipe Balbi <[hidden email]> wrote:

>
> Hi,
>
> Baolin Wang <[hidden email]> writes:
>
> <trim>
>
>>> 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;
>
> hmm, now that I look at this again, in case of XferInProgress, we could
> still have a problem.
>
> I'll fix it up in that commit I pointed you to.

Great. Thanks.

>
> --
> balbi



--
Baolin.wang
Best Regards
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Felipe Balbi-2

Hi,

Baolin Wang <[hidden email]> writes:

> On 26 May 2016 at 17:45, Felipe Balbi <[hidden email]> wrote:
>>
>> Hi,
>>
>> Baolin Wang <[hidden email]> writes:
>>
>> <trim>
>>
>>>> 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;
>>
>> hmm, now that I look at this again, in case of XferInProgress, we could
>> still have a problem.
>>
>> I'll fix it up in that commit I pointed you to.
>
> Great. Thanks.
fixed now:

https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=983b84268656ff2686253b05097d28003bbec52f

--
balbi

signature.asc (834 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Baolin Wang
On 26 May 2016 at 18:27, Felipe Balbi <[hidden email]> wrote:

>
> Hi,
>
> Baolin Wang <[hidden email]> writes:
>> On 26 May 2016 at 17:45, Felipe Balbi <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <[hidden email]> writes:
>>>
>>> <trim>
>>>
>>>>> 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;
>>>
>>> hmm, now that I look at this again, in case of XferInProgress, we could
>>> still have a problem.
>>>
>>> I'll fix it up in that commit I pointed you to.
>>
>> Great. Thanks.
>
> fixed now:
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=983b84268656ff2686253b05097d28003bbec52f

OK. I'll test it again with applying your patch. Thanks.

>
> --
> balbi



--
Baolin.wang
Best Regards
Loading...