Re: [PATCH] hid: usbhid: fix possible deadlock in __usbhid_submit_report

[Oliver Neukum <oneukum@suse.de>]

Am Sonntag, 22. April 2012, 12:51:26 schrieb Ming Lei:
> On Sun, Apr 22, 2012 at 5:49 AM, Alan Stern <stern@rowland.harvard.edu> wrote:

> > Although the kerneldoc doesn't actually say so, it should be safe to
> > assume that usb_unlink_urb calls the completion routine directly _only_
> > in cases where the unlink succeeded.  (We could add this to the
> > kerneldoc.)
> >
> > Therefore: If the URB completes with status other than -ECONNRESET then
> > you can safely take the lock for resubmission.  If the URB completes
> > with status == -ECONNRESET then you know it was unlinked, so you don't
> > need to take the lock -- the race has already been lost.
> >
> > Does that solve your problem?
> 
> Not sure if that does work.

I am afraid it does not work.

> If the URB completes asynchronously after unlinking, its status is still
>  -ECONNRESET, so extra race may be caused without holding the lock
> because complete handler will access some global data.

That is the race. And you need not invoke global data. The original
race opens again if you are submitting a new URB without the lock
held.
This is because we cannot be sure that the same URB is unlinked
only once. A subsequent timeout may kill the wrong URB if the
first is unlinked so that the callback really comes in interrupt.

But the basic idea is brilliant. It's just that the one way logical implication:
recursive direct call of the callback -> status == -ECONNRESET
is not strong enough. But that is very easy to fix. As we know whether
the callback is directly called or not, all we need to do is differentiate
the cases in urb->status, by introducing a new error code.

	Regards
		Oliver