Re: [PATCH] usb: dwc3: gadget: Replace list_for_each_entry_safe() if using giveback

[Wesley Cheng]

On 5/7/2021 8:45 PM, Peter Chen wrote:
> On 21-05-07 11:42:03, Wesley Cheng wrote:
>> 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,

Hi Peter,

Thanks for the review.

> 
> I see dwc3_gadget_del_and_unmap_request remove the list, the lock is
> still held there. Am I something wrong?
> 

The scenario the issue happens in is say the follow thread is running
the sequence below:

Thread#1:
__dwc3_gadget_ep_set_halt() - CLEAR HALT
  -> dwc3_gadget_ep_cleanup_cancelled_requests()
    ->list_for_each_entry_safe()
    ->dwc3_gadget_giveback()
      ->dwc3_gadget_del_and_unmap_request()-n deleted cancelled_list
      ->spin_unlock

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

So now, if thread#1 takes the DWC3 lock again, it will continue to item
n+1, which was already removed by thread#2, leading to a double list
removal.  We saw this issue on our platform after enabling list debug.

Thanks
Wesley Cheng

> Peter
>        
>> 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.
>>
>> Fixes: d4f1afe5e896 ("usb: dwc3: gadget: move requests to cancelled_list")
>> Signed-off-by: Wesley Cheng <wcheng@xxxxxxxxxxxxxx>
>> ---
>>  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);
>>  		dwc3_gadget_ep_skip_trbs(dep, req);
>>  		switch (req->status) {
>>  		case DWC3_REQUEST_STATUS_DISCONNECTED:
>> @@ -2935,11 +2935,11 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
>>  		const struct dwc3_event_depevt *event, int status)
>>  {
>>  	struct dwc3_request	*req;
>> -	struct dwc3_request	*tmp;
>>  
>> -	list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
>> +	while (!list_empty(&dep->started_list)) {
>>  		int ret;
>>  
>> +		req = next_request(&dep->started_list);
>>  		ret = dwc3_gadget_ep_cleanup_completed_request(dep, event,
>>  				req, status);
>>  		if (ret)
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project