Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d2d763e3-0dc8-03df-45ba-8d5c4a25acda@I-love.SAKURA.ne.jp>
Date:   Sat, 10 Sep 2022 20:18:22 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.1
Subject: Re: [syzbot] WARNING: ODEBUG bug in mgmt_index_removed
Content-Language: en-US
To:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>
References: <000000000000532e0e05e826413c@google.com>
Cc:     syzbot <syzbot+844c7bf1b1aa4119c5de@syzkaller.appspotmail.com>,
        linux-bluetooth@vger.kernel.org, syzkaller-bugs@googlegroups.com
From:   Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
In-Reply-To: <000000000000532e0e05e826413c@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);
}