Re: [syzbot] WARNING: ODEBUG bug in mgmt_index_removed

[Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>]

I guess that, since hci_pi(sk)->hdev = hdev in hci_sock_bind() does not check whether a hdev
is already associated with some sk, it is possible that multiple sk are bound to the same hdev.
As a result, lock_sock(sk) is not sufficient for serializing access to hdev, and
hci_dev_lock(hdev) is needed when calling mgmt_index_*(hdev) functions.

If my guess above is correct, I think that what syzbot is telling us is that, due to lack of
serialization via hci_dev_lock(hdev), setting of HCI_MGMT flag from mgmt_init_hdev() from
hci_mgmt_cmd() from hci_sock_sendmsg() is racing with testing of HCI_MGMT flag from
mgmt_index_removed() from hci_sock_bind().

I suggest you to explicitly use lockdep_assert_held() in Bluethooth code for clarifying what
locks need to be held, instead of continue using racy operations like hci_dev_test_and_set_flag()
without holding appropriate locks.



hci_unregister_dev() {
  if (!test_bit(HCI_INIT, &hdev->flags) &&
      !hci_dev_test_flag(hdev, HCI_SETUP) &&
      !hci_dev_test_flag(hdev, HCI_CONFIG)) {
    hci_dev_lock(hdev);
    mgmt_index_removed(hdev) {
      if (!hci_dev_test_flag(hdev, HCI_MGMT))
        return;
      cancel_delayed_work_sync(&hdev->discov_off);
    }
    hci_dev_unlock(hdev);
  }
}

hci_sock_sendmsg() {
  lock_sock(sk);
  mutex_lock(&mgmt_chan_list_lock);
  chan = __hci_mgmt_chan_find(hci_pi(sk)->channel);
  if (chan)
    err = hci_mgmt_cmd(chan, sk, skb) {
      if (hdev && chan->hdev_init) // chan->hdev_init == mgmt_init_hdev
        chan->hdev_init(sk, hdev) {
          if (hci_dev_test_and_set_flag(hdev, HCI_MGMT)) // Missing hci_dev_lock(hdev)
            return;
          INIT_DELAYED_WORK(&hdev->discov_off, discov_off);
        }
      err = handler->func(sk, hdev, cp, len) { // handler->func() == set_external_config or set_public_address
        hci_dev_lock(hdev);
        mgmt_index_removed(hdev) {
          if (!hci_dev_test_flag(hdev, HCI_MGMT))
            return;
          cancel_delayed_work_sync(&hdev->discov_off);
        }
        hci_dev_unlock(hdev);
      }
    }
  else
    err = -EINVAL;
  mutex_unlock(&mgmt_chan_list_lock);
  release_sock(sk);
}

hci_sock_bind() {
  lock_sock(sk);
  mgmt_index_removed(hdev) {
    if (!hci_dev_test_flag(hdev, HCI_MGMT)) // Missing hci_dev_lock(hdev)
      return;
    cancel_delayed_work_sync(&hdev->discov_off); // ODEBUG complains missing INIT_DELAYED_WORK()
  }
  release_sock(sk);
}