Patch for bizzare oops in USB

Patch for bizzare oops in USB

[Pete Zaitcev]

I ran webcam(1) with ov511 and if I hit ^C, the box oopses.
Apparently, the following happens:

1. On SIGINT, v4l closes ov511, which isses a string of
   control requests to quescent the cam.
2. One of those requests enters usb_internal_control_msg
   where it submits the URB and does schedule_timeout().
3. Since the signal is pending [sic], it does not wait,
   but spins testing urb->status.
4. The interrupt is taken on other CPU and it gets into
   sohci_return_urb, then clears status and calls urb_rm_priv.
5. The user thread sees that status becomes zero and *frees the URB*.
6. The urb_rm_priv takes a spinlock and does its dirty buseness.
7. User thread reallocates the URB and resubmits it,
   waiting on the spinlock meanwhile.
8. urb_rm_priv zaps urb->dev in the URB which was already
   freed and reallocated and releases the spinlock.
9. The user thread keels over deep inside td_submit_urb()
   dereferencing urb->dev->something

Took me a couple of days to figure it all out. :)

diff -ur -X dontdiff linux-2.4.8/drivers/usb/usb.c linux-2.4.8-e/drivers/usb/usb.c
--- linux-2.4.8/drivers/usb/usb.c	Tue Jul 24 14:20:56 2001
+++ linux-2.4.8-e/drivers/usb/usb.c	Fri Aug 17 22:03:27 2001
@@ -1066,7 +1066,7 @@
   
 	awd.wakeup = &wqh;
 	init_waitqueue_head(&wqh); 	
-	current->state = TASK_INTERRUPTIBLE;
+	current->state = TASK_UNINTERRUPTIBLE;	/* MUST BE SO. -- zaitcev */
 	add_wait_queue(&wqh, &wait);
 	urb->context = &awd;
 	status = usb_submit_urb(urb);

Re: Patch for bizzare oops in USB

[Thomas Sailer]

Pete Zaitcev schrieb:

> diff -ur -X dontdiff linux-2.4.8/drivers/usb/usb.c linux-2.4.8-e/drivers/usb/usb.c
> --- linux-2.4.8/drivers/usb/usb.c       Tue Jul 24 14:20:56 2001
> +++ linux-2.4.8-e/drivers/usb/usb.c     Fri Aug 17 22:03:27 2001
> @@ -1066,7 +1066,7 @@
> 
>         awd.wakeup = &wqh;
>         init_waitqueue_head(&wqh);
> -       current->state = TASK_INTERRUPTIBLE;
> +       current->state = TASK_UNINTERRUPTIBLE;  /* MUST BE SO. -- zaitcev */
>         add_wait_queue(&wqh, &wait);
>         urb->context = &awd;
>         status = usb_submit_urb(urb);

This is bad for other users of usb_control_msg/usb_bulk_msg that depend on
the sleep to be interruptible. Instead of bouncing back and forth whether
those routines shall sleep interruptibly or uninterruptibly, we should either
provide two routines or a parameter that specifies whether the sleep
shall be interruptible, or create a local version of usb_control_msg
if ov511 is the only user requiring uninterruptible sleep.

Tom

Re: Patch for bizzare oops in USB

[Pete Zaitcev]

> Date: Mon, 20 Aug 2001 13:59:37 +0200
> From: Thomas Sailer <sailer@scs.ch>

> This is bad for other users of usb_control_msg/usb_bulk_msg that depend on
> the sleep to be interruptible.

Would you mind to explain how a user of usb_control_msg may
depend on the sleep being interruptible? Forgets to set a timeout?
Actually, I rethought the problem and I have a better fix,
but for an different reason entirely. A user of usb_control_msg
who knows too much about usb_control_msg still sounds fishy to me.

-- Pete

Re: Patch for bizzare oops in USB

[Thomas Sailer]

Pete Zaitcev schrieb:

> Would you mind to explain how a user of usb_control_msg may
> depend on the sleep being interruptible? Forgets to set a timeout?

You may want to be able to kill a process that waits for a control
message to complete. Especially user processes via devio.c, but
quite possibly also user apps via some kernel driver.

devio.c had its own version of usb_control_msg for that reason,
but it was felt unnecessary and removed.

Tom

Re: Patch for bizzare oops in USB

[Pete Zaitcev]

> Date: Mon, 20 Aug 2001 13:59:37 +0200
> From: Thomas Sailer <sailer@scs.ch>

