Patch for bizzare oops in USB

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
@@ -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);

In article <[email protected]>,

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

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

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

@@ -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. */
+};
 
 /* -------------------------------------------------------------------------- */
 

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

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

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

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