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

[Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>]

On Tue, Apr 24, 2012 at 11:20 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Tue, 24 Apr 2012, Ming Lei wrote:
>
>> > Instead of changing return codes or adding locks, how about
>> > implementing a small state machine for each URB?
>> >
>> >        Initially the state is ACTIVE.
>> >
>> >        When the URB times out, acquire the lock.  If the state is not
>> >        equal to ACTIVE, drop the lock and return immediately (the URB
>> >        is being unlinked concurrently).  Otherwise set the state to
>> >        UNLINK_STARTED, drop the lock, call usb_unlink_urb, and
>> >        reacquire the lock.  If the state hasn't changed, set it back
>> >        to ACTIVE.  But if the state has changed to UNLINK_FINISHED,
>> >        set it to ACTIVE and resubmit.
>> >
>> >        In the completion handler, grab the lock.  If the state
>> >        is ACTIVE, resubmit.  But if the state is UNLINK_STARTED,
>> >        change it to UNLINK_FINISHED and don't resubmit.
>>
>> Sounds a smart solution, but extra care should be put on submit urb
>> in other context, maybe the lock is to be held to check URB state before
>> submitting it because the lock is released during usb_unlink_urb.
>
> Don't you face a similar problem right now?  What happens if the
> timeout expires while the URB completion handler is running?  Then the
> timer routine could end up unlinking the resubmitted URB instead of the
> original transfer.
>
> You need a sort of "generation number", or something like that, to
> prevent this kind of mistake.  The same mechanism should work with the
> state machine.
>
> Are you saying that every place where the URB is submitted, the state
> has to be set to ACTIVE first, and this has to done with the lock held?

Maybe it is not enough. If the URB state is set as UNLINK_STARTED
and the lock is released, will you continue the submitting?

> Yes, that's right.
>
>> Also basically the URB state is maintained by device driver, so looks
>> better to add the state into specific device driver instead of struct URB to
>> save each URB storage, but if so, the solution will become more
>> complicated than the way of adding lock.
>
> Why?  In one case you add a state variable, in the other case you add a
> spinlock.  One isn't much harder than the other.

I mean the whole URB state machine is to be maintained by device driver,
not simply adding a state variable.  But for the way of adding lock, just
several lines of spin_lock/spin_unlock with check on urb->status are
enough.

Looks both ways are generic enough to avoid the problem, and adding
lock is simpler.

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