[patch]race condition in btusb disconnect() handling

btusb has some races in disconnect()

- it doesn't deal with only the iso interface disconnected
- it releases the data interface before unregistering the iso interface
- it isn't carefull to kill all URBs in case of soft disconnect

Signed-off-by: Oliver Neukum <redacted>

	Regards
		Oliver
---

--- linux-2.6.27-rc3/drivers/bluetooth/btusb.c.marcel	2008-08-19 10:21:51.000000000 +0200
+++ linux-2.6.27-rc3/drivers/bluetooth/btusb.c	2008-08-19 14:04:49.000000000 +0200

@@ -169,6 +169,7 @@ static struct usb_device_id blacklist_ta
 struct btusb_data {
 	struct hci_dev       *hdev;
 	struct usb_device    *udev;
+	struct usb_interface *acl;
 	struct usb_interface *isoc;
 
 	spinlock_t lock;

@@ -523,6 +524,14 @@ static int btusb_open(struct hci_dev *hd
 	return err;
 }
 
+static void btusb_stop_traffic(struct btusb_data *data)
+{
+	usb_kill_anchored_urbs(&data->intr_anchor);
+	usb_kill_anchored_urbs(&data->bulk_anchor);
+	usb_kill_anchored_urbs(&data->isoc_anchor);
+	usb_kill_anchored_urbs(&data->tx_anchor);
+}
+
 static int btusb_close(struct hci_dev *hdev)
 {
 	struct btusb_data *data = hdev->driver_data;

@@ -535,13 +544,9 @@ static int btusb_close(struct hci_dev *h
 	flush_work(&data->work);
 
 	clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
-	usb_kill_anchored_urbs(&data->intr_anchor);
-
 	clear_bit(BTUSB_BULK_RUNNING, &data->flags);
-	usb_kill_anchored_urbs(&data->bulk_anchor);
-
 	clear_bit(BTUSB_INTR_RUNNING, &data->flags);
-	usb_kill_anchored_urbs(&data->intr_anchor);
+	btusb_stop_traffic(data);
 
 	return 0;
 }

@@ -822,6 +827,7 @@ static int btusb_probe(struct usb_interf
 	}
 
 	data->udev = interface_to_usbdev(intf);
+	data->acl = intf;
 
 	spin_lock_init(&data->lock);
 

@@ -890,7 +896,7 @@ static int btusb_probe(struct usb_interf
 
 	if (data->isoc) {
 		err = usb_driver_claim_interface(&btusb_driver,
-							data->isoc, NULL);
+							data->isoc, data);
 		if (err < 0) {
 			hci_free_dev(hdev);
 			kfree(data);

@@ -922,13 +928,21 @@ static void btusb_disconnect(struct usb_
 
 	hdev = data->hdev;
 
+	usb_set_intfdata(data->acl, NULL);
 	if (data->isoc)
-		usb_driver_release_interface(&btusb_driver, data->isoc);
-
-	usb_set_intfdata(intf, NULL);
+		usb_set_intfdata(data->isoc, NULL);
 
+	/* unregister before releasing any interface */
 	hci_unregister_dev(hdev);
 
+	cancel_work_sync(&data->work);
+	btusb_stop_traffic(data);
+
+	if (intf == data->isoc)
+		usb_driver_release_interface(&btusb_driver, data->acl);
+	else if (data->isoc)
+		usb_driver_release_interface(&btusb_driver, data->isoc);
+
 	hci_free_dev(hdev);
 }
 

Hi Oliver,

btusb has some races in disconnect()

- it doesn't deal with only the iso interface disconnected

It see the point here, but then we have to release the main interface
cleanly or just disable SCO. I would prefer to just disable SCO.

- it releases the data interface before unregistering the iso interface

How does this happen? It doesn't do it that way.

- it isn't carefull to kill all URBs in case of soft disconnect

The hci_unregister_dev will be calling the close() callback. Check
hci_core.c and hci_dev_do_close().

Regards

Marcel

Am Dienstag 19 August 2008 14:43:01 schrieb Marcel Holtmann:

Hi Oliver,
=20

quoted

btusb has some races in disconnect()
=20
- it doesn't deal with only the iso interface disconnected

=20
It see the point here, but then we have to release the main interface
cleanly or just disable SCO. I would prefer to just disable SCO.

The patch disables the main interface. Just disabling SCO means that
btusb_data.isoc could no longer be treated as constant and locking for it
would be necessary. This seems extravagant for a minor use case.

quoted

- it releases the data interface before unregistering the iso interface

=20
How does this happen? It doesn't do it that way.

=46rom your version:
	if (data->isoc)
		usb_driver_release_interface(&btusb_driver, data->isoc);

	usb_set_intfdata(intf, NULL);

	hci_unregister_dev(hdev);

=46irst you release the interface, then you unregister the hci.

=20

quoted

- it isn't carefull to kill all URBs in case of soft disconnect

=20
The hci_unregister_dev will be calling the close() callback. Check
hci_core.c and hci_dev_do_close().

OK, I'll modify the patch.

	Regards
		Oliver

Hi Oliver,

quoted

quoted

btusb has some races in disconnect()

- it doesn't deal with only the iso interface disconnected

It see the point here, but then we have to release the main interface
cleanly or just disable SCO. I would prefer to just disable SCO.

The patch disables the main interface. Just disabling SCO means that
btusb_data.isoc could no longer be treated as constant and locking for it
would be necessary. This seems extravagant for a minor use case.

I am fine with both ways. Will decide which code looks cleaner :)

quoted

quoted

- it releases the data interface before unregistering the iso interface

How does this happen? It doesn't do it that way.

From your version:
	if (data->isoc)
		usb_driver_release_interface(&btusb_driver, data->isoc);

	usb_set_intfdata(intf, NULL);

	hci_unregister_dev(hdev);

First you release the interface, then you unregister the hci.

Putting it after hci_unregister_dev is an issue. The unregister will
call __hci_dev_put and then in return will call the destruct callback
which will free the data pointer.

So either we use hci_dev_hold or make sure that disconnect can only
disable the SCO interface. That comes directly with the first comment
from you.

Regards

Marcel

Am Dienstag 19 August 2008 15:28:26 schrieb Marcel Holtmann:
Hello,

Hi Oliver,

quoted

quoted

quoted

btusb has some races in disconnect()

- it doesn't deal with only the iso interface disconnected

It see the point here, but then we have to release the main interface
cleanly or just disable SCO. I would prefer to just disable SCO.

The patch disables the main interface. Just disabling SCO means that
btusb_data.isoc could no longer be treated as constant and locking for it
would be necessary. This seems extravagant for a minor use case.

I am fine with both ways. Will decide which code looks cleaner :)

quoted

quoted

quoted

- it releases the data interface before unregistering the iso interface

How does this happen? It doesn't do it that way.

From your version:
	if (data->isoc)
		usb_driver_release_interface(&btusb_driver, data->isoc);

	usb_set_intfdata(intf, NULL);

	hci_unregister_dev(hdev);

First you release the interface, then you unregister the hci.

Putting it after hci_unregister_dev is an issue. The unregister will
call __hci_dev_put and then in return will call the destruct callback
which will free the data pointer.

So either we use hci_dev_hold or make sure that disconnect can only
disable the SCO interface. That comes directly with the first comment
from you.

I'll make a patch with hci_dev_hold. It seems the cleaner solution.

	Regards
		Oliver

Am Dienstag 19 August 2008 15:28:26 schrieb Marcel Holtmann:

quoted

First you release the interface, then you unregister the hci.

Putting it after hci_unregister_dev is an issue. The unregister will
call __hci_dev_put and then in return will call the destruct callback
which will free the data pointer.

So either we use hci_dev_hold or make sure that disconnect can only
disable the SCO interface. That comes directly with the first comment
from you.

Here is a new version. I am afraid normal hci_dev_hold would fail if
the disconnect happens because the btusb module is being unloaded.

	Regards
		Oliver

Signed-off-by: Oliver Neukum <redacted>

----

--- linux-2.6.27-rc3/drivers/bluetooth/btusb.c.marcel	2008-08-19 10:21:51.000000000 +0200
+++ linux-2.6.27-rc3/drivers/bluetooth/btusb.c	2008-08-19 15:50:06.000000000 +0200

@@ -169,6 +169,7 @@ static struct usb_device_id blacklist_ta
 struct btusb_data {
 	struct hci_dev       *hdev;
 	struct usb_device    *udev;
+	struct usb_interface *acl;
 	struct usb_interface *isoc;
 
 	spinlock_t lock;

@@ -523,6 +524,14 @@ static int btusb_open(struct hci_dev *hd
 	return err;
 }
 
+static void btusb_stop_traffic(struct btusb_data *data)
+{
+	usb_kill_anchored_urbs(&data->intr_anchor);
+	usb_kill_anchored_urbs(&data->bulk_anchor);
+	usb_kill_anchored_urbs(&data->isoc_anchor);
+	usb_kill_anchored_urbs(&data->tx_anchor);
+}
+
 static int btusb_close(struct hci_dev *hdev)
 {
 	struct btusb_data *data = hdev->driver_data;

@@ -535,13 +544,9 @@ static int btusb_close(struct hci_dev *h
 	flush_work(&data->work);
 
 	clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
-	usb_kill_anchored_urbs(&data->intr_anchor);
-
 	clear_bit(BTUSB_BULK_RUNNING, &data->flags);
-	usb_kill_anchored_urbs(&data->bulk_anchor);
-
 	clear_bit(BTUSB_INTR_RUNNING, &data->flags);
-	usb_kill_anchored_urbs(&data->intr_anchor);
+	btusb_stop_traffic(data);
 
 	return 0;
 }

@@ -822,6 +827,7 @@ static int btusb_probe(struct usb_interf
 	}
 
 	data->udev = interface_to_usbdev(intf);
+	data->acl = intf;
 
 	spin_lock_init(&data->lock);
 

@@ -890,7 +896,7 @@ static int btusb_probe(struct usb_interf
 
 	if (data->isoc) {
 		err = usb_driver_claim_interface(&btusb_driver,
-							data->isoc, NULL);
+							data->isoc, data);
 		if (err < 0) {
 			hci_free_dev(hdev);
 			kfree(data);

@@ -922,13 +928,23 @@ static void btusb_disconnect(struct usb_
 
 	hdev = data->hdev;
 
-	if (data->isoc)
-		usb_driver_release_interface(&btusb_driver, data->isoc);
+	/* make sure we have a reference */
+	__hci_dev_hold(hdev);
 
-	usb_set_intfdata(intf, NULL);
+	usb_set_intfdata(data->acl, NULL);
+	if (data->isoc)
+		usb_set_intfdata(data->isoc, NULL);
 
+	/* unregister before releasing any interface */
 	hci_unregister_dev(hdev);
 
+	if (intf == data->isoc)
+		usb_driver_release_interface(&btusb_driver, data->acl);
+	else if (data->isoc)
+		usb_driver_release_interface(&btusb_driver, data->isoc);
+
+	/* release the reference */
+	__hci_dev_put(hdev);
 	hci_free_dev(hdev);
 }
 

Hi Oliver,

quoted

quoted

First you release the interface, then you unregister the hci.

Putting it after hci_unregister_dev is an issue. The unregister will
call __hci_dev_put and then in return will call the destruct callback
which will free the data pointer.

So either we use hci_dev_hold or make sure that disconnect can only
disable the SCO interface. That comes directly with the first comment
from you.

Here is a new version. I am afraid normal hci_dev_hold would fail if
the disconnect happens because the btusb module is being unloaded.

the btusb_stop_traffic change is bogus. You don't need that. The flush
callback will be called to ensure that all TX traffic is released.

I am not sure about hci_dev_hold. We ensure that a reference count to
the module is hold when the HCI device is brought up.

Regards

Marcel

Am Dienstag 19 August 2008 16:31:18 schrieb Marcel Holtmann:

Hi Oliver,

quoted

quoted

quoted

First you release the interface, then you unregister the hci.

Putting it after hci_unregister_dev is an issue. The unregister will
call __hci_dev_put and then in return will call the destruct callback
which will free the data pointer.

So either we use hci_dev_hold or make sure that disconnect can only
disable the SCO interface. That comes directly with the first comment
from you.

Here is a new version. I am afraid normal hci_dev_hold would fail if
the disconnect happens because the btusb module is being unloaded.

the btusb_stop_traffic change is bogus. You don't need that. The flush
callback will be called to ensure that all TX traffic is released.

OK.

I am not sure about hci_dev_hold. We ensure that a reference count to
the module is hold when the HCI device is brought up.

But won't the module be marked as going away and make try_module_get()
fail?

	Regards
		Oliver

Signed-off-by: Oliver Neukum <redacted>

---

--- linux-2.6.27-rc3/drivers/bluetooth/btusb.c.marcel	2008-08-19 10:21:51.000000000 +0200
+++ linux-2.6.27-rc3/drivers/bluetooth/btusb.c	2008-08-19 15:50:06.000000000 +0200

@@ -169,6 +169,7 @@ static struct usb_device_id blacklist_ta
 struct btusb_data {
 	struct hci_dev       *hdev;
 	struct usb_device    *udev;
+	struct usb_interface *acl;
 	struct usb_interface *isoc;
 
 	spinlock_t lock;

@@ -822,6 +827,7 @@ static int btusb_probe(struct usb_interf
 	}
 
 	data->udev = interface_to_usbdev(intf);
+	data->acl = intf;
 
 	spin_lock_init(&data->lock);
 

@@ -890,7 +896,7 @@ static int btusb_probe(struct usb_interf
 
 	if (data->isoc) {
 		err = usb_driver_claim_interface(&btusb_driver,
-							data->isoc, NULL);
+							data->isoc, data);
 		if (err < 0) {
 			hci_free_dev(hdev);
 			kfree(data);

@@ -922,13 +928,23 @@ static void btusb_disconnect(struct usb_
 
 	hdev = data->hdev;
 
-	if (data->isoc)
-		usb_driver_release_interface(&btusb_driver, data->isoc);
+	/* make sure we have a reference */
+	__hci_dev_hold(hdev);
 
-	usb_set_intfdata(intf, NULL);
+	usb_set_intfdata(data->acl, NULL);
+	if (data->isoc)
+		usb_set_intfdata(data->isoc, NULL);
 
+	/* unregister before releasing any interface */
 	hci_unregister_dev(hdev);
 
+	if (intf == data->isoc)
+		usb_driver_release_interface(&btusb_driver, data->acl);
+	else if (data->isoc)
+		usb_driver_release_interface(&btusb_driver, data->isoc);
+
+	/* release the reference */
+	__hci_dev_put(hdev);
 	hci_free_dev(hdev);
 }
 

Hi Oliver,

quoted

quoted

quoted

quoted

First you release the interface, then you unregister the hci.

Putting it after hci_unregister_dev is an issue. The unregister will
call __hci_dev_put and then in return will call the destruct callback
which will free the data pointer.

So either we use hci_dev_hold or make sure that disconnect can only
disable the SCO interface. That comes directly with the first comment
from you.

Here is a new version. I am afraid normal hci_dev_hold would fail if
the disconnect happens because the btusb module is being unloaded.

the btusb_stop_traffic change is bogus. You don't need that. The flush
callback will be called to ensure that all TX traffic is released.

OK.

quoted

I am not sure about hci_dev_hold. We ensure that a reference count to
the module is hold when the HCI device is brought up.

But won't the module be marked as going away and make try_module_get()
fail?

when calling "hciconfig hci0 up" then it is impossible to unload the
btusb module since we keep a module reference.

I am thinking that just __set_isoc_interface(..., 0) when disconnect
gets called for the isoc interface and the releasing the interface is
the more cleaner way. I have to think about this.

Regards

Marcel

Am Dienstag 19 August 2008 17:27:23 schrieb Marcel Holtmann:

quoted

quoted

I am not sure about hci_dev_hold. We ensure that a reference count to
the module is hold when the HCI device is brought up.

But won't the module be marked as going away and make try_module_get()
fail?

when calling "hciconfig hci0 up" then it is impossible to unload the
btusb module since we keep a module reference.

There's no guarantee the hci is up when disconnect() is called.

	Regards
		Oliver