Re: [PATCH] usb: core: Solve race condition in usb_kill_anchored_urbs

[Oliver Neukum <oneukum@suse.de>]

Am Montag, den 27.07.2020, 10:22 +0300 schrieb eli.billauer@gmail.com:
> From: Eli Billauer <eli.billauer@gmail.com>
> 
> usb_kill_anchored_urbs() is commonly used to cancel all URBs on an
> anchor just before releasing resources which the URBs rely on. By doing
> so, users of this function rely on that no completer callbacks will take
> place from any URB on the anchor after it returns.

Right, this is a use case that must work.

> However if this function is called in parallel with __usb_hcd_giveback_urb
> processing a URB on the anchor, the latter may call the completer
> callback after usb_kill_anchored_urbs() returns. This can lead to a
> kernel panic due to use after release of memory in interrupt context.
> 
> The race condition is that __usb_hcd_giveback_urb() first unanchors the URB
> and then makes the completer callback. Such URB is hence invisible to
> usb_kill_anchored_urbs(), allowing it to return before the completer has
> been called, since the anchor's urb_list is empty.

Well, the URB must be unachored because the callback may anchor the URB
again. What could we do? The refcount on the URB does not help, because
it guards only the URB itself.
It looks to me like I misdesigned the API a bit. What people really
need is an anchor that is not weighed by calling the callback.
Should I introduce such an API?

> This patch adds a call to usb_wait_anchor_empty_timeout() prior to
> returning. This function waits until urb_list is empty (it should
> already be), but more importantly, until @suspend_wakeups is zero.

That however is really a kludge we cannot have in usbcore.
I am afraid as is the patch should _not_ be applied.

	Regards
		Oliver