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

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

[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@vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 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);
+			}
 		}
 		return;
 	}
@@ -591,11 +593,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_ctrl + HZ * 5))
+		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
+			spin_unlock(&usbhid->lock);
 			usb_unlink_urb(usbhid->urbctrl);
+			spin_lock(&usbhid->lock);
+		}
 	}
 }
 
-- 
1.7.9.5

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

[Oliver Neukum]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Ming Lei <ming.lei-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  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.

	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

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

[Ming Lei]

On Fri, Apr 20, 2012 at 12:11 AM, Oliver Neukum <oneukum@suse.de> 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@vger.kernel.org>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  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-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

Am Freitag, 20. April 2012, 04:10:09 schrieb Ming Lei:
> On Fri, Apr 20, 2012 at 12:11 AM, Oliver Neukum <oneukum@suse.de> 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@vger.kernel.org>
> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >> ---
> >>  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

Indeed.

> 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.

But under spinlock.
 
> So I don't see any races can be introduced by the patch.

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.
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)

	Regards
		Oliver

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

[Ming Lei]

On Fri, Apr 20, 2012 at 3:57 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> 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;
 	}

 	clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
-	spin_unlock_irqrestore(&usbhid->lock, flags);
+	if (!test_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl))
+		spin_unlock_irqrestore(&usbhid->lock, flags);
 	usb_autopm_put_interface_async(usbhid->intf);
 	wake_up(&usbhid->wait);
 }
@@ -458,7 +461,8 @@ static void hid_ctrl(struct urb *urb)
 	struct usbhid_device *usbhid = hid->driver_data;
 	int unplug = 0, status = urb->status;

-	spin_lock(&usbhid->lock);
+	if (!test_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl))
+		spin_lock(&usbhid->lock);

 	switch (status) {
 	case 0:			/* success */
@@ -486,12 +490,14 @@ static void hid_ctrl(struct urb *urb)

 	if (usbhid->ctrlhead != usbhid->ctrltail && !hid_submit_ctrl(hid)) {
 		/* Successfully submitted next urb in queue */
-		spin_unlock(&usbhid->lock);
+		if (!test_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl))
+			spin_unlock(&usbhid->lock);
 		return;
 	}

 	clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
-	spin_unlock(&usbhid->lock);
+	if (!test_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl))
+		spin_unlock(&usbhid->lock);
 	usb_autopm_put_interface_async(usbhid->intf);
 	wake_up(&usbhid->wait);
 }
@@ -546,8 +552,11 @@ 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)) {
+				set_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl);
 				usb_unlink_urb(usbhid->urbout);
+				clear_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl);
+			}
 		}
 		return;
 	}
@@ -594,8 +603,11 @@ 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_ctrl + HZ * 5))
-			usb_unlink_urb(usbhid->urbctrl);
+		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
+				set_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl);
+				usb_unlink_urb(usbhid->urbctrl);
+				clear_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl);
+		}
 	}
 }

@@ -1294,6 +1306,12 @@ static int usbhid_probe(struct usb_interface
*intf, const struct usb_device_id *
 	usbhid->hid = hid;
 	usbhid->intf = intf;
 	usbhid->ifnum = interface->desc.bInterfaceNumber;
+	usbhid->cpuiofl = alloc_percpu(unsigned long);
+	if (!usbhid->cpuiofl) {
+		ret = -ENOMEM;
+		goto err_free;
+	}
+

 	init_waitqueue_head(&usbhid->wait);
 	INIT_WORK(&usbhid->reset_work, hid_reset);
@@ -1306,10 +1324,12 @@ static int usbhid_probe(struct usb_interface
*intf, const struct usb_device_id *
 	if (ret) {
 		if (ret != -ENODEV)
 			hid_err(intf, "can't add hid device: %d\n", ret);
-		goto err_free;
+		goto err_add_dev;
 	}

 	return 0;
+err_add_dev:
+	free_percpu(usbhid->cpuiofl);
 err_free:
 	kfree(usbhid);
 err:
@@ -1327,6 +1347,7 @@ static void usbhid_disconnect(struct usb_interface *intf)

 	usbhid = hid->driver_data;
 	hid_destroy_device(hid);
+	free_percpu(usbhid->cpuiofl);
 	kfree(usbhid);
 }

diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 1883d7b..8fd240e 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -57,6 +57,9 @@ struct usb_interface *usbhid_find_interface(int minor);
 #define HID_KEYS_PRESSED	10
 #define HID_NO_BANDWIDTH	11

+/*per cpu iofl flags */
+#define HID_PCPU_TIMEOUT	1
+
 /*
  * USB-specific HID struct, to be pointed to
  * from struct hid_device->driver_data
@@ -91,6 +94,7 @@ struct usbhid_device {

 	spinlock_t lock;						/* fifo spinlock */
 	unsigned long iofl;                                             /*
I/O flags (CTRL_RUNNING, OUT_RUNNING) */
+	unsigned long __percpu *cpuiofl;                                /*
per cpu I/O flags (PCPU_TIMEOUT) */
 	struct timer_list io_retry;                                     /*
Retry timer */
 	unsigned long stop_retry;                                       /*
Time to give up, in jiffies */
 	unsigned int retry_delay;                                       /*
Delay length in ms */
@@ -104,5 +108,23 @@ struct usbhid_device {
 #define	hid_to_usb_dev(hid_dev) \
 	container_of(hid_dev->dev.parent->parent, struct usb_device, dev)

+static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+	unsigned long *fl = __this_cpu_ptr(addr);
+	set_bit(nr, fl);
+}
+
+static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+	unsigned long *fl = __this_cpu_ptr(addr);
+	clear_bit(nr, fl);
+}
+
+static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
+{
+	unsigned long *fl = __this_cpu_ptr(addr);
+	return test_bit(nr, fl);
+}
+
 #endif


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

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

[Oliver Neukum]

Am Freitag, 20. April 2012, 12:17:51 schrieb Ming Lei:
> On Fri, Apr 20, 2012 at 3:57 PM, Oliver Neukum <oneukum@suse.de> 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.
And why are you using per cpu structures here?

To be blunt, I'd prefer a guarantee that allows usb_unlink_urb() to be
called with spinlocks held.

Well, if we can't get a good usb_unlink_urb() then the lock must be dropped.
Your idea of setting a flag is good.

You'd do in hid_irq_out():

if (usbhid->outhead != usbhid->outtail && !usbhid->timeout_detected && !hid_submit_out(hid))

and you set usbhid->timeout_detected under lock. That way we can never
accidentally kill a new urb. This however means that then we need to kill the
urb and restart the queue preferably from a workqueue and this must be handled
in suspend/resume/close/pre-/postreset/disconnect

As I said, I'd very much appreciate sane semantics for usb_unlink_urb().

	Regards
		Oliver

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

[Ming Lei]

On Fri, Apr 20, 2012 at 6:45 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Freitag, 20. April 2012, 12:17:51 schrieb Ming Lei:
>> On Fri, Apr 20, 2012 at 3:57 PM, Oliver Neukum <oneukum@suse.de> 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.

> And why are you using per cpu structures here?

If we want to introduce flag to address the issue, I think it has to be defined
per CPU because we can't acquire the lock in complete handler triggered
by unlink path, but at the same time we must spin on the lock in complete
handler from other CPUs(non unlinking, irq context)  to avoid races.

IMO, the patch can fix the deadlock problem.

>
> To be blunt, I'd prefer a guarantee that allows usb_unlink_urb() to be
> called with spinlocks held.

I'd prefer it to, but looks it is not practical because quite a few
host controller
drivers need to be modified.

>
> Well, if we can't get a good usb_unlink_urb() then the lock must be dropped.
> Your idea of setting a flag is good.
>
> You'd do in hid_irq_out():
>
> if (usbhid->outhead != usbhid->outtail && !usbhid->timeout_detected && !hid_submit_out(hid))
>
> and you set usbhid->timeout_detected under lock. That way we can never
> accidentally kill a new urb. This however means that then we need to kill the
> urb and restart the queue preferably from a workqueue and this must be handled
> in suspend/resume/close/pre-/postreset/disconnect

It is a solution, but looks far more complicated than the per cpu flag patch.

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

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

[Oliver Neukum]

Am Freitag, 20. April 2012, 14:53:24 schrieb Ming Lei:
> On Fri, Apr 20, 2012 at 6:45 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > Am Freitag, 20. April 2012, 12:17:51 schrieb Ming Lei:
> >> On Fri, Apr 20, 2012 at 3:57 PM, Oliver Neukum <oneukum@suse.de> 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

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

[Ming Lei]

On Fri, Apr 20, 2012 at 6:45 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:

> To be blunt, I'd prefer a guarantee that allows usb_unlink_urb() to be
> called with spinlocks held.

It is allowed to be called with spinlocks held, but the same lock is not
allowed to be acquired in complete handler.

I understand you mean URB complete handler should be guaranteed
to be run only in IRQ context and can't be called by usb_unlink_urb,
don't I?

If yes, many hc drivers are still involved to be modified.

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

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

[Alan Stern]

On Fri, 20 Apr 2012, Oliver Neukum wrote:

> As I said, I'd very much appreciate sane semantics for usb_unlink_urb().

Aside from the practicality issue of altering a large number of
existing drivers, changing the semantics the way you want would be
difficult because it would force the HCDs to defer some giveback
operations to a bottom half or timer routine.

Think about what happens if the URB being unlinked hasn't been
presented to the hardware yet.  Once it has been removed from the HCD's
internal lists, there's no reason not to give it back right away.  And
there's no natural time to give it back later.

Alan Stern

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

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

[Oliver Neukum]

Am Samstag, 21. April 2012, 02:37:35 schrieb Alan Stern:
> On Fri, 20 Apr 2012, Oliver Neukum wrote:
> 
> > As I said, I'd very much appreciate sane semantics for usb_unlink_urb().
> 
> Aside from the practicality issue of altering a large number of
> existing drivers, changing the semantics the way you want would be
> difficult because it would force the HCDs to defer some giveback
> operations to a bottom half or timer routine.

Or a work queue, which would have to be dedicated to avoid deadlocks
with storage.

> Think about what happens if the URB being unlinked hasn't been
> presented to the hardware yet.  Once it has been removed from the HCD's
> internal lists, there's no reason not to give it back right away.  And
> there's no natural time to give it back later.

Now. The question is not when, but from which context.
The context should be uniform, so that the requirements
for locking should also be uniform.

	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

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

[Ming Lei]

On Sat, Apr 21, 2012 at 6:25 PM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Samstag, 21. April 2012, 02:37:35 schrieb Alan Stern:
>> On Fri, 20 Apr 2012, Oliver Neukum wrote:
>>
>> > As I said, I'd very much appreciate sane semantics for usb_unlink_urb().
>>
>> Aside from the practicality issue of altering a large number of
>> existing drivers, changing the semantics the way you want would be
>> difficult because it would force the HCDs to defer some giveback
>> operations to a bottom half or timer routine.
>
> Or a work queue, which would have to be dedicated to avoid deadlocks
> with storage.
>
>> Think about what happens if the URB being unlinked hasn't been
>> presented to the hardware yet.  Once it has been removed from the HCD's
>> internal lists, there's no reason not to give it back right away.  And
>> there's no natural time to give it back later.
>
> Now. The question is not when, but from which context.
> The context should be uniform, so that the requirements
> for locking should also be uniform.

How about always scheduling a tasklet to run what usb_unlink_urb does?
just implement usb_unlink_urb as something like
tasklet_schedule(unlink_tasklet).

Then we can have a uniform lock requirement and no changes are involved
on host controller drivers.


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

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1098 bytes --]

On Sat, 21 Apr 2012, Ming Lei wrote:

> >> Think about what happens if the URB being unlinked hasn't been
> >> presented to the hardware yet.  Once it has been removed from the HCD's
> >> internal lists, there's no reason not to give it back right away.  And
> >> there's no natural time to give it back later.
> >
> > Now. The question is not when, but from which context.
> > The context should be uniform, so that the requirements
> > for locking should also be uniform.
> 
> How about always scheduling a tasklet to run what usb_unlink_urb does?
> just implement usb_unlink_urb as something like
> tasklet_schedule(unlink_tasklet).
> 
> Then we can have a uniform lock requirement and no changes are involved
> on host controller drivers.

The return values would not be correct.

On the other hand, usbnet could call usb_unlink_urb from within a 
tasklet.

Alan Stern

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

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

[Oliver Neukum]

Am Samstag, 21. April 2012, 19:31:20 schrieb Alan Stern:
> On Sat, 21 Apr 2012, Ming Lei wrote:
> 
> > >> Think about what happens if the URB being unlinked hasn't been
> > >> presented to the hardware yet. �Once it has been removed from the HCD's
> > >> internal lists, there's no reason not to give it back right away. �And
> > >> there's no natural time to give it back later.
> > >
> > > Now. The question is not when, but from which context.
> > > The context should be uniform, so that the requirements
> > > for locking should also be uniform.
> > 
> > How about always scheduling a tasklet to run what usb_unlink_urb does?
> > just implement usb_unlink_urb as something like
> > tasklet_schedule(unlink_tasklet).
> > 
> > Then we can have a uniform lock requirement and no changes are involved
> > on host controller drivers.
> 
> The return values would not be correct.
> 
> On the other hand, usbnet could call usb_unlink_urb from within a 
> tasklet.

No. That would introduce a race.
Basically whenever a completion handler resubmits itself, any timeout
has to avoid a race. Checking for a timeout and unlinking must be atomic,
just as the completion handler must make the resubmission and noting
the time atomic.
If I cannot do this with a lock, then a much more complex pattern is necessary.

	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

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

[Alan Stern]

On Sat, 21 Apr 2012, Oliver Neukum wrote:

> > On the other hand, usbnet could call usb_unlink_urb from within a 
> > tasklet.
> 
> No. That would introduce a race.
> Basically whenever a completion handler resubmits itself, any timeout
> has to avoid a race. Checking for a timeout and unlinking must be atomic,
> just as the completion handler must make the resubmission and noting
> the time atomic.
> If I cannot do this with a lock, then a much more complex pattern is necessary.

I see.  I never really understood the problem before.

Although the kerneldoc doesn't actually say so, it should be safe to
assume that usb_unlink_urb calls the completion routine directly _only_
in cases where the unlink succeeded.  (We could add this to the 
kerneldoc.)

Therefore: If the URB completes with status other than -ECONNRESET then 
you can safely take the lock for resubmission.  If the URB completes 
with status == -ECONNRESET then you know it was unlinked, so you don't 
need to take the lock -- the race has already been lost.

Does that solve your problem?

Alan Stern

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

[Ming Lei]

On Sun, Apr 22, 2012 at 5:49 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Sat, 21 Apr 2012, Oliver Neukum wrote:
>
>> > On the other hand, usbnet could call usb_unlink_urb from within a
>> > tasklet.
>>
>> No. That would introduce a race.
>> Basically whenever a completion handler resubmits itself, any timeout
>> has to avoid a race. Checking for a timeout and unlinking must be atomic,
>> just as the completion handler must make the resubmission and noting
>> the time atomic.
>> If I cannot do this with a lock, then a much more complex pattern is necessary.
>
> I see.  I never really understood the problem before.
>
> Although the kerneldoc doesn't actually say so, it should be safe to
> assume that usb_unlink_urb calls the completion routine directly _only_
> in cases where the unlink succeeded.  (We could add this to the
> kerneldoc.)
>
> Therefore: If the URB completes with status other than -ECONNRESET then
> you can safely take the lock for resubmission.  If the URB completes
> with status == -ECONNRESET then you know it was unlinked, so you don't
> need to take the lock -- the race has already been lost.
>
> Does that solve your problem?

Not sure if that does work.

If the URB completes asynchronously after unlinking, its status is still
 -ECONNRESET, so extra race may be caused without holding the lock
because complete handler will access some global data.



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

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1251 bytes --]

On Sun, 22 Apr 2012, Ming Lei wrote:

> > Although the kerneldoc doesn't actually say so, it should be safe to
> > assume that usb_unlink_urb calls the completion routine directly _only_
> > in cases where the unlink succeeded.  (We could add this to the
> > kerneldoc.)
> >
> > Therefore: If the URB completes with status other than -ECONNRESET then
> > you can safely take the lock for resubmission.  If the URB completes
> > with status == -ECONNRESET then you know it was unlinked, so you don't
> > need to take the lock -- the race has already been lost.
> >
> > Does that solve your problem?
> 
> Not sure if that does work.
> 
> If the URB completes asynchronously after unlinking, its status is still
>  -ECONNRESET, so extra race may be caused without holding the lock
> because complete handler will access some global data.

That would be a completely separate race, right?  So maybe it can use a 
different lock for protection -- and this other lock could be dropped 
before usb_unlink_urb is called.

Alan Stern

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

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

[Ming Lei]

On Sun, Apr 22, 2012 at 8:50 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sun, 22 Apr 2012, Ming Lei wrote:
>
>> > Although the kerneldoc doesn't actually say so, it should be safe to
>> > assume that usb_unlink_urb calls the completion routine directly _only_
>> > in cases where the unlink succeeded.  (We could add this to the
>> > kerneldoc.)
>> >
>> > Therefore: If the URB completes with status other than -ECONNRESET then
>> > you can safely take the lock for resubmission.  If the URB completes
>> > with status == -ECONNRESET then you know it was unlinked, so you don't
>> > need to take the lock -- the race has already been lost.
>> >
>> > Does that solve your problem?
>>
>> Not sure if that does work.
>>
>> If the URB completes asynchronously after unlinking, its status is still
>>  -ECONNRESET, so extra race may be caused without holding the lock
>> because complete handler will access some global data.
>
> That would be a completely separate race, right?  So maybe it can use a

Not sure, at least in both usbnet and usbhid cases, the lock is held before
usb_unlink_urb, and the same lock is to be acquired in the URB complete
handler.

> different lock for protection -- and this other lock could be dropped
> before usb_unlink_urb is called.

If the lock which is to be acquired in the URB complete handler is dropped
before calling usb_unlink_urb, one new submitted URB in complete handler
may be unlinked, as mentioned by Oliver already.



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

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 4253 bytes --]

On Sun, 22 Apr 2012, Ming Lei wrote:

