From: Felipe Balbi <balbi@kernel.org>
To: Wesley Cheng <wcheng@codeaurora.org>
Cc: Alan Stern <stern@rowland.harvard.edu>,
gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, jackp@codeaurora.org
Subject: Re: [PATCH] usb: dwc3: gadget: Use list_replace_init() before traversing lists
Date: Thu, 29 Jul 2021 12:31:59 +0300 [thread overview]
Message-ID: <87fsvxsbwc.fsf@kernel.org> (raw)
In-Reply-To: <4e06452a-080f-a2be-ab88-9ac992740ee0@codeaurora.org>
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
>>>
>>> Fix this condition by utilizing list_replace_init(), and traversing
>>> through a local copy of the current elements in the endpoint lists.
>>> This will also set the parent list as empty, so if another thread is
>>> also looping through the list, it will be empty on the next iteration.
>>>
>>> Fixes: d4f1afe5e896 ("usb: dwc3: gadget: move requests to cancelled_list")
>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>>>
>>> ---
>>> Previous patchset:
>>> https://lore.kernel.org/linux-usb/1620716636-12422-1-git-send-email-wcheng@codeaurora.org/
>>> ---
>>> drivers/usb/dwc3/gadget.c | 18 ++++++++++++++++--
>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index a29a4ca..3ce6ed9 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1926,9 +1926,13 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
>>> {
>>> struct dwc3_request *req;
>>> struct dwc3_request *tmp;
>>> + struct list_head local;
>>> struct dwc3 *dwc = dep->dwc;
>>>
>>> - list_for_each_entry_safe(req, tmp, &dep->cancelled_list, list) {
>>> +restart:
>>> + list_replace_init(&dep->cancelled_list, &local);
>>
>> hmm, if the lock is held and IRQs disabled when this runs, then no other
>> threads will be able to append requests to the list which makes the
>> "restart" label unnecessary, no?
>
> We do still call dwc3_gadget_giveback() which would release the lock
> briefly, so if there was another thread waiting on dwc->lock, it would
> be able to add additional items to that list.
>
>>
>> I wonder if we should release the lock and reenable interrupts after
>> replacing the head. The problem is that
>> dwc3_gadget_ep_cleanup_cancelled_requests() can run from the IRQ
>> handler.
>>
>
> We would also need to consider that some of the APIs being called in
> these situations would also have the assumption that the dwc->lock is
> held, ie dwc3_gadget_giveback()
yeah, good point. I think we're good to integrate this, unless Alan can
shed some light on some particular possible race scenario we may have
missed.
In any case:
Acked-by: Felipe Balbi <balbi@kernel.org>
--
balbi
next prev parent reply other threads:[~2021-07-29 9:32 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-29 7:33 [PATCH] usb: dwc3: gadget: Use list_replace_init() before traversing lists Wesley Cheng
2021-07-29 8:09 ` Felipe Balbi
2021-07-29 8:45 ` Wesley Cheng
2021-07-29 9:31 ` Felipe Balbi [this message]
2021-07-29 14:20 ` Alan Stern
2021-08-09 21:04 ` John Stultz
2021-08-09 22:31 ` [RFC][PATCH] dwc3: gadget: Fix losing list items in dwc3_gadget_ep_cleanup_completed_requests() John Stultz
2021-08-09 22:44 ` Thinh Nguyen
2021-08-09 22:53 ` John Stultz
2021-08-09 22:57 ` Thinh Nguyen
2021-08-10 6:05 ` Greg Kroah-Hartman
2021-08-10 7:11 ` Greg Kroah-Hartman
2021-08-10 17:11 ` Wesley Cheng
2021-08-10 20:14 ` Thinh Nguyen
2021-08-10 20:17 ` Thinh Nguyen
2021-08-10 23:40 ` Thinh Nguyen
2021-08-09 21:26 ` [PATCH] usb: dwc3: gadget: Use list_replace_init() before traversing lists John Stultz
2021-08-09 22:07 ` Thinh Nguyen
2021-08-10 3:12 ` Ray Chi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87fsvxsbwc.fsf@kernel.org \
--to=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jackp@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=stern@rowland.harvard.edu \
--cc=wcheng@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).