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 |
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 |
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 |
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)) > 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 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 |
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 |
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; still have a problem. I'll fix it up in that commit I pointed you to. -- balbi |
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 |
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. https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=983b84268656ff2686253b05097d28003bbec52f -- balbi |
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 |
Free forum by Nabble | Edit this page |