> On Sun, Apr 22, 2012 at 8:50 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sun, 22 Apr 2012, Ming Lei wrote:
> >
> >> > Although the kerneldoc doesn't actually say so, it should be safe to
> >> > assume that usb_unlink_urb calls the completion routine directly _only_
> >> > in cases where the unlink succeeded.  (We could add this to the
> >> > kerneldoc.)
> >> >
> >> > Therefore: If the URB completes with status other than -ECONNRESET then
> >> > you can safely take the lock for resubmission.  If the URB completes
> >> > with status == -ECONNRESET then you know it was unlinked, so you don't
> >> > need to take the lock -- the race has already been lost.
> >> >
> >> > Does that solve your problem?
> >>
> >> Not sure if that does work.
> >>
> >> If the URB completes asynchronously after unlinking, its status is still
> >>  -ECONNRESET, so extra race may be caused without holding the lock
> >> because complete handler will access some global data.
> >
> > That would be a completely separate race, right?  So maybe it can use a
> 
> Not sure, at least in both usbnet and usbhid cases, the lock is held before
> usb_unlink_urb, and the same lock is to be acquired in the URB complete
> handler.
> 
> > different lock for protection -- and this other lock could be dropped
> > before usb_unlink_urb is called.
> 
> If the lock which is to be acquired in the URB complete handler is dropped
> before calling usb_unlink_urb, one new submitted URB in complete handler
> may be unlinked, as mentioned by Oliver already.

We are now talking about two locks.  One of them is held during the 
call to usb_unlink_urb; the completion handler does not acquire that 
lock if the URB's status is -ECONNRESET.  The other lock is dropped 
before usb_unlink_urb is called, so the completion handler can safely 
grab it.


On Mon, 23 Apr 2012, Oliver Neukum wrote:

> > If the URB completes asynchronously after unlinking, its status is still
> >  -ECONNRESET, so extra race may be caused without holding the lock
> > because complete handler will access some global data.
> 
> That is the race. And you need not invoke global data. The original
> race opens again if you are submitting a new URB without the lock
> held.
> This is because we cannot be sure that the same URB is unlinked
> only once. A subsequent timeout may kill the wrong URB if the
> first is unlinked so that the callback really comes in interrupt.
> 
> But the basic idea is brilliant. It's just that the one way logical implication:
> recursive direct call of the callback -> status == -ECONNRESET
> is not strong enough. But that is very easy to fix. As we know whether
> the callback is directly called or not, all we need to do is differentiate
> the cases in urb->status, by introducing a new error code.

I don't like the idea of changing the status codes.  It would mean 
changing usb_kill_urb too.

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.

This is a better approach, in that it doesn't make any assumptions 
regarding synchronous vs. asynchronous unlinks.  If you want, you could 
have two different ACTIVE substates, one for URBs which haven't yet 
been unlinked and one for URBs which have been.  Then you could avoid 
unlinking the same URB twice.

Alan Stern

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

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

[Ming Lei]

On Mon, Apr 23, 2012 at 11:42 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sun, 22 Apr 2012, Ming Lei wrote:

>> If the lock which is to be acquired in the URB complete handler is dropped
>> before calling usb_unlink_urb, one new submitted URB in complete handler
>> may be unlinked, as mentioned by Oliver already.
>
> We are now talking about two locks.  One of them is held during the
> call to usb_unlink_urb; the completion handler does not acquire that
> lock if the URB's status is -ECONNRESET.  The other lock is dropped
> before usb_unlink_urb is called, so the completion handler can safely
> grab it.

Yes, adding a new unlink lock plus  -ECONNRESET status can
fix the problem, and it is a very simple solution, just like below:

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index aa1c503..a1adb81 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -430,6 +430,8 @@ static void hid_irq_out(struct urb *urb)
 	}

 	spin_lock_irqsave(&usbhid->lock, flags);
+	if (status != -ECONNRESET)
+		spin_lock(&usbhid->unlink_lock);

 	if (unplug)
 		usbhid->outtail = usbhid->outhead;
@@ -438,11 +440,15 @@ static void hid_irq_out(struct urb *urb)

 	if (usbhid->outhead != usbhid->outtail && !hid_submit_out(hid)) {
 		/* Successfully submitted next urb in queue */
+		if (status != -ECONNRESET)
+			spin_unlock(&usbhid->unlink_lock);
 		spin_unlock_irqrestore(&usbhid->lock, flags);
 		return;
 	}

 	clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
+	if (status != -ECONNRESET)
+		spin_unlock(&usbhid->unlink_lock);
 	spin_unlock_irqrestore(&usbhid->lock, flags);
 	usb_autopm_put_interface_async(usbhid->intf);
 	wake_up(&usbhid->wait);
@@ -459,6 +465,8 @@ static void hid_ctrl(struct urb *urb)
 	int unplug = 0, status = urb->status;

 	spin_lock(&usbhid->lock);
+	if (status != -ECONNRESET)
+		spin_lock(&usbhid->unlink_lock);

 	switch (status) {
 	case 0:			/* success */
@@ -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);
@@ -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);
 		}
 		return;
 	}
@@ -594,8 +611,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_ctrl + HZ * 5))
+		spin_lock(&usbhid->unlink_lock);
+		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
+			spin_unlock(&usbhid->lock);
 			usb_unlink_urb(usbhid->urbctrl);
+			spin_lock(&usbhid->lock);
+		}
+		spin_unlock(&usbhid->unlink_lock);
 	}
 }

@@ -1299,6 +1321,7 @@ static int usbhid_probe(struct usb_interface
*intf, const struct usb_device_id *
 	INIT_WORK(&usbhid->reset_work, hid_reset);
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
 	spin_lock_init(&usbhid->lock);
+	spin_lock_init(&usbhid->unlink_lock);

 	INIT_WORK(&usbhid->led_work, hid_led);

diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 1883d7b..69af387 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -90,6 +90,7 @@ struct usbhid_device {
 	unsigned long last_out;							/* record of last output for timeouts */

 	spinlock_t lock;						/* fifo spinlock */
+	spinlock_t unlink_lock;						/* unlink spinlock */
 	unsigned long iofl;                                             /*
I/O flags (CTRL_RUNNING, OUT_RUNNING) */
 	struct timer_list io_retry;                                     /*
Retry timer */
 	unsigned long stop_retry;                                       /*
Time to give up, in jiffies */


>
> On Mon, 23 Apr 2012, Oliver Neukum wrote:
>
>> > If the URB completes asynchronously after unlinking, its status is still
>> >  -ECONNRESET, so extra race may be caused without holding the lock
>> > because complete handler will access some global data.
>>
>> That is the race. And you need not invoke global data. The original
>> race opens again if you are submitting a new URB without the lock
>> held.
>> This is because we cannot be sure that the same URB is unlinked
>> only once. A subsequent timeout may kill the wrong URB if the
>> first is unlinked so that the callback really comes in interrupt.
>>
>> But the basic idea is brilliant. It's just that the one way logical implication:
>> recursive direct call of the callback -> status == -ECONNRESET
>> is not strong enough. But that is very easy to fix. As we know whether
>> the callback is directly called or not, all we need to do is differentiate
>> the cases in urb->status, by introducing a new error code.
>
> I don't like the idea of changing the status codes.  It would mean
> changing usb_kill_urb too.
>
> 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.

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.

> This is a better approach, in that it doesn't make any assumptions
> regarding synchronous vs. asynchronous unlinks.  If you want, you could
> have two different ACTIVE substates, one for URBs which haven't yet
> been unlinked and one for URBs which have been.  Then you could avoid
> unlinking the same URB twice.


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

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

[Oliver Neukum]

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()
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

	Regards
		Oliver

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

[Ming Lei]

On Tue, Apr 24, 2012 at 10:22 PM, Oliver Neukum <oneukum@suse.de> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

Am Dienstag, 24. April 2012, 17:46:41 schrieb Ming Lei:
> On Tue, Apr 24, 2012 at 10:22 PM, Oliver Neukum <oneukum@suse.de> 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.

I may be dense, but as far as I can tell a resubmit will happen, exactly if
HID_OUT_RUNNING is _not_ cleared.

	Regards
		Oliver

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

[Ming Lei]

On Wed, Apr 25, 2012 at 2:57 AM, Oliver Neukum <oliver@neukum.org> wrote:

 usb_submit_urb()
>>
>> This submit won't happen because HID_OUT_RUNNING is not cleared.
>
> I may be dense, but as far as I can tell a resubmit will happen, exactly if
> HID_OUT_RUNNING is _not_ cleared.

In fact, it should be a double unlink bug, usb_unlink_urb can handle
it correctly
if the lock is held. We also can deal with it easily by checking urb->unlinked,
so how about the below patch?

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index aa1c503..b530463 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -411,10 +411,10 @@ static void hid_irq_out(struct urb *urb)
 {
 	struct hid_device *hid = urb->context;
 	struct usbhid_device *usbhid = hid->driver_data;
-	unsigned long flags;
+	unsigned long status = urb->status;
 	int unplug = 0;

-	switch (urb->status) {
+	switch (status) {
 	case 0:			/* success */
 		break;
 	case -ESHUTDOWN:	/* unplug */
@@ -428,8 +428,9 @@ static void hid_irq_out(struct urb *urb)
 		hid_warn(urb->dev, "output irq status %d received\n",
 			 urb->status);
 	}
-
-	spin_lock_irqsave(&usbhid->lock, flags);
+	if (status != -ECONNRESET)
+		spin_lock(&usbhid->unlink_lock);
+	spin_lock(&usbhid->lock);

 	if (unplug)
 		usbhid->outtail = usbhid->outhead;
@@ -438,12 +439,16 @@ 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);
+		spin_unlock(&usbhid->lock);
+		if (status != -ECONNRESET)
+			spin_unlock(&usbhid->unlink_lock);
 		return;
 	}

 	clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
-	spin_unlock_irqrestore(&usbhid->lock, flags);
+	spin_unlock(&usbhid->lock);
+	if (status != -ECONNRESET)
+		spin_unlock(&usbhid->unlink_lock);
 	usb_autopm_put_interface_async(usbhid->intf);
 	wake_up(&usbhid->wait);
 }
@@ -458,6 +463,8 @@ static void hid_ctrl(struct urb *urb)
 	struct usbhid_device *usbhid = hid->driver_data;
 	int unplug = 0, status = urb->status;

+	if (status != -ECONNRESET)
+		spin_lock(&usbhid->unlink_lock);
 	spin_lock(&usbhid->lock);

 	switch (status) {
@@ -487,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 */
 		spin_unlock(&usbhid->lock);
+		if (status != -ECONNRESET)
+			spin_unlock(&usbhid->unlink_lock);
 		return;
 	}

 	clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
 	spin_unlock(&usbhid->lock);
+	if (status != -ECONNRESET)
+		spin_unlock(&usbhid->unlink_lock);
 	usb_autopm_put_interface_async(usbhid->intf);
 	wake_up(&usbhid->wait);
 }
