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

[Wesley Cheng]

Hi Felipe,

On 5/19/2021 1:52 AM, Wesley Cheng wrote:
> 
> 
> On 5/11/2021 1:13 AM, Felipe Balbi wrote:
>>
>> 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.
>>
> 
> Hi Felipe,
> 
> Sorry for the late response.  So I tried with a list_replace_init() to
> within the list_for_each_entry_safe() loop to update tmp w/ the
> cancelled_list list head, but the issue was still observed.  This is
> because we can't replace the reference the loop already has stored in
> tmp, which is simply updated as the current item on the next iteration.
> 
> I believe this is what you were trying to achieve?
> 
Was wondering if you had any further inputs on this change?  As
mentioned, I tried a few things with list_replace_init(), which did not
work.

Thanks
Wesley Cheng

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