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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 24, 2012 at 10:22 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
> Am Dienstag, 24. April 2012, 06:19:00 schrieb Ming Lei:
>
>> @@ -486,11 +494,15 @@ static void hid_ctrl(struct urb *urb)
>>
>>       if (usbhid->ctrlhead != usbhid->ctrltail && !hid_submit_ctrl(hid)) {
>>               /* Successfully submitted next urb in queue */
>> +             if (status != -ECONNRESET)
>> +                     spin_unlock(&usbhid->unlink_lock);
>>               spin_unlock(&usbhid->lock);
>>               return;
>>       }
>>
>>       clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
>> +     if (status != -ECONNRESET)
>> +             spin_unlock(&usbhid->unlink_lock);
>>       spin_unlock(&usbhid->lock);
>>       usb_autopm_put_interface_async(usbhid->intf);
>>       wake_up(&usbhid->wait);
>
> Now you race against a double time out
>
> CPU A                                                           CPU B
>
> __usbhid_submit_report()
> time_after()
> usb_unlink_urb()
> -- this has to go to the hardware -->
>                                                                        hid_irq_out()
>                                                                        if (status != -ECONNRESET)
>                                                                        --> no lock
>                                                                        hid_submit_out()
> __usbhid_submit_report()
> time_after()
>                                                                        usb_submit_urb()

This submit won't happen because HID_OUT_RUNNING is not cleared.

> usb_unlink_urb()
>
>
>> @@ -546,8 +558,13 @@ static void __usbhid_submit_report(struct
>> hid_device *hid, struct hid_report *re
>>                        * no race because this is called under
>>                        * spinlock
>>                        */
>> -                     if (time_after(jiffies, usbhid->last_out + HZ * 5))
>> +                     spin_lock(&usbhid->unlink_lock);
>> +                     if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
>> +                             spin_unlock(&usbhid->lock);
>>                               usb_unlink_urb(usbhid->urbout);
>> +                             spin_lock(&usbhid->lock);
>> +                     }
>> +                     spin_unlock(&usbhid->unlink_lock);
>
> AB-BA deadlock

OK, if we always acquire unlink_lock before lock in usbhid_submit_report,
hid_led, hid_ctrl, and hid_irq_out, the AB-BA deadlock can be removed.

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux