Hi David,

On Fri, Mar 09, 2012 at 03:04:11PM +0100, David Herrmann wrote:

On Fri, Mar 9, 2012 at 1:53 PM, Johan Hovold [off-list ref] wrote:

quoted

Make sure hci_dev_open returns immediately if hci_dev_unregister has
been called.

This fixes a race between hci_dev_open and hci_dev_unregister which can
lead to a NULL-pointer dereference.

Bug is 100% reproducible using hciattach and a disconnected serial port:

0. # hciattach -n /dev/ttyO1 any noflow

1. hci_dev_open called from hci_power_on grabs req lock
2. hci_init_req executes but device fails to initialise (times out
  eventually)
3. hci_dev_open is called from hci_sock_ioctl and sleeps on req lock
4. hci_uart_tty_close calls hci_dev_unregister and sleeps on req lock in
  hci_dev_do_close
5. hci_dev_open (1) releases req lock
6. hci_dev_do_close grabs req lock and returns as device is not up
7. hci_dev_unregister sleeps in destroy_workqueue
8. hci_dev_open (3) grabs req lock, calls hci_init_req and eventually sleeps
9. hci_dev_unregister finishes, while hci_dev_open is still running...

[...]

quoted

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 00596e8..e8879b9 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h

@@ -93,6 +93,8 @@ enum {

 * states from the controller.
 */
 enum {
+       HCI_UNREGISTER,
+
       HCI_LE_SCAN,
 };

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d6448f0..22b6781 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c

@@ -525,6 +525,11 @@ int hci_dev_open(__u16 dev)

       hci_req_lock(hdev);

+       if (test_bit(HCI_UNREGISTER, &hdev->dev_flags)) {
+               ret = -ENODEV;
+               goto done;
+       }
+

Isn't it enough to check for HCI_RUNNING here? We obviously have a
race here as we take the device with hci_dev_get(), then sleep and
then we do not check whether the device is still alive. However,
drivers are required to reset HCI_RUNNING before calling
hci_unregister_dev() (which is bogus anyway, but its the way we
handled it in the past) therefore it should be enough for us to check
for HCI_RUNNING.

I'm afraid this won't work as hci_dev_open is responsible for setting
HCI_RUNNING in the first place (set in hdev->open(hdev) called from
hci_dev_open).

Thanks,
Johan