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

[Oliver Neukum <oneukum@suse.de>]

Am Montag, den 27.07.2020, 14:27 +0300 schrieb Eli Billauer:
> Hello, Oliver.
> 
> On 27/07/20 13:14, Oliver Neukum wrote:
> > That however is really a kludge we cannot have in usbcore.
> > I am afraid as is the patch should_not_  be applied.
> >    
> 
> Could you please explain further why the suggested patch is unsuitable?

Hi,

certainly.

1. timeouts are generally a bad idea, especially if the timeout does
not come out of a spec.

2. That involves quoting you:

Alternatively, if the driver submits URBs to the same anchor while 
usb_kill_anchored_urbs() is called, this timeout might be reached. This 
could happen, for example, if the completer function that ran in the 
racy situation resubmits the URB. If that situation isn't cleared within 
1000ms, it means that there's a URB in the system that the driver isn't 
aware of. Maybe that situation is worth more than a WARN.

That is an entirely valid use case. And a bulk URB may take a potentially
unbounded time to complete.

My failure in this case is simply overengineering.
If this line:

        usb_unanchor_urb(urb);

In __usb_hcd_giveback_urb(struct urb *urb) weren't there, the issue
would not exist. I misdesigned the API in automatically unanchoring
a completing URB.
Simply removing it now is no longer possible, so we need to come up with
a more complex solution.

	Regards
		Oliver