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

[Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>]

Am Mittwoch, 25. April 2012, 09:02:58 schrieb Ming Lei:
> On Wed, Apr 25, 2012 at 2:19 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> 
> >> @@ -546,8 +557,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))
> >> +
> >> +                     if (time_after(jiffies, usbhid->last_out + HZ * 5) &&
> >> +                                     !usbhid->urbout->unlinked) {
> >> +                             spin_unlock(&usbhid->lock);
> >>                               usb_unlink_urb(usbhid->urbout);
> >> +                             spin_lock(&usbhid->lock);
> >> +                     }
> >>               }
> >>               return;
> >>       }
> >
> > Same objection. You are just making the race unlikelier. The flag
> > needs to be set under a lock you hold while checking time_after().
> 
> urb->unlinked is checked with holding both the two lockes, but set with
> holding the unlink lock or the lock or both, looks it is enough to
> avoid the race, isn't it?

That would work. Yet, having a second lock is not the best solution.

> > We'd be back at the original proposal.
> 
> Introducing a new flag to describe 'unlinked' is still OK, but
> borrowing urb->unlinked is better, the bad is that it is private.

And it needs to be explicitly checked. We'd better use urb->reject.
I've posted a patch to do so.

	Regards
		Oliver
--
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