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

[Oliver Neukum <oneukum@suse.de>]

Am Freitag, 20. April 2012, 04:10:09 schrieb Ming Lei:
> On Fri, Apr 20, 2012 at 12:11 AM, Oliver Neukum <oneukum@suse.de> wrote:
> > Am Donnerstag, 19. April 2012, 15:51:04 schrieb Ming Lei:
> >> The URB complete handler may be called by usb_unlink_urb directly,
> >> so deadlock will be triggered in __usbhid_submit_report since
> >> usbhid->lock is to be acquired in ctrl/out URB complete handler
> >> but it is hold before calling usb_unlink_urb.
> >>
> >> This patch avoids the deadlock by releasing the lock before
> >> calling usb_unlink_urb.
> >>
> >> CC: <stable@vger.kernel.org>
> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >> ---
> >>  drivers/hid/usbhid/hid-core.c |   16 ++++++++++------
> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> >> index aa1c503..b5d07da 100644
> >> --- a/drivers/hid/usbhid/hid-core.c
> >> +++ b/drivers/hid/usbhid/hid-core.c
> >> @@ -543,11 +543,13 @@ static void __usbhid_submit_report(struct hid_device *hid, struct hid_report *re
> >>                        * the queue is known to run
> >>                        * but an earlier request may be stuck
> >>                        * we may need to time out
> >> -                      * no race because this is called under
> >> -                      * spinlock
> >> +                      * release spinlock to avoid deadlock.
> >>                        */
> >> -                     if (time_after(jiffies, usbhid->last_out + HZ * 5))
> >> +                     if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
> >> +                             spin_unlock(&usbhid->lock);
> >>                               usb_unlink_urb(usbhid->urbout);
> >> +                             spin_lock(&usbhid->lock);
> >
> > The problem indeed exists on some HCDs.
> > I am afraid if you drop the lock there you introduce a race whereby
> > you might unlink the wrong request.
> 
> The complete handler is called just one time per one submit in either

Indeed.

> irq path or unlink path. Secondly, usb_unlink_urb itself is race free.
> Finally, usb_unlink_urb was always the last function called inside
> __usbhid_submit_report.

But under spinlock.
 
> So I don't see any races can be introduced by the patch.

You are racing with hid_irq_out(). It calls hid_submit_out()
under lock. So if hid_irq_out() is running between dropping
the lock and usb_unlink_urb() you may kill the newly submitted
urb, not the old urb that has timed out.
You must make sure that between the times you check usbhid->last_out
and calling unlink hid_submit_out() cannot be called.
You can't just drop the lock (at least on SMP)

	Regards
		Oliver