| Message ID | 20190111060212.7390-1-mgautam@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: gadget: Fail request submission if it was already queued | expand |
Hi, Manu Gautam <mgautam@codeaurora.org> writes: > If a function driver tries to re-submit an already queued request, > it can results in corruption of pending/started request lists. > Catch such conditions and fail the request submission to DCD. > > Signed-off-by: Manu Gautam <mgautam@codeaurora.org> > --- > drivers/usb/dwc3/gadget.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 679c12e14522..51716c6b286a 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1290,6 +1290,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > &req->request, req->dep->name)) > return -EINVAL; > > + if (req->request.status == -EINPROGRESS) { this test is really not enough. What if gadget driver set status to EINPROGRESS before submission? A better check would involve making sure req isn't part of dep->pending_list or dep->started_list or dep->cancelled_list. It's clear that this won't work very well as the amount of requests grow. Anyway, which gadget driver did this? Why is it only affecting dwc3?
Hi, On 1/11/2019 1:13 PM, Felipe Balbi wrote: > Hi, > > Manu Gautam <mgautam@codeaurora.org> writes: >> If a function driver tries to re-submit an already queued request, >> it can results in corruption of pending/started request lists. >> Catch such conditions and fail the request submission to DCD. >> >> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >> --- >> drivers/usb/dwc3/gadget.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 679c12e14522..51716c6b286a 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1290,6 +1290,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >> &req->request, req->dep->name)) >> return -EINVAL; >> >> + if (req->request.status == -EINPROGRESS) { > this test is really not enough. What if gadget driver set status to > EINPROGRESS before submission? A better check would involve making sure > req isn't part of dep->pending_list or dep->started_list or > dep->cancelled_list. It's clear that this won't work very well as the > amount of requests grow. Thanks for quick review. 'request.status' check can be replaced: +if (!list_empty(&req->list) { And replace list_del with list_del_init from dwc3_gadget_giveback() > > Anyway, which gadget driver did this? Why is it only affecting dwc3? Function driver is not in upstream (f_mtp.c). >
Hi, Manu Gautam <mgautam@codeaurora.org> writes: >> Manu Gautam <mgautam@codeaurora.org> writes: >>> If a function driver tries to re-submit an already queued request, >>> it can results in corruption of pending/started request lists. >>> Catch such conditions and fail the request submission to DCD. >>> >>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >>> --- >>> drivers/usb/dwc3/gadget.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 679c12e14522..51716c6b286a 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1290,6 +1290,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >>> &req->request, req->dep->name)) >>> return -EINVAL; >>> >>> + if (req->request.status == -EINPROGRESS) { >> this test is really not enough. What if gadget driver set status to >> EINPROGRESS before submission? A better check would involve making sure >> req isn't part of dep->pending_list or dep->started_list or >> dep->cancelled_list. It's clear that this won't work very well as the >> amount of requests grow. > > Thanks for quick review. > 'request.status' check can be replaced: > +if (!list_empty(&req->list) { > > And replace list_del with list_del_init from dwc3_gadget_giveback() I would rather avoid this. We could start tracking our own internal dwc3_request status. Something along the lines of: diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index df876418cb78..5c3ee741541f 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -863,6 +863,7 @@ struct dwc3_hwparams { * @num_pending_sgs: counter to pending sgs * @num_queued_sgs: counter to the number of sgs which already got queued * @remaining: amount of data remaining + * @status: internal dwc3 request status tracking * @epnum: endpoint number to which this request refers * @trb: pointer to struct dwc3_trb * @trb_dma: DMA address of @trb @@ -883,6 +884,14 @@ struct dwc3_request { unsigned num_pending_sgs; unsigned int num_queued_sgs; unsigned remaining; + + unsigned int status; +#define DWC3_REQUEST_STATUS_QUEUED 0 +#define DWC3_REQUEST_STATUS_STARTED 1 +#define DWC3_REQUEST_STATUS_CANCELLED 2 +#define DWC3_REQUEST_STATUS_COMPLETED 3 +#define DWC3_REQUEST_STATUS_UNKNOWN -1 + u8 epnum; struct dwc3_trb *trb; dma_addr_t trb_dma; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 07bd31bb2f8a..74db274786bc 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -208,6 +208,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req, struct dwc3 *dwc = dep->dwc; dwc3_gadget_del_and_unmap_request(dep, req, status); + req->status = DWC3_REQUEST_STATUS_COMPLETED; spin_unlock(&dwc->lock); usb_gadget_giveback_request(&dep->endpoint, &req->request); @@ -846,6 +847,7 @@ static struct usb_request *dwc3_gadget_ep_alloc_request(struct usb_ep *ep, req->direction = dep->direction; req->epnum = dep->number; req->dep = dep; + req->status = DWC3_REQUEST_STATUS_UNKNOWN; trace_dwc3_alloc_request(req); @@ -1434,6 +1436,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) &req->request, req->dep->name)) return -EINVAL; + if (WARN(req->status < DWC3_REQUEST_STATUS_COMPLETED, + "%s: request %pK already in flight\n", + dep->name, &req->request)) + return -EINVAL; + pm_runtime_get(dwc->dev); req->request.actual = 0; @@ -1442,6 +1449,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) trace_dwc3_ep_queue(req); list_add_tail(&req->list, &dep->pending_list); + req->status = DWC3_REQUEST_STATUS_QUEUED; /* * NOTICE: Isochronous endpoints should NEVER be prestarted. We must diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h index 023a473648eb..6aebe8c0eae1 100644 --- a/drivers/usb/dwc3/gadget.h +++ b/drivers/usb/dwc3/gadget.h @@ -76,6 +76,7 @@ static inline void dwc3_gadget_move_started_request(struct dwc3_request *req) struct dwc3_ep *dep = req->dep; req->started = true; + req->status = DWC3_REQUEST_STATUS_STARTED; list_move_tail(&req->list, &dep->started_list); } @@ -91,6 +92,7 @@ static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request *req) struct dwc3_ep *dep = req->dep; req->started = false; + req->status = DWC3_REQUEST_STATUS_CANCELLED; list_move_tail(&req->list, &dep->cancelled_list); } With this, we can remove some of the other request flags, such as "started". >> Anyway, which gadget driver did this? Why is it only affecting dwc3? > > Function driver is not in upstream (f_mtp.c). So this could happen with any UDC, right? Why is f_mtp.c queueing the same request twice?
Hi, On 1/11/2019 2:51 PM, Felipe Balbi wrote: > Hi, > > Manu Gautam <mgautam@codeaurora.org> writes: >>> Manu Gautam <mgautam@codeaurora.org> writes: >>>> If a function driver tries to re-submit an already queued request, >>>> it can results in corruption of pending/started request lists. >>>> Catch such conditions and fail the request submission to DCD. >>>> >>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >>>> --- >>>> drivers/usb/dwc3/gadget.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index 679c12e14522..51716c6b286a 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -1290,6 +1290,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >>>> &req->request, req->dep->name)) >>>> return -EINVAL; >>>> >>>> + if (req->request.status == -EINPROGRESS) { >>> this test is really not enough. What if gadget driver set status to >>> EINPROGRESS before submission? A better check would involve making sure >>> req isn't part of dep->pending_list or dep->started_list or >>> dep->cancelled_list. It's clear that this won't work very well as the >>> amount of requests grow. >> Thanks for quick review. >> 'request.status' check can be replaced: >> +if (!list_empty(&req->list) { >> >> And replace list_del with list_del_init from dwc3_gadget_giveback() > I would rather avoid this. We could start tracking our own internal > dwc3_request status. Something along the lines of: Thanks for this quick patch. [snip] > > With this, we can remove some of the other request flags, such as "started". > >>> Anyway, which gadget driver did this? Why is it only affecting dwc3? >> Function driver is not in upstream (f_mtp.c). > So this could happen with any UDC, right? Why is f_mtp.c queueing the > same request twice? Looks like chipidea UDC already has such a check. It is not yet clear that why f_mtp queued same request twice. However, having a safeguard check in UDC should be helpful. >
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 679c12e14522..51716c6b286a 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1290,6 +1290,12 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) &req->request, req->dep->name)) return -EINVAL; + if (req->request.status == -EINPROGRESS) { + dev_err(dwc->dev, "%s: %pK request already in queue\n", + dep->name, req); + return -EBUSY; + } + pm_runtime_get(dwc->dev); req->request.actual = 0;
If a function driver tries to re-submit an already queued request, it can results in corruption of pending/started request lists. Catch such conditions and fail the request submission to DCD. Signed-off-by: Manu Gautam <mgautam@codeaurora.org> --- drivers/usb/dwc3/gadget.c | 6 ++++++ 1 file changed, 6 insertions(+)