[2/2] usb: dwc3: gadget: never call ->complete() from ->ep_queue()

[2/2] usb: dwc3: gadget: never call ->complete() from ->ep_queue()

[Felipe Balbi]

This is a requirement which has always existed but, somehow, wasn't
reflected in the documentation and problems weren't found until now
when Tuba Yavuz found a possible deadlock happening between dwc3 and
f_hid. She described the situation as follows:

spin_lock_irqsave(&hidg->write_spinlock, flags); // first acquire
/* we our function has been disabled by host */
if (!hidg->req) {
	free_ep_req(hidg->in_ep, hidg->req);
	goto try_again;
}

[...]

status = usb_ep_queue(hidg->in_ep, hidg->req, GFP_ATOMIC);
=>
	[...]
	=> usb_gadget_giveback_request
		=>
		f_hidg_req_complete
			=>
			spin_lock_irqsave(&hidg->write_spinlock, flags); // second acquire

Note that this happens because dwc3 would call ->complete() on a
failed usb_ep_queue() due to failed Start Transfer command. This is,
anyway, a theoretical situation because dwc3 currently uses "No
Response Update Transfer" command for Bulk and Interrupt endpoints.

It's still good to make this case impossible to happen even if the "No
Reponse Update Transfer" command is changed.

Reported-by: Tuba Yavuz <tuba@ece.ufl.edu>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---

Greg, if you want to pick these two patches as they are, please go
ahead. If you want, I can also add a Cc stable tag, your call.

I'll prepare patches for v4.18 which will cleanup a few extra details
about how dwc3 kicks transfers and that will make the case a lot
clearer as to how things should behave.

Tuba, thanks for the reports :-) 

 drivers/usb/dwc3/gadget.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 550ee952c0d1..8796a5ee9bb9 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -166,18 +166,8 @@ static void dwc3_ep_inc_deq(struct dwc3_ep *dep)
 	dwc3_ep_inc_trb(&dep->trb_dequeue);
 }
 
-/**
- * dwc3_gadget_giveback - call struct usb_request's ->complete callback
- * @dep: The endpoint to whom the request belongs to
- * @req: The request we're giving back
- * @status: completion code for the request
- *
- * Must be called with controller's lock held and interrupts disabled. This
- * function will unmap @req and call its ->complete() callback to notify upper
- * layers that it has completed.
- */
-void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
-		int status)
+void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
+		struct dwc3_request *req, int status)
 {
 	struct dwc3			*dwc = dep->dwc;
 
@@ -190,18 +180,35 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
 
 	if (req->trb)
 		usb_gadget_unmap_request_by_dev(dwc->sysdev,
-						&req->request, req->direction);
+				&req->request, req->direction);
 
 	req->trb = NULL;
-
 	trace_dwc3_gadget_giveback(req);
 
+	if (dep->number > 1)
+		pm_runtime_put(dwc->dev);
+}
+
+/**
+ * dwc3_gadget_giveback - call struct usb_request's ->complete callback
+ * @dep: The endpoint to whom the request belongs to
+ * @req: The request we're giving back
+ * @status: completion code for the request
+ *
+ * Must be called with controller's lock held and interrupts disabled. This
+ * function will unmap @req and call its ->complete() callback to notify upper
+ * layers that it has completed.
+ */
+void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
+		int status)
+{
+	struct dwc3			*dwc = dep->dwc;
+
+	dwc3_gadget_del_and_unmap_request(dep, req, status);
+
 	spin_unlock(&dwc->lock);
 	usb_gadget_giveback_request(&dep->endpoint, &req->request);
 	spin_lock(&dwc->lock);
-
-	if (dep->number > 1)
-		pm_runtime_put(dwc->dev);
 }
 
 /**
@@ -1227,7 +1234,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 		if (req->trb)
 			memset(req->trb, 0, sizeof(struct dwc3_trb));
 		dep->queued_requests--;
-		dwc3_gadget_giveback(dep, req, ret);
+		dwc3_gadget_del_and_unmap_request(dep, req, ret);
 		return ret;
 	}
 

[2/2] usb: dwc3: gadget: never call ->complete() from ->ep_queue()

[Greg Kroah-Hartman]

On Mon, Mar 26, 2018 at 01:14:47PM +0300, Felipe Balbi wrote:
> This is a requirement which has always existed but, somehow, wasn't
> reflected in the documentation and problems weren't found until now
> when Tuba Yavuz found a possible deadlock happening between dwc3 and
> f_hid. She described the situation as follows:
> 
> spin_lock_irqsave(&hidg->write_spinlock, flags); // first acquire
> /* we our function has been disabled by host */
> if (!hidg->req) {
> 	free_ep_req(hidg->in_ep, hidg->req);
> 	goto try_again;
> }
> 
> [...]
> 
> status = usb_ep_queue(hidg->in_ep, hidg->req, GFP_ATOMIC);
> =>
> 	[...]
> 	=> usb_gadget_giveback_request
> 		=>
> 		f_hidg_req_complete
> 			=>
> 			spin_lock_irqsave(&hidg->write_spinlock, flags); // second acquire
> 
> Note that this happens because dwc3 would call ->complete() on a
> failed usb_ep_queue() due to failed Start Transfer command. This is,
> anyway, a theoretical situation because dwc3 currently uses "No
> Response Update Transfer" command for Bulk and Interrupt endpoints.
> 
> It's still good to make this case impossible to happen even if the "No
> Reponse Update Transfer" command is changed.
> 
> Reported-by: Tuba Yavuz <tuba@ece.ufl.edu>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: stable <stable@vger.kernel.org>
> ---
> 
> Greg, if you want to pick these two patches as they are, please go
> ahead. If you want, I can also add a Cc stable tag, your call.

I've added the stable tag and applied both of these patches now.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html