[patch 3/3] hci_usb.h: fix hard-to-trigger race

[patch 3/3] hci_usb.h: fix hard-to-trigger race

[akpm]

From: Pavel Machek <pavel@ucw.cz>

If someone tries to _urb_unlink while _urb_queue_head is running, he'll see
_urb->queue == NULL and fail to do any locking.  Prevent that from happening
by strategically placed barriers.

Signed-off-by: Pavel Machek <pavel@suse.cz>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Dave Young <hidave.darkstar@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/bluetooth/hci_usb.h |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff -puN drivers/bluetooth/hci_usb.h~hci_usbh-fix-hard-to-trigger-race drivers/bluetooth/hci_usb.h
--- a/drivers/bluetooth/hci_usb.h~hci_usbh-fix-hard-to-trigger-race
+++ a/drivers/bluetooth/hci_usb.h
@@ -70,7 +70,8 @@ static inline void _urb_queue_head(struc
 {
 	unsigned long flags;
 	spin_lock_irqsave(&q->lock, flags);
-	list_add(&_urb->list, &q->head); _urb->queue = q;
+	/* _urb_unlink needs to know which spinlock to use, thus mb(). */
+	_urb->queue = q; mb(); list_add(&_urb->list, &q->head);
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 
@@ -78,19 +79,23 @@ static inline void _urb_queue_tail(struc
 {
 	unsigned long flags;
 	spin_lock_irqsave(&q->lock, flags);
-	list_add_tail(&_urb->list, &q->head); _urb->queue = q;
+	/* _urb_unlink needs to know which spinlock to use, thus mb(). */
+	_urb->queue = q; mb(); list_add_tail(&_urb->list, &q->head);
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 
 static inline void _urb_unlink(struct _urb *_urb)
 {
-	struct _urb_queue *q = _urb->queue;
+	struct _urb_queue *q;
 	unsigned long flags;
-	if (q) {
-		spin_lock_irqsave(&q->lock, flags);
-		list_del(&_urb->list); _urb->queue = NULL;
-		spin_unlock_irqrestore(&q->lock, flags);
-	}
+
+	mb();
+	q = _urb->queue;
+	/* If q is NULL, it will die at easy-to-debug NULL pointer dereference.
+	   No need to BUG(). */
+	spin_lock_irqsave(&q->lock, flags);
+	list_del(&_urb->list); _urb->queue = NULL;
+	spin_unlock_irqrestore(&q->lock, flags);
 }
 
 struct hci_usb {
_

Re: [patch 3/3] hci_usb.h: fix hard-to-trigger race

[Marcel Holtmann]

Hi Andrew,

> From: Pavel Machek <pavel@ucw.cz>
>
> If someone tries to _urb_unlink while _urb_queue_head is running,  
> he'll see
> _urb->queue == NULL and fail to do any locking.  Prevent that from  
> happening
> by strategically placed barriers.

let me repeat this, the hci_usb driver is not worth fixing. Doing our  
own URB handling is a bad idea. The btusb driver should fix all of  
this. Only exception is that it is missing all the quirks, but that  
was me being lazy.

Regards

Marcel

Re: [patch 3/3] hci_usb.h: fix hard-to-trigger race

[Andrew Morton]

On Fri, 18 Apr 2008 23:10:12 +0200
Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:

> Hi Andrew,
> 
> > From: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> >
> > If someone tries to _urb_unlink while _urb_queue_head is running,  
> > he'll see
> > _urb->queue == NULL and fail to do any locking.  Prevent that from  
> > happening
> > by strategically placed barriers.
> 
> let me repeat this, the hci_usb driver is not worth fixing. Doing our  
> own URB handling is a bad idea. The btusb driver should fix all of  
> this. Only exception is that it is missing all the quirks, but that  
> was me being lazy.
> 

ok...  But as long as the old code is buildable and installable, we should
fix bugs in it?

Re: [patch 3/3] hci_usb.h: fix hard-to-trigger race

[David Miller]

From: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Date: Fri, 18 Apr 2008 14:21:53 -0700

> On Fri, 18 Apr 2008 23:10:12 +0200
> Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> 
> > Hi Andrew,
> > 
> > > From: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> > >
> > > If someone tries to _urb_unlink while _urb_queue_head is running,  
> > > he'll see
> > > _urb->queue == NULL and fail to do any locking.  Prevent that from  
> > > happening
> > > by strategically placed barriers.
> > 
> > let me repeat this, the hci_usb driver is not worth fixing. Doing our  
> > own URB handling is a bad idea. The btusb driver should fix all of  
> > this. Only exception is that it is missing all the quirks, but that  
> > was me being lazy.
> > 
> 
> ok...  But as long as the old code is buildable and installable, we should
> fix bugs in it?

Yep.

Re: [patch 3/3] hci_usb.h: fix hard-to-trigger race

[Marcel Holtmann]

Hi Andrew,

>>> If someone tries to _urb_unlink while _urb_queue_head is running,
>>> he'll see
>>> _urb->queue == NULL and fail to do any locking.  Prevent that from
>>> happening
>>> by strategically placed barriers.
>>
>> let me repeat this, the hci_usb driver is not worth fixing. Doing our
>> own URB handling is a bad idea. The btusb driver should fix all of
>> this. Only exception is that it is missing all the quirks, but that
>> was me being lazy.
>>
>
> ok...  But as long as the old code is buildable and installable, we  
> should
> fix bugs in it?

I am okay with it and happy to accept any fixes, but to be quite  
honest, that this driver still works is in some cases pure luck. Doing  
the URB handling by ourself is really simply plain work. No excuses  
here and parts of it is my fault. I know that. Hence I started a new  
implementation from scratch.

Regards

Marcel

[patch] document hci_usb as broken

[Pavel Machek]

Hi!

>> ok...  But as long as the old code is buildable and installable, we should
>> fix bugs in it?
>
> I am okay with it and happy to accept any fixes, but to be quite honest, 
> that this driver still works is in some cases pure luck. Doing the URB 
> handling by ourself is really simply plain work. No excuses here and parts 
> of it is my fault. I know that. Hence I started a new implementation from 
> scratch.

Ok, so I guess this is good idea... (I'd prefer previous race patch to
still be applied; but driver is broken even with that fix...)

I thought about adding && BROKEN to Kconfig...

---

hci_usb is fatally broken, document it as such.

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 075598e..20d9279 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -3,13 +3,16 @@ menu "Bluetooth device drivers"
 	depends on BT
 
 config BT_HCIUSB
-	tristate "HCI USB driver"
+	tristate "HCI USB driver (dangerous, use alternate driver below)"
 	depends on USB
 	help
 	  Bluetooth HCI USB driver.
 	  This driver is required if you want to use Bluetooth devices with
 	  USB interface.
 
+	  Unfortunately, locking in this driver is fatally broken; it will
+	  corrupt memory on surprise disconnect and during resume.
+
 	  Say Y here to compile support for Bluetooth USB devices into the
 	  kernel or say M to compile it as module (hci_usb).
 
diff --git a/drivers/bluetooth/hci_usb.c b/drivers/bluetooth/hci_usb.c
index 192522e..6cc96b4 100644
--- a/drivers/bluetooth/hci_usb.c
+++ b/drivers/bluetooth/hci_usb.c
@@ -1,8 +1,22 @@
-/* 
+/*
+
+    This driver has fatally broken locking.
+
+    DO NOT USE.
+
+    See btusb.c for cleaner / shorter / actually working driver.
+
+
+
+
+
+
+
+
+ 
    HCI USB driver for Linux Bluetooth protocol stack (BlueZ)
    Copyright (C) 2000-2001 Qualcomm Incorporated
    Written 2000,2001 by Maxim Krasnyansky <maxk@qualcomm.com>
-
    Copyright (C) 2003 Maxim Krasnyansky <maxk@qualcomm.com>
 
    This program is free software; you can redistribute it and/or modify

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] document hci_usb as broken

[Marcel Holtmann]

Hi Pavel,

>>> ok...  But as long as the old code is buildable and installable,  
>>> we should
>>> fix bugs in it?
>>
>> I am okay with it and happy to accept any fixes, but to be quite  
>> honest,
>> that this driver still works is in some cases pure luck. Doing the  
>> URB
>> handling by ourself is really simply plain work. No excuses here  
>> and parts
>> of it is my fault. I know that. Hence I started a new  
>> implementation from
>> scratch.
>
> Ok, so I guess this is good idea... (I'd prefer previous race patch to
> still be applied; but driver is broken even with that fix...)
>
> I thought about adding && BROKEN to Kconfig...
>
> ---
>
> hci_usb is fatally broken, document it as such.
>
> Signed-off-by: Pavel Machek <pavel@suse.cz>
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 075598e..20d9279 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -3,13 +3,16 @@ menu "Bluetooth device drivers"
> 	depends on BT
>
> config BT_HCIUSB
> -	tristate "HCI USB driver"
> +	tristate "HCI USB driver (dangerous, use alternate driver below)"
> 	depends on USB
> 	help
> 	  Bluetooth HCI USB driver.
> 	  This driver is required if you want to use Bluetooth devices with
> 	  USB interface.
>
> +	  Unfortunately, locking in this driver is fatally broken; it will
> +	  corrupt memory on surprise disconnect and during resume.
> +
> 	  Say Y here to compile support for Bluetooth USB devices into the
> 	  kernel or say M to compile it as module (hci_usb).

this would be fine with me. Acked-by: Marcel Holtmann <marcel@holtmann.org 
 >

> diff --git a/drivers/bluetooth/hci_usb.c b/drivers/bluetooth/hci_usb.c
> index 192522e..6cc96b4 100644
> --- a/drivers/bluetooth/hci_usb.c
> +++ b/drivers/bluetooth/hci_usb.c
> @@ -1,8 +1,22 @@
> -/*
> +/*
> +
> +    This driver has fatally broken locking.
> +
> +    DO NOT USE.
> +
> +    See btusb.c for cleaner / shorter / actually working driver.

NAK to this hunk. Not needed. The Kconfig help is enough.

Regards

Marcel

Re: [patch] document hci_usb as broken

[Pavel Machek]

Hi!

>> index 075598e..20d9279 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -3,13 +3,16 @@ menu "Bluetooth device drivers"
>> 	depends on BT
>>
>> config BT_HCIUSB
>> -	tristate "HCI USB driver"
>> +	tristate "HCI USB driver (dangerous, use alternate driver below)"
>> 	depends on USB
>> 	help
>> 	  Bluetooth HCI USB driver.
>> 	  This driver is required if you want to use Bluetooth devices with
>> 	  USB interface.
>>
>> +	  Unfortunately, locking in this driver is fatally broken; it will
>> +	  corrupt memory on surprise disconnect and during resume.
>> +
>> 	  Say Y here to compile support for Bluetooth USB devices into the
>> 	  kernel or say M to compile it as module (hci_usb).
>
> this would be fine with me. Acked-by: Marcel Holtmann <marcel@holtmann.org>
>
>> diff --git a/drivers/bluetooth/hci_usb.c b/drivers/bluetooth/hci_usb.c
>> index 192522e..6cc96b4 100644
>> --- a/drivers/bluetooth/hci_usb.c
>> +++ b/drivers/bluetooth/hci_usb.c
>> @@ -1,8 +1,22 @@
>> -/*
>> +/*
>> +
>> +    This driver has fatally broken locking.
>> +
>> +    DO NOT USE.
>> +
>> +    See btusb.c for cleaner / shorter / actually working driver.
>
> NAK to this hunk. Not needed. The Kconfig help is enough.

Developers trying to debug hci_usb problems will nor see the help
text, since they probably have it enabled in their .config already and
will not be asked again...

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html