From: Eli Billauer <eli.billauer@gmail.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
hdegoede@redhat.com, oneukum@suse.de
Subject: Re: [PATCH v2] usb: core: Solve race condition in anchor cleanup functions
Date: Wed, 29 Jul 2020 18:08:38 +0300 [thread overview]
Message-ID: <5F2190F6.5020901@gmail.com> (raw)
In-Reply-To: <20200729133846.GA1530967@rowland.harvard.edu>
On 29/07/20 16:38, Alan Stern wrote:
> With a small amount of restructuring you can eliminate three unlock-lock
> pairs and avoid the need for usb_anchor_safe_empty():
> [ ... ]
> All you have to do is move this spin_lock_irq() above the start of the
> outer loop...
> [ ... ]
> .. and move this spin_unlock_irq() below the end of the outer loop.
> Likewise for the two other routines.
>
>
I'm afraid that might not work. The whole purpose of the outer loop is
to kick in when urb_list is empty, but there's this unanchor-completer
race going on. So the inner loop will be skipped, because
list_empty(&anchor->urb_list) will evaluate true. As a result, the
spinlock will be held as the loop spins, until the completer has finished.
But if the completer tries to take the same lock, we're deadlocked. For
example, if it resubmits the URB, which is pretty much the point of this
extra while loop.
This is also the reason why I didn't just modify the original
while-loop's condition, so it would go on spinning as long the race
condition is in effect. It mustn't spin with the lock held.
>> > + } while (unlikely(!usb_anchor_safe_empty(anchor)));
>>
> likely() and unlikely() are frowned upon unless you can provide actual
> measurements showing that they make a significant difference. In this
> case they don't matter, since the bottleneck is the usb_kill_urb() call.
>
The irony is that I added this "unlikely" for the human reader, and not
for the compiler: I wanted to communicate that the outer loop is
unlikely to kick in. I'll keep that in mind for v3 of this patch.
Thanks,
Eli
next prev parent reply other threads:[~2020-07-29 15:09 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-29 10:31 [PATCH v2] usb: core: Solve race condition in anchor cleanup functions eli.billauer
2020-07-29 13:38 ` Alan Stern
2020-07-29 15:08 ` Eli Billauer [this message]
2020-07-30 7:28 ` Oliver Neukum
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=5F2190F6.5020901@gmail.com \
--to=eli.billauer@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=hdegoede@redhat.com \
--cc=linux-usb@vger.kernel.org \
--cc=oneukum@suse.de \
--cc=stern@rowland.harvard.edu \
/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).