Am Freitag, 20. April 2012, 14:53:24 schrieb Ming Lei:
> On Fri, Apr 20, 2012 at 6:45 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
> > Am Freitag, 20. April 2012, 12:17:51 schrieb Ming Lei:
> >> On Fri, Apr 20, 2012 at 3:57 PM, Oliver Neukum <oneukum@xxxxxxx> 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.
>
> The flag is defined as per cpu variable, so it can't changed during the two
> test_cpu_bit in complete handler and can't cause the races you mentioned.
Yes, you are right. It looks safe, but no longer understandable.
You need a big, fat comment to explain the problem and the solution.
Regards
Oliver
--
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
