Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: wcheng=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 50F49C433F1
Subject: Re: [PATCH v2] usb: dwc3: gadget: Replace list_for_each_entry_safe()
 if using giveback
From:   Wesley Cheng <wcheng@codeaurora.org>
To:     Felipe Balbi <balbi@kernel.org>, gregkh@linuxfoundation.org,
        peter.chen@kernel.org
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        jackp@codeaurora.org
References: <1620716636-12422-1-git-send-email-wcheng@codeaurora.org>
 <87tun9g01v.fsf@kernel.org>
 <2675db9e-0cab-06b5-2986-0b4456a1f040@codeaurora.org>
 <5156238d-c1d8-a0d3-47af-8b52467fd071@codeaurora.org>
Message-ID: <fc346f3c-6e3d-b96c-d64a-2ae4cf4218d4@codeaurora.org>
Date:   Tue, 20 Jul 2021 23:14:14 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <5156238d-c1d8-a0d3-47af-8b52467fd071@codeaurora.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Felipe,

On 6/9/2021 1:57 PM, Wesley Cheng wrote:
> 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@codeaurora.org> 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@codeaurora.org>
>>>> Reviewed-by: Peter Chen <peter.chen@kernel.org>
>>>> ---
>>>> 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.
> 

Sorry for the ping.  Is this change OK to add as is?  We've been running
into this instance pretty frequently during our testing, so just wanted
to close on the proper changes being merged upstream.

Thanks
Wesley Cheng


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