> > -       current->state = TASK_INTERRUPTIBLE;
> > +       current->state = TASK_UNINTERRUPTIBLE;  /* MUST BE SO. -- zaitcev */

> This is bad for other users of usb_control_msg/usb_bulk_msg that depend on
> the sleep to be interruptible. Instead of bouncing back and forth whether
> those routines shall sleep interruptibly or uninterruptibly, we should either
> provide two routines or a parameter that specifies whether the sleep
> shall be interruptible, or create a local version of usb_control_msg
> if ov511 is the only user requiring uninterruptible sleep.

A prolifiration of subtly different versions of basic primitives
is not an answer either. But wait, here's a better fix. The
root of the evil is that the waiting thread accesses urb->status
before a callback happened, which is unsafe.

BTW, I took a liberty to clean the thing up a bit. It looked as
if the author of that fragment was not sure of what he was doing,
and the style was quite dirty. I think a number of wrongs for
such a small fragment was astonishing.

 - "status" was an errno in the begining, then 5 lines down
   it's bool (== timeout), then it turns into system style once again.
 - Wasted pointer to wait head
 - Unused void *stuff.
 - typedef without _t and unprefixed type name in global header.
 - Urban legend of test for waitqueue_active() before wakeup.
   This is one half wrong because so many people do it,
   anyone has an idea why? Half a point deducted.
 - Confused and redundant checking for -EINPROGRESS
   (even if it was the cause of oops)

And the last one ...
 - THE GOD DAMN RACE THAT OOPS - that's 10 hacker points down!
   It only goes to show that replacing interruptible_sleep_on
   with add_wait_queue/schedule/remove_wait_queue does not make
   any racy code correct automatically.

Cheesh, I am surprised _anything_ in Linux kernel works.

-- Pete

--- linux-2.4.8/include/linux/usb.h	Fri Aug 10 18:16:46 2001
+++ linux-2.4.8-e/include/linux/usb.h	Mon Aug 20 10:28:36 2001
@@ -539,13 +539,12 @@
  *                         SYNCHRONOUS CALL SUPPORT                  *
  *-------------------------------------------------------------------*/
 
-typedef struct
+struct usb_api_data
 {
-  wait_queue_head_t *wakeup;
-
-  void* stuff;
-  /* more to follow */
-} api_wrapper_data;
+	wait_queue_head_t wqh;
+	int done;
+	/* void* stuff;	*/	/* Possible extension later. */
+};
 
 /* -------------------------------------------------------------------------- */
 