@@ -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;
 	}
@@ -594,8 +610,12 @@ 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_ctrl + HZ * 5))
+		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5) &&
+				!usbhid->urbctrl->unlinked) {
+			spin_unlock(&usbhid->lock);
 			usb_unlink_urb(usbhid->urbctrl);
+			spin_lock(&usbhid->lock);
+		}
 	}
 }

@@ -604,9 +624,11 @@ void usbhid_submit_report(struct hid_device *hid,
struct hid_report *report, uns
 	struct usbhid_device *usbhid = hid->driver_data;
 	unsigned long flags;

-	spin_lock_irqsave(&usbhid->lock, flags);
+	spin_lock_irqsave(&usbhid->unlink_lock, flags);
+	spin_lock(&usbhid->lock);
 	__usbhid_submit_report(hid, report, dir);
-	spin_unlock_irqrestore(&usbhid->lock, flags);
+	spin_unlock(&usbhid->lock);
+	spin_unlock_irqrestore(&usbhid->unlink_lock, flags);
 }
 EXPORT_SYMBOL_GPL(usbhid_submit_report);

@@ -625,13 +647,15 @@ static void hid_led(struct work_struct *work)
 		return;
 	}

-	spin_lock_irqsave(&usbhid->lock, flags);
+	spin_lock_irqsave(&usbhid->unlink_lock, flags);
+	spin_lock(&usbhid->lock);
 	if (!test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
 		usbhid->ledcount = hidinput_count_leds(hid);
 		hid_dbg(usbhid->hid, "New ledcount = %u\n", usbhid->ledcount);
 		__usbhid_submit_report(hid, field->report, USB_DIR_OUT);
 	}
-	spin_unlock_irqrestore(&usbhid->lock, flags);
+	spin_unlock(&usbhid->lock);
+	spin_unlock_irqrestore(&usbhid->unlink_lock, flags);
 }

 static int usb_hidinput_input_event(struct input_dev *dev, unsigned
int type, unsigned int code, int value)
@@ -1299,6 +1323,7 @@ static int usbhid_probe(struct usb_interface
*intf, const struct usb_device_id *
 	INIT_WORK(&usbhid->reset_work, hid_reset);
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
 	spin_lock_init(&usbhid->lock);
+	spin_lock_init(&usbhid->unlink_lock);

 	INIT_WORK(&usbhid->led_work, hid_led);

diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 1883d7b..69af387 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -90,6 +90,7 @@ struct usbhid_device {
 	unsigned long last_out;							/* record of last output for timeouts */

 	spinlock_t lock;						/* fifo spinlock */
+	spinlock_t unlink_lock;						/* unlink spinlock */
 	unsigned long iofl;                                             /*
I/O flags (CTRL_RUNNING, OUT_RUNNING) */
 	struct timer_list io_retry;                                     /*
Retry timer */
 	unsigned long stop_retry;                                       /*
Time to give up, in jiffies */



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

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

[Oliver Neukum]

Am Mittwoch, 25. April 2012, 03:27:19 schrieb Ming Lei:
> On Wed, Apr 25, 2012 at 2:57 AM, Oliver Neukum <oliver@neukum.org> wrote:
> 
>  usb_submit_urb()
> >>
> >> This submit won't happen because HID_OUT_RUNNING is not cleared.
> >
> > I may be dense, but as far as I can tell a resubmit will happen, exactly if
> > HID_OUT_RUNNING is _not_ cleared.
> 
> In fact, it should be a double unlink bug, usb_unlink_urb can handle
> it correctly
> if the lock is held. We also can deal with it easily by checking urb->unlinked,
> so how about the below patch?
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index aa1c503..b530463 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -411,10 +411,10 @@ static void hid_irq_out(struct urb *urb)
>  {
>  	struct hid_device *hid = urb->context;
>  	struct usbhid_device *usbhid = hid->driver_data;
> -	unsigned long flags;
> +	unsigned long status = urb->status;

Error codes are negative.


> @@ -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().
We'd be back at the original proposal.

	Regards
		Oliver

-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

Am Mittwoch, 25. April 2012, 08:19:32 schrieb Oliver Neukum:
> Am Mittwoch, 25. April 2012, 03:27:19 schrieb Ming Lei:

> > @@ -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().
> We'd be back at the original proposal.

In fact, now that I think about it, we could solve this with splitting
up usb_poison_urb(). We need to increase urb->reject under the
lock and then drop the lock. The only problem is double timeout.

	Regards
		Oliver
 

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

[Ming Lei]

On Wed, Apr 25, 2012 at 2:19 PM, Oliver Neukum <oneukum@suse.de> 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?

> 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.

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

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

[Oliver Neukum]

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

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 2306 bytes --]

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?  
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.

Alan Stern

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

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

[Ming Lei]

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

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

[Oliver Neukum]

Am Montag, 23. April 2012, 17:42:11 schrieb Alan Stern:
> I don't like the idea of changing the status codes.  It would mean 
> changing usb_kill_urb too.

Why?

> 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.
> 
> This is a better approach, in that it doesn't make any assumptions 
> regarding synchronous vs. asynchronous unlinks.  If you want, you could 
> have two different ACTIVE substates, one for URBs which haven't yet 
> been unlinked and one for URBs which have been.  Then you could avoid 
> unlinking the same URB twice.

That would work, provided we extend the status machine by an error
code when resubmission is not desired and check for UNLINK_STARTED
also when a timeout begins. But I wouldn't call that a simple solution.

	Regards
		Oliver

-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 
--
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

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

[Alan Stern]

On Tue, 24 Apr 2012, Oliver Neukum wrote:

> Am Montag, 23. April 2012, 17:42:11 schrieb Alan Stern:
> > I don't like the idea of changing the status codes.  It would mean 
> > changing usb_kill_urb too.
> 
> Why?

Okay, maybe it wouldn't.  If usb_unlink_urb returned -ENOENT as the 
status for synchronous givebacks then they could be treated the same as 
usb_kill_urb's synchronous completions.

But it certainly would mean changing every HCD.

> > 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.
> > 
> > This is a better approach, in that it doesn't make any assumptions 
> > regarding synchronous vs. asynchronous unlinks.  If you want, you could 
> > have two different ACTIVE substates, one for URBs which haven't yet 
> > been unlinked and one for URBs which have been.  Then you could avoid 
> > unlinking the same URB twice.
> 
> That would work, provided we extend the status machine by an error
> code when resubmission is not desired

Presumably there would be a "resubmit" function which could be called
by both the completion handler and the unlink handler.  It should be
able to tell if a resubmission was not desired, with no need for extra
state information.

How does the current driver decide whether to resubmit?

> and check for UNLINK_STARTED
> also when a timeout begins.

That was part of my description above: "If the state is not equal to
ACTIVE, drop the lock and return immediately (the URB is being unlinked
concurrently)."

> But I wouldn't call that a simple solution.

Well, that's a matter of opinion.  It does have the advantage, unlike 
lots of other proposals in this email thread, of being a _correct_ 
solution.

Alan Stern

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

[Oliver Neukum]

Am Dienstag, 24. April 2012, 17:10:04 schrieb Alan Stern:
> > and check for UNLINK_STARTED
> > also when a timeout begins.
> 
> That was part of my description above: "If the state is not equal to
> ACTIVE, drop the lock and return immediately (the URB is being unlinked
> concurrently)."
> 
> > But I wouldn't call that a simple solution.
> 
> Well, that's a matter of opinion.  It does have the advantage, unlike 
> lots of other proposals in this email thread, of being a correct 
> solution.

Indeed. Upon further thought it seems to me that we need to prevent
resubmission and all is well. And we almost have all the infrastructure.

So how about this, making it more reusable:

>From ce77d793252d1abf3a0ec6c67e4a7a05ac6d846a Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver@neukum.org>
Date: Wed, 25 Apr 2012 09:54:00 +0200
Subject: [PATCH] usbhid: prevent deadlock during timeout

On some HCDs usb_unlink_urb() can directly call the
completion handler. That limits the spinlocks that can
be taken in the handler to locks not held while calling
usb_unlink_urb()
To prevent a race with resubmission, this patch exposes
usbcore's infrastructure for blocking submission, uses it
and so drops the lock without causing a race in usbhid.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/hid/usbhid/hid-core.c |   38 ++++++++++++++++++++++++++++++++++----
 drivers/usb/core/urb.c        |   30 ++++++++++++++++++++++++++++++
 include/linux/usb.h           |    2 ++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 5bf91db..763257d 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -535,11 +535,27 @@ 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
+			 * no race because the URB is blocked under
 			 * spinlock
 			 */
-			if (time_after(jiffies, usbhid->last_out + HZ * 5))
+			if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
+				usb_block_urb(usbhid->urbout);
+				/* drop lock to not deadlock if the callback is called */
+				spin_unlock(&usbhid->lock);
 				usb_unlink_urb(usbhid->urbout);
+				spin_lock(&usbhid->lock);
+				usb_unblock_urb(usbhid->urbout);
+				/*
+				 * if the unlinking has already completed
+				 * the pump will have been stopped
+				 * it must be restarted now
+				 */
+				if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
+					if (!hid_submit_out(hid))
+						set_bit(HID_OUT_RUNNING, &usbhid->iofl);
+					
+
+			}
 		}
 		return;
 	}
