hci_conn_abort may run concurrently with operations that e.g. change
conn handle or delete it. Similarly, hci_disconnect_all_sync iterates
conn_hash without locks/RCU which is not OK.
To make it correct vs locking, use hci_conn_sync_queue to hold
hdev->lock and check hci_conn aliveness after waiting for events.
Make iteration in hci_disconnect_all_sync safe. Since we don't have a
flag for indicating whether a connection was aborted, just make a copy
of the conn_hash and iterate the copy.
Signed-off-by: Pauli Virtanen <pav@xxxxxx>
---
Notes:
The iterate-over-copy is ugly, could add another hci_conn flag. Or
change things so that hci_conn_get keeps the hci_conn in conn_hash so we
can always continue the iteration safely as long as a reference was
held.
net/bluetooth/hci_conn.c | 10 ++------
net/bluetooth/hci_sync.c | 52 ++++++++++++++++++++++++++++------------
2 files changed, 39 insertions(+), 23 deletions(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 208eb5eea451..ba339a0eb851 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2845,12 +2845,7 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
static int abort_conn_sync(struct hci_dev *hdev, void *data)
{
- struct hci_conn *conn;
- u16 handle = PTR_ERR(data);
-
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
+ struct hci_conn *conn = data;
return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
}
@@ -2886,8 +2881,7 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
}
}
- return hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
- NULL);
+ return hci_conn_sync_queue(conn, abort_conn_sync, conn, NULL);
}
struct hci_conn_sync_work_entry {
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 6a295a9e1f5d..101548fe81da 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5407,6 +5407,8 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
{
int err;
+ lockdep_assert_held(&hdev->lock);
+
switch (conn->state) {
case BT_CONNECTED:
case BT_CONFIG:
@@ -5418,21 +5420,15 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
* or in case of LE it was still scanning so it can be cleanup
* safely.
*/
- if (err) {
- hci_dev_lock(hdev);
+ if (err && hci_conn_is_alive(hdev, conn))
hci_conn_failed(conn, err);
- hci_dev_unlock(hdev);
- }
return err;
case BT_CONNECT2:
return hci_reject_conn_sync(hdev, conn, reason);
case BT_OPEN:
/* Cleanup bises that failed to be established */
- if (test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags)) {
- hci_dev_lock(hdev);
+ if (test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags))
hci_conn_failed(conn, reason);
- hci_dev_unlock(hdev);
- }
break;
default:
conn->state = BT_CLOSED;
@@ -5444,16 +5440,42 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
static int hci_disconnect_all_sync(struct hci_dev *hdev, u8 reason)
{
- struct hci_conn *conn, *tmp;
- int err;
+ struct hci_conn *conn;
+ struct hci_conn **conns;
+ int err = 0;
+ size_t i, n;
- list_for_each_entry_safe(conn, tmp, &hdev->conn_hash.list, list) {
- err = hci_abort_conn_sync(hdev, conn, reason);
- if (err)
- return err;
+ hci_cmd_sync_dev_lock(hdev);
+
+ /* Make a copy of conn_hash, because hci_abort_conn_sync may release the
+ * lock and wait for events, during which the list may be mutated
+ * arbitrarily.
+ */
+ n = 0;
+ list_for_each_entry(conn, &hdev->conn_hash.list, list)
+ ++n;
+
+ conns = kvcalloc(n, sizeof(*conns), GFP_KERNEL);
+ if (!conns) {
+ err = -ENOMEM;
+ goto unlock;
}
- return 0;
+ i = 0;
+ list_for_each_entry(conn, &hdev->conn_hash.list, list)
+ conns[i++] = hci_conn_get(conn);
+
+ for (i = 0; i < n; ++i) {
+ if (!err)
+ err = hci_abort_conn_sync(hdev, conns[i], reason);
+ hci_conn_put(conns[i]);
+ }
+
+ kvfree(conns);
+
+unlock:
+ hci_cmd_sync_dev_unlock(hdev);
+ return err;
}
/* This function perform power off HCI command sequence as follows:
--
2.41.0
