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

[Oliver Neukum <oneukum@suse.de>]

Am Freitag, 20. April 2012, 12:17:51 schrieb Ming Lei:
> On Fri, Apr 20, 2012 at 3:57 PM, Oliver Neukum <oneukum@suse.de> wrote:
> >
> > 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.
> 
> Yes, it is the race I missed, :-(
> 
> > 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)
> 
> Looks it is not easy to avoid the race if the lock is to be dropped.
> 
> So how about not acquiring the lock during unlinking as below?
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index aa1c503..b437223 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -429,7 +429,8 @@ static void hid_irq_out(struct urb *urb)
>                          urb->status);
>         }
> 
> -       spin_lock_irqsave(&usbhid->lock, flags);
> +       if (!test_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl))
> +               spin_lock_irqsave(&usbhid->lock, flags);
> 
>         if (unplug)
>                 usbhid->outtail = usbhid->outhead;
> @@ -438,12 +439,14 @@ static void hid_irq_out(struct urb *urb)
> 
>         if (usbhid->outhead != usbhid->outtail && !hid_submit_out(hid)) {
>                 /* Successfully submitted next urb in queue */
> -               spin_unlock_irqrestore(&usbhid->lock, flags);
> +               if (!test_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl))
> +                       spin_unlock_irqrestore(&usbhid->lock, flags);
>                 return;
>         }

No. This introduces a race where you can drop a lock you've not taken
and vice versa.
And why are you using per cpu structures here?

To be blunt, I'd prefer a guarantee that allows usb_unlink_urb() to be
called with spinlocks held.

Well, if we can't get a good usb_unlink_urb() then the lock must be dropped.
Your idea of setting a flag is good.

You'd do in hid_irq_out():

if (usbhid->outhead != usbhid->outtail && !usbhid->timeout_detected && !hid_submit_out(hid))

and you set usbhid->timeout_detected under lock. That way we can never
accidentally kill a new urb. This however means that then we need to kill the
urb and restart the queue preferably from a workqueue and this must be handled
in suspend/resume/close/pre-/postreset/disconnect

As I said, I'd very much appreciate sane semantics for usb_unlink_urb().

	Regards
		Oliver