@@ -583,11 +599,25 @@ 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
+		 * no race because the URB is blocked under
 		 * spinlock
 		 */
-		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5))
+		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
+			usb_block_urb(usbhid->urbctrl);
+			/* drop lock to not deadlock if the callback is called */
+			spin_unlock(&usbhid->lock);
 			usb_unlink_urb(usbhid->urbctrl);
+			spin_lock(&usbhid->lock);
+			usb_unblock_urb(usbhid->urbctrl);
+			/*
+			 * if the unlinking has already completed
+			 * the pump will have been stopped
+			 * it must be restarted now
+			 */
+			if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
+				if (!hid_submit_ctrl(hid))
+					set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
+		}
 	}
 }
 
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index cd9b3a2..1d1b010 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -681,6 +681,36 @@ void usb_unpoison_urb(struct urb *urb)
 EXPORT_SYMBOL_GPL(usb_unpoison_urb);
 
 /**
+ * usb_block_urb - reliably prevent further use of an URB
+ * @urb: pointer to URB to be blocked, may be NULL
+ *
+ * After the routine has run, attempts to resubmit the URB will fail
+ * with error -EPERM.  Thus even if the URB's completion handler always
+ * tries to resubmit, it will not succeed and the URB will become idle.
+ *
+ * The URB must not be deallocated while this routine is running.  In
+ * particular, when a driver calls this routine, it must insure that the
+ * completion handler cannot deallocate the URB.
+ */
+void usb_block_urb(struct urb *urb)
+{
+        if (!urb)
+                return;
+
+        atomic_inc(&urb->reject);
+}
+EXPORT_SYMBOL_GPL(usb_block_urb);
+
+void usb_unblock_urb(struct urb *urb)
+{
+        if (!urb)
+                return;
+
+        atomic_dec(&urb->reject);
+}
+EXPORT_SYMBOL_GPL(usb_unblock_urb);
+
+/**
  * usb_kill_anchored_urbs - cancel transfer requests en masse
  * @anchor: anchor the requests are bound to
  *
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 73b68d1..23df8ae 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1379,6 +1379,8 @@ extern int usb_unlink_urb(struct urb *urb);
 extern void usb_kill_urb(struct urb *urb);
 extern void usb_poison_urb(struct urb *urb);
 extern void usb_unpoison_urb(struct urb *urb);
+extern void usb_block_urb(struct urb *urb);
+extern void usb_unblock_urb(struct urb *urb);
 extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
 extern void usb_poison_anchored_urbs(struct usb_anchor *anchor);
 extern void usb_unpoison_anchored_urbs(struct usb_anchor *anchor);
-- 
1.7.1

	Regards
		Oliver

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

[Ming Lei]

On Wed, Apr 25, 2012 at 4:06 PM, Oliver Neukum <oneukum@suse.de> wrote:

>
> Indeed. Upon further thought it seems to me that we need to prevent
> resubmission and all is well. And we almost have all the infrastructure.
>
> So how about this, making it more reusable:

Yes, this one is good, better than the adding lock.

>
> From ce77d793252d1abf3a0ec6c67e4a7a05ac6d846a Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@neukum.org>
> Date: Wed, 25 Apr 2012 09:54:00 +0200
> Subject: [PATCH] usbhid: prevent deadlock during timeout
>
> On some HCDs usb_unlink_urb() can directly call the
> completion handler. That limits the spinlocks that can
> be taken in the handler to locks not held while calling
> usb_unlink_urb()
> To prevent a race with resubmission, this patch exposes
> usbcore's infrastructure for blocking submission, uses it
> and so drops the lock without causing a race in usbhid.
>
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> ---
>  drivers/hid/usbhid/hid-core.c |   38 ++++++++++++++++++++++++++++++++++----
>  drivers/usb/core/urb.c        |   30 ++++++++++++++++++++++++++++++
>  include/linux/usb.h           |    2 ++
>  3 files changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 5bf91db..763257d 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -535,11 +535,27 @@ 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
> +                        * no race because the URB is blocked under
>                         * spinlock
>                         */
> -                       if (time_after(jiffies, usbhid->last_out + HZ * 5))
> +                       if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
> +                               usb_block_urb(usbhid->urbout);
> +                               /* drop lock to not deadlock if the callback is called */
> +                               spin_unlock(&usbhid->lock);
>                                usb_unlink_urb(usbhid->urbout);
> +                               spin_lock(&usbhid->lock);
> +                               usb_unblock_urb(usbhid->urbout);
> +                               /*
> +                                * if the unlinking has already completed
> +                                * the pump will have been stopped
> +                                * it must be restarted now
> +                                */
> +                               if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
> +                                       if (!hid_submit_out(hid))

You need to add check of "usbhid->outtail != usbhid->outhead" above.

> +                                               set_bit(HID_OUT_RUNNING, &usbhid->iofl);
> +
> +
> +                       }
>                }
>                return;
>        }
> @@ -583,11 +599,25 @@ 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
> +                * no race because the URB is blocked under
>                 * spinlock
>                 */
> -               if (time_after(jiffies, usbhid->last_ctrl + HZ * 5))
> +               if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
> +                       usb_block_urb(usbhid->urbctrl);
> +                       /* drop lock to not deadlock if the callback is called */
> +                       spin_unlock(&usbhid->lock);
>                        usb_unlink_urb(usbhid->urbctrl);
> +                       spin_lock(&usbhid->lock);
> +                       usb_unblock_urb(usbhid->urbctrl);
> +                       /*
> +                        * if the unlinking has already completed
> +                        * the pump will have been stopped
> +                        * it must be restarted now
> +                        */
> +                       if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
> +                               if (!hid_submit_ctrl(hid))

Similar as above.

> +                                       set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
> +               }
>        }
>  }
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index cd9b3a2..1d1b010 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -681,6 +681,36 @@ void usb_unpoison_urb(struct urb *urb)
>  EXPORT_SYMBOL_GPL(usb_unpoison_urb);
>
>  /**
> + * usb_block_urb - reliably prevent further use of an URB
> + * @urb: pointer to URB to be blocked, may be NULL
> + *
> + * After the routine has run, attempts to resubmit the URB will fail
> + * with error -EPERM.  Thus even if the URB's completion handler always
> + * tries to resubmit, it will not succeed and the URB will become idle.
> + *
> + * The URB must not be deallocated while this routine is running.  In
> + * particular, when a driver calls this routine, it must insure that the
> + * completion handler cannot deallocate the URB.
> + */
> +void usb_block_urb(struct urb *urb)
> +{
> +        if (!urb)
> +                return;
> +
> +        atomic_inc(&urb->reject);
> +}
> +EXPORT_SYMBOL_GPL(usb_block_urb);
> +
> +void usb_unblock_urb(struct urb *urb)
> +{
> +        if (!urb)
> +                return;
> +
> +        atomic_dec(&urb->reject);
> +}
> +EXPORT_SYMBOL_GPL(usb_unblock_urb);
> +
> +/**
>  * usb_kill_anchored_urbs - cancel transfer requests en masse
>  * @anchor: anchor the requests are bound to
>  *
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 73b68d1..23df8ae 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1379,6 +1379,8 @@ extern int usb_unlink_urb(struct urb *urb);
>  extern void usb_kill_urb(struct urb *urb);
>  extern void usb_poison_urb(struct urb *urb);
>  extern void usb_unpoison_urb(struct urb *urb);
> +extern void usb_block_urb(struct urb *urb);
> +extern void usb_unblock_urb(struct urb *urb);
>  extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
>  extern void usb_poison_anchored_urbs(struct usb_anchor *anchor);
>  extern void usb_unpoison_anchored_urbs(struct usb_anchor *anchor);


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

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

[Oliver Neukum]

Am Mittwoch, 25. April 2012, 11:14:19 schrieb Ming Lei:
> > -                       if (time_after(jiffies, usbhid->last_out + HZ * 5))
> > +                       if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
> > +                               usb_block_urb(usbhid->urbout);
> > +                               /* drop lock to not deadlock if the callback is called */
> > +                               spin_unlock(&usbhid->lock);
> >                                usb_unlink_urb(usbhid->urbout);
> > +                               spin_lock(&usbhid->lock);
> > +                               usb_unblock_urb(usbhid->urbout);
> > +                               /*
> > +                                * if the unlinking has already completed
> > +                                * the pump will have been stopped
> > +                                * it must be restarted now
> > +                                */
> > +                               if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
> > +                                       if (!hid_submit_out(hid))
> 
> You need to add check of "usbhid->outtail != usbhid->outhead" above.

