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

[Ming Lei <ming.lei@xxxxxxxxxxxxx>]

On Fri, Apr 20, 2012 at 12:11 AM, Oliver Neukum <oneukum@xxxxxxx> 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@xxxxxxxxxxxxxxx>
>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
>> ---
>>  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
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.

So I don't see any races can be introduced by the patch.


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