* Patch for bizzare oops in USB
@ 2001-08-18 5:31 Pete Zaitcev
2001-08-20 11:59 ` Thomas Sailer
0 siblings, 1 reply; 8+ messages in thread
From: Pete Zaitcev @ 2001-08-18 5:31 UTC (permalink / raw)
To: johannes; +Cc: linux-kernel, 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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for bizzare oops in USB
2001-08-18 5:31 Patch for bizzare oops in USB Pete Zaitcev
@ 2001-08-20 11:59 ` Thomas Sailer
2001-08-20 21:06 ` Pete Zaitcev
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Thomas Sailer @ 2001-08-20 11:59 UTC (permalink / raw)
To: Pete Zaitcev; +Cc: johannes, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for bizzare oops in USB
2001-08-20 11:59 ` Thomas Sailer
@ 2001-08-20 21:06 ` Pete Zaitcev
2001-08-21 8:29 ` Thomas Sailer
2001-08-20 21:44 ` Pete Zaitcev
2001-08-20 22:12 ` Eugene Crosser
2 siblings, 1 reply; 8+ messages in thread
From: Pete Zaitcev @ 2001-08-20 21:06 UTC (permalink / raw)
To: t.sailer; +Cc: linux-kernel
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for bizzare oops in USB
2001-08-20 11:59 ` Thomas Sailer
2001-08-20 21:06 ` Pete Zaitcev
@ 2001-08-20 21:44 ` Pete Zaitcev
2001-08-21 4:01 ` Johannes Erdfelt
2001-08-20 22:12 ` Eugene Crosser
2 siblings, 1 reply; 8+ messages in thread
From: Pete Zaitcev @ 2001-08-20 21:44 UTC (permalink / raw)
To: johannes; +Cc: Pete Zaitcev, linux-kernel, t.sailer
> 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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for bizzare oops in USB
2001-08-20 11:59 ` Thomas Sailer
2001-08-20 21:06 ` Pete Zaitcev
2001-08-20 21:44 ` Pete Zaitcev
@ 2001-08-20 22:12 ` Eugene Crosser
2 siblings, 0 replies; 8+ messages in thread
From: Eugene Crosser @ 2001-08-20 22:12 UTC (permalink / raw)
To: linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for bizzare oops in USB
2001-08-20 21:44 ` Pete Zaitcev
@ 2001-08-21 4:01 ` Johannes Erdfelt
2001-08-21 4:17 ` Pete Zaitcev
0 siblings, 1 reply; 8+ messages in thread
From: Johannes Erdfelt @ 2001-08-21 4:01 UTC (permalink / raw)
To: Pete Zaitcev; +Cc: linux-kernel, t.sailer
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for bizzare oops in USB
2001-08-21 4:01 ` Johannes Erdfelt
@ 2001-08-21 4:17 ` Pete Zaitcev
0 siblings, 0 replies; 8+ messages in thread
From: Pete Zaitcev @ 2001-08-21 4:17 UTC (permalink / raw)
To: Johannes Erdfelt; +Cc: Pete Zaitcev, linux-kernel
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for bizzare oops in USB
2001-08-20 21:06 ` Pete Zaitcev
@ 2001-08-21 8:29 ` Thomas Sailer
0 siblings, 0 replies; 8+ messages in thread
From: Thomas Sailer @ 2001-08-21 8:29 UTC (permalink / raw)
To: Pete Zaitcev; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2001-08-21 8:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-08-18 5:31 Patch for bizzare oops in USB Pete Zaitcev
2001-08-20 11:59 ` Thomas Sailer
2001-08-20 21:06 ` Pete Zaitcev
2001-08-21 8:29 ` Thomas Sailer
2001-08-20 21:44 ` Pete Zaitcev
2001-08-21 4:01 ` Johannes Erdfelt
2001-08-21 4:17 ` Pete Zaitcev
2001-08-20 22:12 ` Eugene Crosser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).