Done. Could you test?

	Regards
		Oliver
>From 9ff6b78dc59c98b9844dc9922549fd338828a889 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
Date: Wed, 25 Apr 2012 12:50:37 +0200
Subject: [PATCH] usbhid: prevent deadlock during timeout

On some HCDs usb_unlink_urb() can directly call the
completion handler. That limits the spinlocks that can
be taken in the handler to locks not held while calling
usb_unlink_urb()
To prevent a race with resubmission, this patch exposes
usbcore's infrastructure for blocking submission, uses it
and so drops the lock without causing a race in usbhid.

Signed-off-by: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
---
 drivers/hid/usbhid/hid-core.c |   61 +++++++++++++++++++++++++++++++++++++----
 drivers/usb/core/urb.c        |   30 ++++++++++++++++++++
 include/linux/usb.h           |    2 +
 3 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 5bf91db..c8eab8d 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -399,6 +399,16 @@ static int hid_submit_ctrl(struct hid_device *hid)
  * Output interrupt completion handler.
  */
 
+static int irq_out_pump_restart(struct hid_device *hid)
+{
+	struct usbhid_device *usbhid = hid->driver_data;
+
+	if (usbhid->outhead != usbhid->outtail)
+		return hid_submit_out(hid);
+	else
+		return -1;
+}
+
 static void hid_irq_out(struct urb *urb)
 {
 	struct hid_device *hid = urb->context;
@@ -428,7 +438,7 @@ static void hid_irq_out(struct urb *urb)
 	else
 		usbhid->outtail = (usbhid->outtail + 1) & (HID_OUTPUT_FIFO_SIZE - 1);
 
-	if (usbhid->outhead != usbhid->outtail && !hid_submit_out(hid)) {
+	if (!irq_out_pump_restart(hid)) {
 		/* Successfully submitted next urb in queue */
 		spin_unlock_irqrestore(&usbhid->lock, flags);
 		return;
@@ -443,6 +453,15 @@ static void hid_irq_out(struct urb *urb)
 /*
  * Control pipe completion handler.
  */
+static int ctrl_pump_restart(struct hid_device *hid)
+{
+	struct usbhid_device *usbhid = hid->driver_data;
+
+        if (usbhid->ctrlhead != usbhid->ctrltail)
+		return hid_submit_ctrl(hid);
+	else
+		return -1;
+}
 
 static void hid_ctrl(struct urb *urb)
 {
@@ -476,7 +495,7 @@ static void hid_ctrl(struct urb *urb)
 	else
 		usbhid->ctrltail = (usbhid->ctrltail + 1) & (HID_CONTROL_FIFO_SIZE - 1);
 
-	if (usbhid->ctrlhead != usbhid->ctrltail && !hid_submit_ctrl(hid)) {
+	if (!ctrl_pump_restart(hid)) {
 		/* Successfully submitted next urb in queue */
 		spin_unlock(&usbhid->lock);
 		return;
@@ -535,11 +554,27 @@ 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
+			 * no race because the URB is blocked under
 			 * spinlock
 			 */
-			if (time_after(jiffies, usbhid->last_out + HZ * 5))
+			if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
+				usb_block_urb(usbhid->urbout);
+				/* drop lock to not deadlock if the callback is called */
+				spin_unlock(&usbhid->lock);
 				usb_unlink_urb(usbhid->urbout);
+				spin_lock(&usbhid->lock);
+				usb_unblock_urb(usbhid->urbout);
+				/*
+				 * if the unlinking has already completed
+				 * the pump will have been stopped
+				 * it must be restarted now
+				 */
+				if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
+					if (!irq_out_pump_restart(hid))
+						set_bit(HID_OUT_RUNNING, &usbhid->iofl);
+					
+
+			}
 		}
 		return;
 	}
@@ -583,11 +618,25 @@ 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
+		 * no race because the URB is blocked under
 		 * spinlock
 		 */
-		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5))
+		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
+			usb_block_urb(usbhid->urbctrl);
+			/* drop lock to not deadlock if the callback is called */
+			spin_unlock(&usbhid->lock);
 			usb_unlink_urb(usbhid->urbctrl);
+			spin_lock(&usbhid->lock);
+			usb_unblock_urb(usbhid->urbctrl);
+			/*
+			 * if the unlinking has already completed
+			 * the pump will have been stopped
+			 * it must be restarted now
+			 */
+			if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
+				if (!ctrl_pump_restart(hid))
+					set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
+		}
 	}
 }
 
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index cd9b3a2..1d1b010 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -681,6 +681,36 @@ void usb_unpoison_urb(struct urb *urb)
 EXPORT_SYMBOL_GPL(usb_unpoison_urb);
 
 /**
+ * usb_block_urb - reliably prevent further use of an URB
+ * @urb: pointer to URB to be blocked, may be NULL
+ *
+ * After the routine has run, attempts to resubmit the URB will fail
+ * with error -EPERM.  Thus even if the URB's completion handler always
+ * tries to resubmit, it will not succeed and the URB will become idle.
+ *
+ * The URB must not be deallocated while this routine is running.  In
+ * particular, when a driver calls this routine, it must insure that the
+ * completion handler cannot deallocate the URB.
+ */
+void usb_block_urb(struct urb *urb)
+{
+        if (!urb)
+                return;
+
+        atomic_inc(&urb->reject);
+}
+EXPORT_SYMBOL_GPL(usb_block_urb);
+
+void usb_unblock_urb(struct urb *urb)
+{
+        if (!urb)
+                return;
+
+        atomic_dec(&urb->reject);
+}
+EXPORT_SYMBOL_GPL(usb_unblock_urb);
+
+/**
  * usb_kill_anchored_urbs - cancel transfer requests en masse
  * @anchor: anchor the requests are bound to
  *
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 73b68d1..23df8ae 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1379,6 +1379,8 @@ extern int usb_unlink_urb(struct urb *urb);
 extern void usb_kill_urb(struct urb *urb);
 extern void usb_poison_urb(struct urb *urb);
 extern void usb_unpoison_urb(struct urb *urb);
+extern void usb_block_urb(struct urb *urb);
+extern void usb_unblock_urb(struct urb *urb);
 extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
 extern void usb_poison_anchored_urbs(struct usb_anchor *anchor);
 extern void usb_unpoison_anchored_urbs(struct usb_anchor *anchor);
-- 
1.7.1
--
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

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

[Huajun Li]

On Wed, Apr 25, 2012 at 6:52 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Mittwoch, 25. April 2012, 11:14:19 schrieb Ming Lei:
>> > -                       if (time_after(jiffies, usbhid->last_out + HZ * 5))
>> > +                       if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
>> > +                               usb_block_urb(usbhid->urbout);
>> > +                               /* drop lock to not deadlock if the callback is called */
>> > +                               spin_unlock(&usbhid->lock);
>> >                                usb_unlink_urb(usbhid->urbout);
>> > +                               spin_lock(&usbhid->lock);
>> > +                               usb_unblock_urb(usbhid->urbout);
>> > +                               /*
>> > +                                * if the unlinking has already completed
>> > +                                * the pump will have been stopped
>> > +                                * it must be restarted now
>> > +                                */
>> > +                               if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
>> > +                                       if (!hid_submit_out(hid))
>>
>> You need to add check of "usbhid->outtail != usbhid->outhead" above.
>
> Done. Could you test?
>
>        Regards
>                Oliver
> From 9ff6b78dc59c98b9844dc9922549fd338828a889 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@neukum.org>
> Date: Wed, 25 Apr 2012 12:50:37 +0200
> Subject: [PATCH] usbhid: prevent deadlock during timeout
>
> On some HCDs usb_unlink_urb() can directly call the
> completion handler. That limits the spinlocks that can
> be taken in the handler to locks not held while calling
> usb_unlink_urb()
> To prevent a race with resubmission, this patch exposes
> usbcore's infrastructure for blocking submission, uses it
> and so drops the lock without causing a race in usbhid.
>
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> ---
>  drivers/hid/usbhid/hid-core.c |   61 +++++++++++++++++++++++++++++++++++++----
>  drivers/usb/core/urb.c        |   30 ++++++++++++++++++++
>  include/linux/usb.h           |    2 +
>  3 files changed, 87 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 5bf91db..c8eab8d 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -399,6 +399,16 @@ static int hid_submit_ctrl(struct hid_device *hid)
>  * Output interrupt completion handler.
>  */
>
> +static int irq_out_pump_restart(struct hid_device *hid)
> +{
> +       struct usbhid_device *usbhid = hid->driver_data;
> +
> +       if (usbhid->outhead != usbhid->outtail)
> +               return hid_submit_out(hid);
> +       else
> +               return -1;
> +}
> +
>  static void hid_irq_out(struct urb *urb)
>  {
>        struct hid_device *hid = urb->context;
> @@ -428,7 +438,7 @@ static void hid_irq_out(struct urb *urb)
>        else
>                usbhid->outtail = (usbhid->outtail + 1) & (HID_OUTPUT_FIFO_SIZE - 1);
>
> -       if (usbhid->outhead != usbhid->outtail && !hid_submit_out(hid)) {
> +       if (!irq_out_pump_restart(hid)) {
>                /* Successfully submitted next urb in queue */
>                spin_unlock_irqrestore(&usbhid->lock, flags);
>                return;
> @@ -443,6 +453,15 @@ static void hid_irq_out(struct urb *urb)
>  /*
>  * Control pipe completion handler.
>  */
> +static int ctrl_pump_restart(struct hid_device *hid)
> +{
> +       struct usbhid_device *usbhid = hid->driver_data;
> +
> +        if (usbhid->ctrlhead != usbhid->ctrltail)
> +               return hid_submit_ctrl(hid);
> +       else
> +               return -1;
> +}
>
>  static void hid_ctrl(struct urb *urb)
>  {
> @@ -476,7 +495,7 @@ static void hid_ctrl(struct urb *urb)
>        else
>                usbhid->ctrltail = (usbhid->ctrltail + 1) & (HID_CONTROL_FIFO_SIZE - 1);
>
> -       if (usbhid->ctrlhead != usbhid->ctrltail && !hid_submit_ctrl(hid)) {
> +       if (!ctrl_pump_restart(hid)) {
>                /* Successfully submitted next urb in queue */
>                spin_unlock(&usbhid->lock);
>                return;
> @@ -535,11 +554,27 @@ 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
> +                        * no race because the URB is blocked under
>                         * spinlock
>                         */
> -                       if (time_after(jiffies, usbhid->last_out + HZ * 5))
> +                       if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
> +                               usb_block_urb(usbhid->urbout);
> +                               /* drop lock to not deadlock if the callback is called */
> +                               spin_unlock(&usbhid->lock);
>                                usb_unlink_urb(usbhid->urbout);
> +                               spin_lock(&usbhid->lock);
> +                               usb_unblock_urb(usbhid->urbout);
> +                               /*
> +                                * if the unlinking has already completed
> +                                * the pump will have been stopped
> +                                * it must be restarted now
> +                                */
> +                               if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
> +                                       if (!irq_out_pump_restart(hid))
> +                                               set_bit(HID_OUT_RUNNING, &usbhid->iofl);
> +
> +
> +                       }
>                }
>                return;
>        }
> @@ -583,11 +618,25 @@ 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
> +                * no race because the URB is blocked under
>                 * spinlock
>                 */
> -               if (time_after(jiffies, usbhid->last_ctrl + HZ * 5))
> +               if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
> +                       usb_block_urb(usbhid->urbctrl);
> +                       /* drop lock to not deadlock if the callback is called */
> +                       spin_unlock(&usbhid->lock);
>                        usb_unlink_urb(usbhid->urbctrl);
> +                       spin_lock(&usbhid->lock);
> +                       usb_unblock_urb(usbhid->urbctrl);
> +                       /*
> +                        * if the unlinking has already completed
> +                        * the pump will have been stopped
> +                        * it must be restarted now
> +                        */
> +                       if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
> +                               if (!ctrl_pump_restart(hid))
> +                                       set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
> +               }
>        }
>  }
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index cd9b3a2..1d1b010 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -681,6 +681,36 @@ void usb_unpoison_urb(struct urb *urb)
>  EXPORT_SYMBOL_GPL(usb_unpoison_urb);
>
>  /**
> + * usb_block_urb - reliably prevent further use of an URB
> + * @urb: pointer to URB to be blocked, may be NULL
> + *
> + * After the routine has run, attempts to resubmit the URB will fail
> + * with error -EPERM.  Thus even if the URB's completion handler always
> + * tries to resubmit, it will not succeed and the URB will become idle.
> + *
> + * The URB must not be deallocated while this routine is running.  In
> + * particular, when a driver calls this routine, it must insure that the
> + * completion handler cannot deallocate the URB.
> + */
> +void usb_block_urb(struct urb *urb)
> +{
> +        if (!urb)
> +                return;
> +
> +        atomic_inc(&urb->reject);
> +}
> +EXPORT_SYMBOL_GPL(usb_block_urb);
> +
> +void usb_unblock_urb(struct urb *urb)
> +{
> +        if (!urb)
> +                return;
> +
> +        atomic_dec(&urb->reject);
> +}
> +EXPORT_SYMBOL_GPL(usb_unblock_urb);
> +