--- linux-2.4.8/drivers/usb/usb.c	Tue Jul 24 14:20:56 2001
+++ linux-2.4.8-e/drivers/usb/usb.c	Mon Aug 20 14:05:45 2001
@@ -1041,15 +1041,10 @@
  *-------------------------------------------------------------------*/
 static void usb_api_blocking_completion(urb_t *urb)
 {
-	api_wrapper_data *awd = (api_wrapper_data *)urb->context;
+	struct usb_api_data *awd = (struct usb_api_data *)urb->context;
 
-	if (waitqueue_active(awd->wakeup))
-		wake_up(awd->wakeup);
-#if 0
-	else
-		dbg("(blocking_completion): waitqueue empty!"); 
-		// even occurs if urb was unlinked by timeout...
-#endif
+	awd->done = 1;
+	wake_up(&awd->wqh);
 }
 
 /*-------------------------------------------------------------------*
@@ -1060,35 +1055,32 @@
 static int usb_start_wait_urb(urb_t *urb, int timeout, int* actual_length)
 { 
 	DECLARE_WAITQUEUE(wait, current);
-	DECLARE_WAIT_QUEUE_HEAD(wqh);
-	api_wrapper_data awd;
+	struct usb_api_data awd;
 	int status;
-  
-	awd.wakeup = &wqh;
-	init_waitqueue_head(&wqh); 	
+
+	init_waitqueue_head(&awd.wqh); 	
+	awd.done = 0;
+
 	current->state = TASK_INTERRUPTIBLE;
-	add_wait_queue(&wqh, &wait);
+	add_wait_queue(&awd.wqh, &wait);
+
 	urb->context = &awd;
 	status = usb_submit_urb(urb);
 	if (status) {
 		// something went wrong
 		usb_free_urb(urb);
 		current->state = TASK_RUNNING;
-		remove_wait_queue(&wqh, &wait);
+		remove_wait_queue(&awd.wqh, &wait);
 		return status;
 	}
 
-	if (urb->status == -EINPROGRESS) {
-		while (timeout && urb->status == -EINPROGRESS)
-			status = timeout = schedule_timeout(timeout);
-	} else
-		status = 1;
+	while (timeout && !awd.done)
+		timeout = schedule_timeout(timeout);
 
 	current->state = TASK_RUNNING;
-	remove_wait_queue(&wqh, &wait);
+	remove_wait_queue(&awd.wqh, &wait);
 
-	if (!status) {
-		// timeout
+	if (!timeout) {
 		printk("usb_control/bulk_msg: timeout\n");
 		usb_unlink_urb(urb);  // remove urb safely
 		status = -ETIMEDOUT;

Re: Patch for bizzare oops in USB

[Johannes Erdfelt]

On Mon, Aug 20, 2001, Pete Zaitcev <zaitcev@redhat.com> wrote:
> A prolifiration of subtly different versions of basic primitives
> is not an answer either. But wait, here's a better fix. The
> root of the evil is that the waiting thread accesses urb->status
> before a callback happened, which is unsafe.
> 
> BTW, I took a liberty to clean the thing up a bit. It looked as
> if the author of that fragment was not sure of what he was doing,
> and the style was quite dirty. I think a number of wrongs for
> such a small fragment was astonishing.
> 
>  - "status" was an errno in the begining, then 5 lines down
>    it's bool (== timeout), then it turns into system style once again.
>  - Wasted pointer to wait head
>  - Unused void *stuff.
>  - typedef without _t and unprefixed type name in global header.
>  - Urban legend of test for waitqueue_active() before wakeup.
>    This is one half wrong because so many people do it,
>    anyone has an idea why? Half a point deducted.
>  - Confused and redundant checking for -EINPROGRESS
>    (even if it was the cause of oops)
> 
> And the last one ...
>  - THE GOD DAMN RACE THAT OOPS - that's 10 hacker points down!
>    It only goes to show that replacing interruptible_sleep_on
>    with add_wait_queue/schedule/remove_wait_queue does not make
>    any racy code correct automatically.
> 
> Cheesh, I am surprised _anything_ in Linux kernel works.

So am I. To be honest, there's a bunch of things in the USB code I'm not
entirely happy.

I didn't see many of them until recently, but I guess with experience,
comes wisdom. I had intended to fix many of them in 2.5 when it finally
forks.

I like your patch, but since we have the new completion stuff now, we
should probably use that. I'll make the mod and send off the patch to
Linus when I get back from this business trip.

Thanks.

JE

Re: Patch for bizzare oops in USB

[Pete Zaitcev]

> Date: Tue, 21 Aug 2001 00:01:26 -0400
> From: Johannes Erdfelt <johannes@erdfelt.com>

> I like your patch, but since we have the new completion stuff now, we
> should probably use that. I'll make the mod and send off the patch to
> Linus when I get back from this business trip.

OK, thanks. Meanwhile Alan picked my patch for 2.4.8-ac8
so I'll make a revert for him when you have the time and
Linus picks your fix.

-- Pete

Re: Patch for bizzare oops in USB

[Eugene Crosser]

In article <3B80FBA9.556B7B2B@scs.ch>,
        Thomas Sailer <sailer@scs.ch> writes:

>> --- linux-2.4.8/drivers/usb/usb.c       Tue Jul 24 14:20:56 2001
>> +++ linux-2.4.8-e/drivers/usb/usb.c     Fri Aug 17 22:03:27 2001
>> @@ -1066,7 +1066,7 @@
>> 
>>         awd.wakeup = &wqh;
>>         init_waitqueue_head(&wqh);
>> -       current->state = TASK_INTERRUPTIBLE;
>> +       current->state = TASK_UNINTERRUPTIBLE;  /* MUST BE SO. -- zaitcev */
>>         add_wait_queue(&wqh, &wait);
>>         urb->context = &awd;
>>         status = usb_submit_urb(urb);
> 
> This is bad for other users of usb_control_msg/usb_bulk_msg that depend on
> the sleep to be interruptible. Instead of bouncing back and forth whether
> those routines shall sleep interruptibly or uninterruptibly, we should either
> provide two routines or a parameter that specifies whether the sleep
> shall be interruptible, or create a local version of usb_control_msg
> if ov511 is the only user requiring uninterruptible sleep.

I observe similar Oops with D-Link USB radio tuner on uhci when I hit
Ctrl-C (SMP system, UP system with ohci works).  I was preparing to
post ksymoops report when I read Pete's message ;)

Eugene