Hi,
Wesley Cheng <wcheng@xxxxxxxxxxxxxx> writes:
> The list_for_each_entry_safe() macro saves the current item (n) and
> the item after (n+1), so that n can be safely removed without
> corrupting the list. However, when traversing the list and removing
> items using gadget giveback, the DWC3 lock is briefly released,
> allowing other routines to execute. There is a situation where, while
> items are being removed from the cancelled_list using
> dwc3_gadget_ep_cleanup_cancelled_requests(), the pullup disable
> routine is running in parallel (due to UDC unbind). As the cleanup
> routine removes n, and the pullup disable removes n+1, once the
> cleanup retakes the DWC3 lock, it references a request who was already
> removed/handled. With list debug enabled, this leads to a panic.
> Ensure all instances of the macro are replaced where gadget giveback
> is used.
>
> Example call stack:
>
> Thread#1:
> __dwc3_gadget_ep_set_halt() - CLEAR HALT
> -> dwc3_gadget_ep_cleanup_cancelled_requests()
> ->list_for_each_entry_safe()
> ->dwc3_gadget_giveback(n)
> ->dwc3_gadget_del_and_unmap_request()- n deleted[cancelled_list]
> ->spin_unlock
> ->Thread#2 executes
> ...
> ->dwc3_gadget_giveback(n+1)
> ->Already removed!
>
> Thread#2:
> dwc3_gadget_pullup()
> ->waiting for dwc3 spin_lock
> ...
> ->Thread#1 released lock
> ->dwc3_stop_active_transfers()
> ->dwc3_remove_requests()
> ->fetches n+1 item from cancelled_list (n removed by Thread#1)
> ->dwc3_gadget_giveback()
> ->dwc3_gadget_del_and_unmap_request()- n+1 deleted[cancelled_list]
> ->spin_unlock
>
> Fixes: d4f1afe5e896 ("usb: dwc3: gadget: move requests to cancelled_list")
> Signed-off-by: Wesley Cheng <wcheng@xxxxxxxxxxxxxx>
> Reviewed-by: Peter Chen <peter.chen@xxxxxxxxxx>
> ---
> Changes in v2:
> - Updated commit message with context call stack of an example scenario
> seen on device.
>
> drivers/usb/dwc3/gadget.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index dd80e5c..efa939b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1737,10 +1737,10 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *r
> static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
> {
> struct dwc3_request *req;
> - struct dwc3_request *tmp;
> struct dwc3 *dwc = dep->dwc;
>
> - list_for_each_entry_safe(req, tmp, &dep->cancelled_list, list) {
> + while (!list_empty(&dep->cancelled_list)) {
> + req = next_request(&dep->cancelled_list);
couldn't this be solved list_replace_init() instead? Then we can keep
using the regular list_for_each_entry_safe() which has an added semantic
meaning due to its name.
--
balbi
Attachment:
signature.asc
Description: PGP signature