Hi,
  So far, usb_unblock_urb() does same thing as usb_unpoison_urb(), so
is it possible reusing it just by adding a macro define?

> +/**
>  * usb_kill_anchored_urbs - cancel transfer requests en masse
>  * @anchor: anchor the requests are bound to
>  *
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 73b68d1..23df8ae 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1379,6 +1379,8 @@ extern int usb_unlink_urb(struct urb *urb);
>  extern void usb_kill_urb(struct urb *urb);
>  extern void usb_poison_urb(struct urb *urb);
>  extern void usb_unpoison_urb(struct urb *urb);
> +extern void usb_block_urb(struct urb *urb);
> +extern void usb_unblock_urb(struct urb *urb);
>  extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
>  extern void usb_poison_anchored_urbs(struct usb_anchor *anchor);
>  extern void usb_unpoison_anchored_urbs(struct usb_anchor *anchor);
> --
> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Oliver Neukum]

Am Mittwoch, 25. April 2012, 13:24:14 schrieb Huajun Li:
> Hi,
>   So far, usb_unblock_urb() does same thing as usb_unpoison_urb(), so
> is it possible reusing it just by adding a macro define?
> 

A patch is welcome.

	Regards
		Oliver

-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 
--
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

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

[Ming Lei]

On Wed, Apr 25, 2012 at 6:52 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Mittwoch, 25. April 2012, 11:14:19 schrieb Ming Lei:
>> You need to add check of "usbhid->outtail != usbhid->outhead" above.
>
> Done. Could you test?

Basically my USB keyboard can work well as before with the patch, but the
timeout is not covered since it can't be triggered in my box.

>
>        Regards
>                Oliver
> From 9ff6b78dc59c98b9844dc9922549fd338828a889 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@neukum.org>
> Date: Wed, 25 Apr 2012 12:50:37 +0200
> Subject: [PATCH] usbhid: prevent deadlock during timeout
>
> On some HCDs usb_unlink_urb() can directly call the
> completion handler. That limits the spinlocks that can
> be taken in the handler to locks not held while calling
> usb_unlink_urb
> To prevent a race with resubmission, this patch exposes
> usbcore's infrastructure for blocking submission, uses it
> and so drops the lock without causing a race in usbhid.
>
> Signed-off-by: Oliver Neukum <oneukum@suse.de>

Reported-by: Ming Lei <ming.lei@canonical.com>

> ---
>  drivers/hid/usbhid/hid-core.c |   61 +++++++++++++++++++++++++++++++++++++----
>  drivers/usb/core/urb.c        |   30 ++++++++++++++++++++
>  include/linux/usb.h           |    2 +
>  3 files changed, 87 insertions(+), 6 deletions(-)
>


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

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

[Alan Stern]

On Wed, 25 Apr 2012, Oliver Neukum wrote:

> From 9ff6b78dc59c98b9844dc9922549fd338828a889 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org>
> Date: Wed, 25 Apr 2012 12:50:37 +0200
> Subject: [PATCH] usbhid: prevent deadlock during timeout
> 
> On some HCDs usb_unlink_urb() can directly call the
> completion handler. That limits the spinlocks that can
> be taken in the handler to locks not held while calling
> usb_unlink_urb()
> To prevent a race with resubmission, this patch exposes
> usbcore's infrastructure for blocking submission, uses it
> and so drops the lock without causing a race in usbhid.

Simply preventing resubmission is a good idea.

> Signed-off-by: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/hid/usbhid/hid-core.c |   61 +++++++++++++++++++++++++++++++++++++----
>  drivers/usb/core/urb.c        |   30 ++++++++++++++++++++
>  include/linux/usb.h           |    2 +
>  3 files changed, 87 insertions(+), 6 deletions(-)

This should be split into two patches: one for usbhid and one for 
usbcore.

> +void usb_block_urb(struct urb *urb)
> +{
> +        if (!urb)
> +                return;
> +
> +        atomic_inc(&urb->reject);
> +}
> +EXPORT_SYMBOL_GPL(usb_block_urb);

Personally, I prefer

	if (urb)
		atomic_inc(&urb->reject);

It's a matter of taste; do what you want.

> +void usb_unblock_urb(struct urb *urb)
> +{
> +        if (!urb)
> +                return;
> +
> +        atomic_dec(&urb->reject);
> +}
> +EXPORT_SYMBOL_GPL(usb_unblock_urb);

As was pointed out, this could be eliminated by...

> +
> +/**
>   * usb_kill_anchored_urbs - cancel transfer requests en masse
>   * @anchor: anchor the requests are bound to
>   *
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 73b68d1..23df8ae 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1379,6 +1379,8 @@ extern int usb_unlink_urb(struct urb *urb);
>  extern void usb_kill_urb(struct urb *urb);
>  extern void usb_poison_urb(struct urb *urb);
>  extern void usb_unpoison_urb(struct urb *urb);
> +extern void usb_block_urb(struct urb *urb);
> +extern void usb_unblock_urb(struct urb *urb);

changing this to

#define usb_unblock_urb usb_unpoison_urb

Alan Stern

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

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

[Jiri Kosina]

On Wed, 25 Apr 2012, Alan Stern wrote:

> On Wed, 25 Apr 2012, Oliver Neukum wrote:
> 
> > From 9ff6b78dc59c98b9844dc9922549fd338828a889 Mon Sep 17 00:00:00 2001
> > From: Oliver Neukum <oliver@neukum.org>
> > Date: Wed, 25 Apr 2012 12:50:37 +0200
> > Subject: [PATCH] usbhid: prevent deadlock during timeout
> > 
> > On some HCDs usb_unlink_urb() can directly call the
> > completion handler. That limits the spinlocks that can
> > be taken in the handler to locks not held while calling
> > usb_unlink_urb()
> > To prevent a race with resubmission, this patch exposes
> > usbcore's infrastructure for blocking submission, uses it
> > and so drops the lock without causing a race in usbhid.
> 
> Simply preventing resubmission is a good idea.
> 
> > Signed-off-by: Oliver Neukum <oneukum@suse.de>
> > ---
> >  drivers/hid/usbhid/hid-core.c |   61 +++++++++++++++++++++++++++++++++++++----
> >  drivers/usb/core/urb.c        |   30 ++++++++++++++++++++
> >  include/linux/usb.h           |    2 +
> >  3 files changed, 87 insertions(+), 6 deletions(-)
> 
> This should be split into two patches: one for usbhid and one for 
> usbcore.

Alternatively you can use my

	Acked-by: Jiri Kosina <jkosina@suse.cz>

for the usbhid part and apply through USB tree.

Thanks everybody,

-- 
Jiri Kosina
SUSE Labs

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

[Greg Kroah-Hartman]

On Fri, Apr 27, 2012 at 12:44:57AM +0200, Jiri Kosina wrote:
> On Wed, 25 Apr 2012, Alan Stern wrote:
> 
> > On Wed, 25 Apr 2012, Oliver Neukum wrote:
> > 
> > > From 9ff6b78dc59c98b9844dc9922549fd338828a889 Mon Sep 17 00:00:00 2001
> > > From: Oliver Neukum <oliver@neukum.org>
> > > Date: Wed, 25 Apr 2012 12:50:37 +0200
> > > Subject: [PATCH] usbhid: prevent deadlock during timeout
> > > 
> > > On some HCDs usb_unlink_urb() can directly call the
> > > completion handler. That limits the spinlocks that can
> > > be taken in the handler to locks not held while calling
> > > usb_unlink_urb()
> > > To prevent a race with resubmission, this patch exposes
> > > usbcore's infrastructure for blocking submission, uses it
> > > and so drops the lock without causing a race in usbhid.
> > 
> > Simply preventing resubmission is a good idea.
> > 
> > > Signed-off-by: Oliver Neukum <oneukum@suse.de>
> > > ---
> > >  drivers/hid/usbhid/hid-core.c |   61 +++++++++++++++++++++++++++++++++++++----
> > >  drivers/usb/core/urb.c        |   30 ++++++++++++++++++++
> > >  include/linux/usb.h           |    2 +
> > >  3 files changed, 87 insertions(+), 6 deletions(-)
> > 
> > This should be split into two patches: one for usbhid and one for 
> > usbcore.
> 
> Alternatively you can use my
> 
> 	Acked-by: Jiri Kosina <jkosina@suse.cz>
> 
> for the usbhid part and apply through USB tree.
> 
> Thanks everybody,

Wait, was this really a valid solution here?  If so, can someone bounce
this to me...

thanks,

greg k-h

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

[Oliver Neukum]

Am Sonntag, 22. April 2012, 12:51:26 schrieb Ming Lei:
> On Sun, Apr 22, 2012 at 5:49 AM, Alan Stern <stern@rowland.harvard.edu> wrote:

> > Although the kerneldoc doesn't actually say so, it should be safe to
> > assume that usb_unlink_urb calls the completion routine directly _only_
> > in cases where the unlink succeeded.  (We could add this to the
> > kerneldoc.)
> >
> > Therefore: If the URB completes with status other than -ECONNRESET then
> > you can safely take the lock for resubmission.  If the URB completes
> > with status == -ECONNRESET then you know it was unlinked, so you don't
> > need to take the lock -- the race has already been lost.
> >
> > Does that solve your problem?
> 
> Not sure if that does work.

I am afraid it does not work.

> If the URB completes asynchronously after unlinking, its status is still
>  -ECONNRESET, so extra race may be caused without holding the lock
> because complete handler will access some global data.

That is the race. And you need not invoke global data. The original
race opens again if you are submitting a new URB without the lock
held.
This is because we cannot be sure that the same URB is unlinked
only once. A subsequent timeout may kill the wrong URB if the
first is unlinked so that the callback really comes in interrupt.

But the basic idea is brilliant. It's just that the one way logical implication:
recursive direct call of the callback -> status == -ECONNRESET
is not strong enough. But that is very easy to fix. As we know whether
the callback is directly called or not, all we need to do is differentiate
the cases in urb->status, by introducing a new error code.

	Regards
		Oliver

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

[Ming Lei]

On Sun, Apr 22, 2012 at 1:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 21 Apr 2012, Ming Lei wrote:
>> How about always scheduling a tasklet to run what usb_unlink_urb does?
>> just implement usb_unlink_urb as something like
>> tasklet_schedule(unlink_tasklet).
>>
>> Then we can have a uniform lock requirement and no changes are involved
>> on host controller drivers.
>
> The return values would not be correct.

If you run 'git grep -n usb_unlink_urb drivers/usb/', it may show that
most of callers do not check its return value, and the others only check
for dumping warnings. If usb_unlink_urb is converted into tasklet
implementation, we still can dump these warnings inside its tasklet function.

>
> On the other hand, usbnet could call usb_unlink_urb from within a
> tasklet.

Sorry, you mean tasklet_schedule can't be called inside a tasklet?

Thanks,
--
Ming Lei

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

[Alan Stern]

On Sun, 22 Apr 2012, Ming Lei wrote:

> On Sun, Apr 22, 2012 at 1:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Sat, 21 Apr 2012, Ming Lei wrote:
> >> How about always scheduling a tasklet to run what usb_unlink_urb does?
> >> just implement usb_unlink_urb as something like
> >> tasklet_schedule(unlink_tasklet).
> >>
> >> Then we can have a uniform lock requirement and no changes are involved
> >> on host controller drivers.
> >
> > The return values would not be correct.
> 
> If you run 'git grep -n usb_unlink_urb drivers/usb/', it may show that
> most of callers do not check its return value, and the others only check
> for dumping warnings. If usb_unlink_urb is converted into tasklet
> implementation, we still can dump these warnings inside its tasklet function.

That sounds rather awkward.  How would the "tasklet-ized" version of 
usb_unlink_urb know what warnings to issue?

> > On the other hand, usbnet could call usb_unlink_urb from within a
> > tasklet.
> 
> Sorry, you mean tasklet_schedule can't be called inside a tasklet?

What I meant is: If you're going to run in a tasklet, it doesn't matter
whether the tasklet is started by the usb_unlink_urb function or by its
caller.  The end result should be the same either way.

However Oliver has already objected to using a tasklet for unlinking.

Alan Stern

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

[Oliver Neukum]

Am Sonntag, 22. April 2012, 13:53:32 schrieb Ming Lei:
> >> Then we can have a uniform lock requirement and no changes are involved
> >> on host controller drivers.
> >
> > The return values would not be correct.
> 
> If you run 'git grep -n usb_unlink_urb drivers/usb/', it may show that
> most of callers do not check its return value, and the others only check
> for dumping warnings. If usb_unlink_urb is converted into tasklet
> implementation, we still can dump these warnings inside its tasklet function.

That is a very bad idea. You make implementing a proper reference count next
to impossible this way. And it makes no sense. If you use a work queue you
can use usb_kill_urb() anyway.

	Regards
		Oliver

-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 
--
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

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

[Dmitry Torokhov]

On Friday, April 20, 2012 06:17:51 PM Ming Lei wrote:
> On Fri, Apr 20, 2012 at 3:57 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> 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?

<skip>

Why don't you do something like this:

	urb_to_unlink = usbhid->urbout;
        usbhid->urbout = NULL;

        spin_unlock(&usbhid->lock);
        usb_unlink_urb(urb_to_unlink);
        spin_lock(&usbhid->lock);

and of course comment it properly.

Thanks.

-- 
Dmitry
--
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

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

[Ming Lei]

On Sat, Apr 21, 2012 at 5:59 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Friday, April 20, 2012 06:17:51 PM Ming Lei wrote:

> Why don't you do something like this:
>
>        urb_to_unlink = usbhid->urbout;
>        usbhid->urbout = NULL;

This may trigger oops in hid_submit_out called by hid_irq_out.

Even though you can check if usbhid->urbout is NULL inside
hid_submit_out, and just not submit it if it is NULL, then the solution
become similar with Oliver's idea, and the problem is that when the
usbhid->urbout will be resubmitted, which looks may involve much
more changes than the per cpu flag patch.

The difficulty is in the race between unlink with complete handler(irq),
both may run concurrently on different CPUs.

>
>        spin_unlock(&usbhid->lock);
>        usb_unlink_urb(urb_to_unlink);
>        spin_lock(&usbhid->lock);
>
> and of course comment it properly.
>
> Thanks.
>
> --
> Dmitry



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