[PATCH RFC 0/6] Locking in hci_sync

[PATCH RFC 0/6] Locking in hci_sync

[Pauli Virtanen]

The current guarantees that a given hci_conn is not freed concurrently
appear to be:

- hci_dev_lock is held, or,
- rcu_read_lock is held  (doesn't guarantee valid conn state), or,
- hci_conn_get refcount held (doesn't guarantee valid conn state),
- current task is running from hdev->workqueue (which is ordered)

The last condition is not exactly true as hci_conn_del is currently
called also elsewhere, but is assumed in hci_event.c and hci_core.c.

It does not appear possible to assume in hdev->req_workqueue, especially
in hci_sync callbacks, that a given conn stays alive at any time if none
of the above precautions are taken.  Similarly, any conn_hash iteration
without locks there appears invalid.

E.g. Disconnect events emitted from remote appear possible to occur at
any time in hci_sync.  There are also some hci_conn_del callsites not in
hdev->workqueue which can run concurrently, eg in __iso_sock_close
(which also doesn't hold hdev->lock).

KASAN crashes are known on real HW, and the deletion race conditions can
be hit in VHCI (see the other BlueZ tester patch series).

***

What to do?

One way to guard against using already freed conns is hci_conn_get +
check that the connection was not deleted, inside suitable critical
section. I didn't find a reliable existing check if hci_conn_del was
run.

To enable using hci_conn_get, the series here suggests again adding
HCI_CONN_DELETED flag to indicate whether hci_conn_del has run (with
hdev->lock or in hdev->workqueue).

It also adds some helpers to make writing hci_sync callbacks that
operate on a given hci_conn or need hdev->lock easier to write. The
patches here add hci_conn_sync_queue, which check if hci_conn was
deleted in meantime, and hold hdev->lock in the callback (but release it
during waits), so that concurrent modification of hci_conn can only
happen during event waits. This is something that is already assumed but
might not be true eg. if the two workqueues run jobs on different CPU.

The last two patches in the series are some motivating ISO related
changes, for proper cleanup of CIS, lookup by handles doesn't quite
work (and also doesn't protect against the deletion race).

***

I tried to find some alternatives, but this seemed minimal one.

I don't see how to make use of hci_conn_drop/hold here, as they appear
to have different purpose, if we change that then how socket channels
work seem to a new mechanism. E.g. nonzero refcnt from socket should not
keep connection alive when eg. Disconnect event from remote occurs.  In
that case we also need to clean up the connection ASAP so the handle can
be reused.

One alternative that makes continuing conn_hash iteration a bit simpler
would be to remove hci_conn from conn_hash only when hci_conn_get
refcount hits zero, so holding hci_conn_get reference would keep the
conn a valid iteration cursor. Also setting conn->type to INVALID_LINK
on deletion could then make lookup functions skip deleted conns.
However, one would need to take a look at all places where conn_hash is
iterated and think it through.

It maybe could also be possible to not allow events to be processed
while hci_sync is running, except when it is waiting for an event (=
more or less, take hci_cmd_sync_dev_lock in all callbacks so hdev->lock
serializes things).  However, it seems a deletion flag would be needed
also here, as the conn might be gone while we are waiting for events.

Pauli Virtanen (6):
  Bluetooth: hci_conn: hci_conn_cleanup is not needed, combine with del
  Bluetooth: hci_conn: add hci_conn_is_alive
  Bluetooth: hci_sync: add hci_conn_sync_queue and
    hci_cmd_sync_dev_(un)lock
  Bluetooth: hci_sync: fix locking in hci_conn_abort and
    hci_disconnect_all_sync
  Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting
  Bluetooth: ISO: handle bound CIS cleanup via hci_conn

 include/net/bluetooth/hci_core.h |  22 +++++
 include/net/bluetooth/hci_sync.h |   3 +
 net/bluetooth/hci_conn.c         | 157 ++++++++++++++++++++-----------
 net/bluetooth/hci_sync.c         |  97 +++++++++++++++----
 net/bluetooth/iso.c              |  14 +--
 5 files changed, 211 insertions(+), 82 deletions(-)

-- 
2.41.0

[PATCH RFC 1/6] Bluetooth: hci_conn: hci_conn_cleanup is not needed, combine with del

[Pauli Virtanen]

hci_conn_cleanup is no longer needed, so move the code back to
hci_conn_del to keep the hci_conn teardown in a single place.

This undoes commit b958f9a3e877 ("Bluetooth: Fix reference counting for
LE-scan based connections"), but keeps the current order of cleanup
operations.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/hci_conn.c | 78 +++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index cccc2b8b60a8..a71a54a5c8d8 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -139,45 +139,6 @@ static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
 	hci_update_passive_scan(hdev);
 }
 
-static void hci_conn_cleanup(struct hci_conn *conn)
-{
-	struct hci_dev *hdev = conn->hdev;
-
-	if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
-		hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
-
-	if (test_and_clear_bit(HCI_CONN_FLUSH_KEY, &conn->flags))
-		hci_remove_link_key(hdev, &conn->dst);
-
-	hci_chan_list_flush(conn);
-
-	hci_conn_hash_del(hdev, conn);
-
-	if (conn->cleanup)
-		conn->cleanup(conn);
-
-	if (conn->type == SCO_LINK || conn->type == ESCO_LINK) {
-		switch (conn->setting & SCO_AIRMODE_MASK) {
-		case SCO_AIRMODE_CVSD:
-		case SCO_AIRMODE_TRANSP:
-			if (hdev->notify)
-				hdev->notify(hdev, HCI_NOTIFY_DISABLE_SCO);
-			break;
-		}
-	} else {
-		if (hdev->notify)
-			hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
-	}
-
-	hci_conn_del_sysfs(conn);
-
-	debugfs_remove_recursive(conn->debugfs);
-
-	hci_dev_put(hdev);
-
-	hci_conn_put(conn);
-}
-
 static void hci_acl_create_connection(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
@@ -1127,12 +1088,39 @@ void hci_conn_del(struct hci_conn *conn)
 
 	skb_queue_purge(&conn->data_q);
 
-	/* Remove the connection from the list and cleanup its remaining
-	 * state. This is a separate function since for some cases like
-	 * BT_CONNECT_SCAN we *only* want the cleanup part without the
-	 * rest of hci_conn_del.
-	 */
-	hci_conn_cleanup(conn);
+	if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+		hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
+	if (test_and_clear_bit(HCI_CONN_FLUSH_KEY, &conn->flags))
+		hci_remove_link_key(hdev, &conn->dst);
+
+	hci_chan_list_flush(conn);
+
+	hci_conn_hash_del(hdev, conn);
+
+	if (conn->cleanup)
+		conn->cleanup(conn);
+
+	if (conn->type == SCO_LINK || conn->type == ESCO_LINK) {
+		switch (conn->setting & SCO_AIRMODE_MASK) {
+		case SCO_AIRMODE_CVSD:
+		case SCO_AIRMODE_TRANSP:
+			if (hdev->notify)
+				hdev->notify(hdev, HCI_NOTIFY_DISABLE_SCO);
+			break;
+		}
+	} else {
+		if (hdev->notify)
+			hdev->notify(hdev, HCI_NOTIFY_CONN_DEL);
+	}
+
+	hci_conn_del_sysfs(conn);
+
+	debugfs_remove_recursive(conn->debugfs);
+
+	hci_dev_put(hdev);
+
+	hci_conn_put(conn);
 }
 
 struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
-- 
2.41.0

[PATCH RFC 2/6] Bluetooth: hci_conn: add hci_conn_is_alive

[Pauli Virtanen]

There's currently no good way to know if a given hci_conn we hold
hci_conn_get reference for has been deleted in the meanwhile.

To enable checking whether a connection is still alive, add
HCI_CONN_DELETED flag to indicate whether hci_conn_del has run.

The flag is meaningful also with RCU lock only, but with different
semantics.  If hci_conn_is_alive(conn) returns true inside
rcu_read_lock, conn was in conn_hash from the point of view of the
current task when the flag was read. Then its deletion will not complete
at least before rcu_read_unlock.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    This is now suggesting the same thing once again, because the
    alternatives seem limited, and it's currently hard to write code in
    hci_sync safe against memory bugs.
    
    One alternative here is to remove the connection from conn_hash only
    when hci_conn_get/put refcount goes to zero. This simplifies resuming
    interrupted conn_hash iteration, as hci_conn_get then ensures the conn
    remains a valid iteration cursor. However, some deletion indicator flag
    is still needed, if we want to do something else with the conn.
    
    The suggestion to use conn->refcnt seems a bit hard to make it work,
    since that is used for different purpose (marking what connections
    userspace keeps alive), and it seems we'd need to change all callsites
    for hci_conn_del.
    
    E.g. in Disconnect Complete event we must remove the conn from sysfs and
    invalidate its handle immediately as the handle can be reused.
    Similarly for failed LE Create CIS while hci_conn_abort is running.
    
    It maybe could also be possible to not allow events to run while
    hci_sync is running, except when it is waiting for an event.  This seems
    actually to be the concurrency model what is assumed in the hci_sync
    code (but it is not what is actually happening).  However, it seems the
    deletion flag would be needed also here, as the conn might be gone while
    we are waiting for events.

 include/net/bluetooth/hci_core.h | 15 +++++++++++++++
 net/bluetooth/hci_conn.c         |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8200a6689b39..34e4ad7c32e7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -978,6 +978,7 @@ enum {
 	HCI_CONN_CREATE_CIS,
 	HCI_CONN_BIG_SYNC,
 	HCI_CONN_BIG_SYNC_FAILED,
+	HCI_CONN_DELETED,
 };
 
 static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
@@ -997,6 +998,7 @@ static inline bool hci_conn_sc_enabled(struct hci_conn *conn)
 static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c)
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
+	WARN_ON(test_bit(HCI_CONN_DELETED, &c->flags));
 	list_add_tail_rcu(&c->list, &h->list);
 	switch (c->type) {
 	case ACL_LINK:
@@ -1024,6 +1026,7 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c)
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
 
+	WARN_ON(!test_bit(HCI_CONN_DELETED, &c->flags));
 	list_del_rcu(&c->list);
 	synchronize_rcu();
 
@@ -1049,6 +1052,18 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c)
 	}
 }
 
+/* With hdev->lock: whether hci_conn is in conn_hash.
+ * With RCU: if true, the hci_conn is valid conn_hash iteration cursor and
+ * hci_conn_hash_del has not completed. (Note that if hci_conn was obtained in
+ * this critical section it is always valid, but this may return false!)
+ */
+static inline bool hci_conn_is_alive(struct hci_dev *hdev, struct hci_conn *c)
+{
+	RCU_LOCKDEP_WARN(!lockdep_is_held(&hdev->lock) && !rcu_read_lock_held(),
+			 "suspicious locking");
+	return !test_bit(HCI_CONN_DELETED, &c->flags);
+}
+
 static inline unsigned int hci_conn_num(struct hci_dev *hdev, __u8 type)
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index a71a54a5c8d8..ee304618bf0a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1052,9 +1052,13 @@ static void hci_conn_unlink(struct hci_conn *conn)
 void hci_conn_del(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
+	bool deleted;
 
 	BT_DBG("%s hcon %p handle %d", hdev->name, conn, conn->handle);
 
+	deleted = test_and_set_bit(HCI_CONN_DELETED, &conn->flags);
+	WARN_ON(deleted);
+
 	hci_conn_unlink(conn);
 
 	cancel_delayed_work_sync(&conn->disc_work);
-- 
2.41.0

[PATCH RFC 3/6] Bluetooth: hci_sync: add hci_conn_sync_queue and hci_cmd_sync_dev_(un)lock

[Pauli Virtanen]

Operations on a given hci_conn in hci_sync are hard to write safely,
because the conn might be deleted or modified unexpectedly either
concurrently or when waiting for the events.  Holding hci_dev_lock in
the sync callbacks is also inconvenient since it has to be manually
released before entering the event waits.

Add convenience utilities to make things easier:

Add hci_cmd_sync_dev_lock/unlock, for easier writing of hci_sync
callbacks where hci_dev_lock should be held. The lock is however still
released and reacquired around request waits. Callbacks using them can
then assume that state changes protected by hci_dev_lock can only occur
when waiting for events. (This is currently assumed in many of the
callbacks.)

Add hci_conn_sync_queue/submit, whose callback on entry holds
hci_dev_lock and the hci_conn has not been deleted.  If it was deleted
while the sync command was queued, the destroy routine has err -ENODEV.
Similarly, synchronous commands called in the callback fail with ENODEV
if the conn was deleted during the wait.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/hci_core.h |  7 ++++
 include/net/bluetooth/hci_sync.h |  3 ++
 net/bluetooth/hci_conn.c         | 60 ++++++++++++++++++++++++++++++++
 net/bluetooth/hci_sync.c         | 31 +++++++++++++++++
 4 files changed, 101 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 34e4ad7c32e7..8e218300ef4e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -519,6 +519,9 @@ struct hci_dev {
 	struct work_struct	cmd_sync_cancel_work;
 	struct work_struct	reenable_adv_work;
 
+	bool			cmd_sync_locked;
+	struct hci_conn		*cmd_sync_conn;
+
 	__u16			discov_timeout;
 	struct delayed_work	discov_off;
 
@@ -1400,6 +1403,10 @@ void hci_conn_del(struct hci_conn *conn);
 void hci_conn_hash_flush(struct hci_dev *hdev);
 void hci_conn_check_pending(struct hci_dev *hdev);
 
+void hci_conn_sync_set_conn(struct hci_dev *hdev, struct hci_conn *conn);
+int hci_conn_sync_queue(struct hci_conn *conn, hci_cmd_sync_work_func_t func,
+			void *data, hci_cmd_sync_work_destroy_t destroy);
+
 struct hci_chan *hci_chan_create(struct hci_conn *conn);
 void hci_chan_del(struct hci_chan *chan);
 void hci_chan_list_flush(struct hci_conn *conn);
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index b516a0f4a55b..a9a94950d523 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -46,6 +46,9 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 		       void *data, hci_cmd_sync_work_destroy_t destroy);
 
+void hci_cmd_sync_dev_lock(struct hci_dev *hdev);
+void hci_cmd_sync_dev_unlock(struct hci_dev *hdev);
+
 int hci_update_eir_sync(struct hci_dev *hdev);
 int hci_update_class_sync(struct hci_dev *hdev);
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index ee304618bf0a..208eb5eea451 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2889,3 +2889,63 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
 	return hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
 				  NULL);
 }
+
+struct hci_conn_sync_work_entry {
+	struct hci_conn *conn;
+	hci_cmd_sync_work_func_t func;
+	void *data;
+	hci_cmd_sync_work_destroy_t destroy;
+};
+
+static int hci_conn_sync_work_run(struct hci_dev *hdev, void *data)
+{
+	struct hci_conn_sync_work_entry *entry = data;
+	int err;
+
+	hci_cmd_sync_dev_lock(hdev);
+	hdev->cmd_sync_conn = entry->conn;
+
+	if (hci_conn_is_alive(hdev, entry->conn))
+		err = entry->func(hdev, entry->data);
+	else
+		err = -ENODEV;
+
+	hdev->cmd_sync_conn = NULL;
+	hci_cmd_sync_dev_unlock(hdev);
+
+	return err;
+}
+
+static void hci_conn_sync_work_destroy(struct hci_dev *hdev, void *data,
+				       int err)
+{
+	struct hci_conn_sync_work_entry *entry = data;
+
+	if (entry->destroy)
+		entry->destroy(hdev, entry->data, err);
+	hci_conn_put(entry->conn);
+	kfree(entry);
+}
+
+int hci_conn_sync_queue(struct hci_conn *conn, hci_cmd_sync_work_func_t func,
+			void *data, hci_cmd_sync_work_destroy_t destroy)
+{
+	struct hci_conn_sync_work_entry *entry;
+	int err;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	entry->func = func;
+	entry->data = data;
+	entry->destroy = destroy;
+	entry->conn = hci_conn_get(conn);
+
+	err = hci_cmd_sync_queue(conn->hdev, hci_conn_sync_work_run, entry,
+				 hci_conn_sync_work_destroy);
+	if (err)
+		kfree(entry);
+
+	return err;
+}
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 3348a1b0e3f7..6a295a9e1f5d 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -164,10 +164,16 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
 	if (err < 0)
 		return ERR_PTR(err);
 
+	if (hdev->cmd_sync_locked)
+		hci_dev_unlock(hdev);
+
 	err = wait_event_interruptible_timeout(hdev->req_wait_q,
 					       hdev->req_status != HCI_REQ_PEND,
 					       timeout);
 
+	if (hdev->cmd_sync_locked)
+		hci_dev_lock(hdev);
+
 	if (err == -ERESTARTSYS)
 		return ERR_PTR(-EINTR);
 
@@ -185,6 +191,11 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
 		break;
 	}
 
+	if (hdev->cmd_sync_conn) {
+		if (!hci_conn_is_alive(hdev, hdev->cmd_sync_conn))
+			err = -ENODEV;
+	}
+
 	hdev->req_status = 0;
 	hdev->req_result = 0;
 	skb = hdev->req_skb;
@@ -740,6 +751,26 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 }
 EXPORT_SYMBOL(hci_cmd_sync_queue);
 
+void hci_cmd_sync_dev_lock(struct hci_dev *hdev)
+{
+	lockdep_assert_held(&hdev->req_lock);
+
+	hci_dev_lock(hdev);
+
+	WARN_ON_ONCE(hdev->cmd_sync_locked);
+	hdev->cmd_sync_locked = true;
+}
+
+void hci_cmd_sync_dev_unlock(struct hci_dev *hdev)
+{
+	lockdep_assert_held(&hdev->req_lock);
+
+	WARN_ON_ONCE(!hdev->cmd_sync_locked);
+	hdev->cmd_sync_locked = false;
+
+	hci_dev_unlock(hdev);
+}
+
 int hci_update_eir_sync(struct hci_dev *hdev)
 {
 	struct hci_cp_write_eir cp;
-- 
2.41.0

[PATCH RFC 4/6] Bluetooth: hci_sync: fix locking in hci_conn_abort and hci_disconnect_all_sync

[Pauli Virtanen]

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@iki.fi>
---

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

[PATCH RFC 5/6] Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting

[Pauli Virtanen]

Dropped CIS that are in state BT_OPEN/BT_BOUND, and in state BT_CONNECT
with HCI_CONN_CREATE_CIS unset, should be cleaned up immediately.
Closing CIS ISO sockets should result to the hci_conn be deleted, so
that potentially pending CIG removal can run.

hci_abort_conn cannot refer to them by handle, since their handle is
still unset if Set CIG Parameters has not yet completed.

This fixes CIS not being terminated if the socket is shut down
immediately after connection, so that the hci_abort_conn runs before Set
CIG Parameters completes. See new BlueZ test "ISO Connect Close - Success"

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/hci_sync.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 101548fe81da..3926213c29e6 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5339,6 +5339,10 @@ static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn,
 		if (test_bit(HCI_CONN_CREATE_CIS, &conn->flags))
 			return hci_disconnect_sync(hdev, conn, reason);
 
+		/* CIS with no Create CIS sent have nothing to cancel */
+		if (bacmp(&conn->dst, BDADDR_ANY))
+			return HCI_ERROR_LOCAL_HOST_TERM;
+
 		/* There is no way to cancel a BIS without terminating the BIG
 		 * which is done later on connection cleanup.
 		 */
@@ -5426,9 +5430,17 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
 	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))
+		/* Cleanup failed CIS, and BIS that failed to be established */
+		if (bacmp(&conn->dst, BDADDR_ANY) ||
+		    test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags))
+			hci_conn_failed(conn, reason);
+		break;
+	case BT_BOUND:
+		/* Bound CIS should be cleaned up */
+		if (conn->type == ISO_LINK && bacmp(&conn->dst, BDADDR_ANY))
 			hci_conn_failed(conn, reason);
+		else
+			conn->state = BT_CLOSED;
 		break;
 	default:
 		conn->state = BT_CLOSED;
-- 
2.41.0

[PATCH RFC 6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn

[Pauli Virtanen]

Calling hci_conn_del in __iso_sock_close is invalid. It needs
hdev->lock, but it cannot be acquired there due to lock ordering.

Fix this by doing cleanup via hci_conn_drop.

Return hci_conn with refcount 1 from hci_bind_cis and hci_connect_cis,
so that the iso_conn always holds one reference.  This also fixes
refcounting when error handling.

Since hci_conn_abort shall handle termination of connections in any
state properly, we can handle BT_CONNECT socket state in the same way as
BT_CONNECTED.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/hci_conn.c |  5 +++++
 net/bluetooth/iso.c      | 14 +-------------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index ba339a0eb851..33fbdc8e0748 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1901,6 +1901,8 @@ struct hci_conn *hci_bind_cis(struct hci_dev *hdev, bdaddr_t *dst,
 		return ERR_PTR(-EINVAL);
 	}
 
+	hci_conn_hold(cis);
+
 	cis->iso_qos = *qos;
 	cis->state = BT_BOUND;
 
@@ -2254,6 +2256,9 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
 		return ERR_PTR(-ENOLINK);
 	}
 
+	/* Link takes the refcount */
+	hci_conn_drop(cis);
+
 	cis->state = BT_CONNECT;
 
 	hci_le_create_cis_pending(hdev);
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index cbe3299b4a41..358954bfbb32 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -628,6 +628,7 @@ static void __iso_sock_close(struct sock *sk)
 		iso_sock_cleanup_listen(sk);
 		break;
 
+	case BT_CONNECT:
 	case BT_CONNECTED:
 	case BT_CONFIG:
 		if (iso_pi(sk)->conn->hcon) {
@@ -643,19 +644,6 @@ static void __iso_sock_close(struct sock *sk)
 		break;
 
 	case BT_CONNECT2:
-		iso_chan_del(sk, ECONNRESET);
-		break;
-	case BT_CONNECT:
-		/* In case of DEFER_SETUP the hcon would be bound to CIG which
-		 * needs to be removed so just call hci_conn_del so the cleanup
-		 * callback do what is needed.
-		 */
-		if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags) &&
-		    iso_pi(sk)->conn->hcon) {
-			hci_conn_del(iso_pi(sk)->conn->hcon);
-			iso_pi(sk)->conn->hcon = NULL;
-		}
-
 		iso_chan_del(sk, ECONNRESET);
 		break;
 	case BT_DISCONN:
-- 
2.41.0

RE: Locking in hci_sync

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=769854

---Test result---

Test Summary:
CheckPatch                    PASS      5.24 seconds
GitLint                       FAIL      2.16 seconds
SubjectPrefix                 PASS      0.75 seconds
BuildKernel                   PASS      32.99 seconds
CheckAllWarning               PASS      36.22 seconds
CheckSparse                   PASS      42.20 seconds
CheckSmatch                   PASS      112.30 seconds
BuildKernel32                 PASS      31.92 seconds
TestRunnerSetup               PASS      484.75 seconds
TestRunner_l2cap-tester       PASS      23.23 seconds
TestRunner_iso-tester         PASS      44.83 seconds
TestRunner_bnep-tester        PASS      10.64 seconds
TestRunner_mgmt-tester        PASS      216.30 seconds
TestRunner_rfcomm-tester      PASS      16.08 seconds
TestRunner_sco-tester         PASS      16.79 seconds
TestRunner_ioctl-tester       PASS      17.90 seconds
TestRunner_mesh-tester        PASS      13.44 seconds
TestRunner_smp-tester         PASS      14.31 seconds
TestRunner_userchan-tester    PASS      11.30 seconds
IncrementalBuild              PASS      102.70 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[RFC,2/6] Bluetooth: hci_conn: add hci_conn_is_alive

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
22: B2 Line has trailing whitespace: "    "
28: B2 Line has trailing whitespace: "    "
33: B2 Line has trailing whitespace: "    "
37: B2 Line has trailing whitespace: "    "
[RFC,3/6] Bluetooth: hci_sync: add hci_conn_sync_queue and hci_cmd_sync_dev_(un)lock

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (84>80): "[RFC,3/6] Bluetooth: hci_sync: add hci_conn_sync_queue and hci_cmd_sync_dev_(un)lock"
[RFC,4/6] Bluetooth: hci_sync: fix locking in hci_conn_abort and hci_disconnect_all_sync

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (88>80): "[RFC,4/6] Bluetooth: hci_sync: fix locking in hci_conn_abort and hci_disconnect_all_sync"


---
Regards,
Linux Bluetooth

Re: [PATCH RFC 1/6] Bluetooth: hci_conn: hci_conn_cleanup is not needed, combine with del

[Paul Menzel]

Dear Pauli,


Thank you for your patch.

You might want to make the commit message summary a statement about the 
action. Maybe:

Combine unneeded hci_conn_cleanup() with hci_conn_del()

Am 26.07.23 um 23:25 schrieb Pauli Virtanen:
> hci_conn_cleanup is no longer needed, so move the code back to

Why is it no longer needed?

> hci_conn_del to keep the hci_conn teardown in a single place.
> 
> This undoes commit b958f9a3e877 ("Bluetooth: Fix reference counting for
> LE-scan based connections"), but keeps the current order of cleanup
> operations.
> 
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>   net/bluetooth/hci_conn.c | 78 +++++++++++++++++-----------------------
>   1 file changed, 33 insertions(+), 45 deletions(-)

[…]


Kind regards,

Paul

Re: [PATCH RFC 6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn

[Luiz Augusto von Dentz]

Hi Pauli,

On Wed, Jul 26, 2023 at 2:37 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Calling hci_conn_del in __iso_sock_close is invalid. It needs
> hdev->lock, but it cannot be acquired there due to lock ordering.
>
> Fix this by doing cleanup via hci_conn_drop.
>
> Return hci_conn with refcount 1 from hci_bind_cis and hci_connect_cis,
> so that the iso_conn always holds one reference.  This also fixes
> refcounting when error handling.
>
> Since hci_conn_abort shall handle termination of connections in any
> state properly, we can handle BT_CONNECT socket state in the same way as
> BT_CONNECTED.
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  net/bluetooth/hci_conn.c |  5 +++++
>  net/bluetooth/iso.c      | 14 +-------------
>  2 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index ba339a0eb851..33fbdc8e0748 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1901,6 +1901,8 @@ struct hci_conn *hci_bind_cis(struct hci_dev *hdev, bdaddr_t *dst,
>                 return ERR_PTR(-EINVAL);
>         }
>
> +       hci_conn_hold(cis);
> +
>         cis->iso_qos = *qos;
>         cis->state = BT_BOUND;
>
> @@ -2254,6 +2256,9 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
>                 return ERR_PTR(-ENOLINK);
>         }
>
> +       /* Link takes the refcount */
> +       hci_conn_drop(cis);
> +
>         cis->state = BT_CONNECT;
>
>         hci_le_create_cis_pending(hdev);
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index cbe3299b4a41..358954bfbb32 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -628,6 +628,7 @@ static void __iso_sock_close(struct sock *sk)
>                 iso_sock_cleanup_listen(sk);
>                 break;
>
> +       case BT_CONNECT:
>         case BT_CONNECTED:
>         case BT_CONFIG:
>                 if (iso_pi(sk)->conn->hcon) {
> @@ -643,19 +644,6 @@ static void __iso_sock_close(struct sock *sk)
>                 break;
>
>         case BT_CONNECT2:
> -               iso_chan_del(sk, ECONNRESET);
> -               break;
> -       case BT_CONNECT:
> -               /* In case of DEFER_SETUP the hcon would be bound to CIG which
> -                * needs to be removed so just call hci_conn_del so the cleanup
> -                * callback do what is needed.
> -                */
> -               if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags) &&
> -                   iso_pi(sk)->conn->hcon) {
> -                       hci_conn_del(iso_pi(sk)->conn->hcon);
> -                       iso_pi(sk)->conn->hcon = NULL;
> -               }
> -
>                 iso_chan_del(sk, ECONNRESET);
>                 break;
>         case BT_DISCONN:
> --
> 2.41.0

I guess this sort of fix can be sent separately which I guess helps
here since we can prioritize the ones that don't have side effects.

-- 
Luiz Augusto von Dentz

Re: [PATCH RFC 5/6] Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting

[Luiz Augusto von Dentz]

Hi Pauli,

On Wed, Jul 26, 2023 at 2:43 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Dropped CIS that are in state BT_OPEN/BT_BOUND, and in state BT_CONNECT
> with HCI_CONN_CREATE_CIS unset, should be cleaned up immediately.
> Closing CIS ISO sockets should result to the hci_conn be deleted, so
> that potentially pending CIG removal can run.
>
> hci_abort_conn cannot refer to them by handle, since their handle is
> still unset if Set CIG Parameters has not yet completed.
>
> This fixes CIS not being terminated if the socket is shut down
> immediately after connection, so that the hci_abort_conn runs before Set
> CIG Parameters completes. See new BlueZ test "ISO Connect Close - Success"
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  net/bluetooth/hci_sync.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 101548fe81da..3926213c29e6 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5339,6 +5339,10 @@ static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn,
>                 if (test_bit(HCI_CONN_CREATE_CIS, &conn->flags))
>                         return hci_disconnect_sync(hdev, conn, reason);
>
> +               /* CIS with no Create CIS sent have nothing to cancel */
> +               if (bacmp(&conn->dst, BDADDR_ANY))
> +                       return HCI_ERROR_LOCAL_HOST_TERM;
> +
>                 /* There is no way to cancel a BIS without terminating the BIG
>                  * which is done later on connection cleanup.
>                  */
> @@ -5426,9 +5430,17 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
>         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))
> +               /* Cleanup failed CIS, and BIS that failed to be established */
> +               if (bacmp(&conn->dst, BDADDR_ANY) ||
> +                   test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags))

bacmp(&conn->dst, BDADDR_ANY) will match connections other than
ISO_LINK as well so I wonder if this is intentional? If it is then we
need to update the commands to reflect that we are going to call
hci_conn_failed, it seems we didn't call it before but perhaps this is
a side effect of the other changes.

> +                       hci_conn_failed(conn, reason);
> +               break;
> +       case BT_BOUND:
> +               /* Bound CIS should be cleaned up */
> +               if (conn->type == ISO_LINK && bacmp(&conn->dst, BDADDR_ANY))
>                         hci_conn_failed(conn, reason);
> +               else
> +                       conn->state = BT_CLOSED;
>                 break;
>         default:
>                 conn->state = BT_CLOSED;
> --
> 2.41.0
>


-- 
Luiz Augusto von Dentz

Re: [PATCH RFC 6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn

[Pauli Virtanen]

Hi Luiz,

to, 2023-07-27 kello 14:14 -0700, Luiz Augusto von Dentz kirjoitti:
> On Wed, Jul 26, 2023 at 2:37 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Calling hci_conn_del in __iso_sock_close is invalid. It needs
> > hdev->lock, but it cannot be acquired there due to lock ordering.
> > 
> > Fix this by doing cleanup via hci_conn_drop.
> > 
> > Return hci_conn with refcount 1 from hci_bind_cis and hci_connect_cis,
> > so that the iso_conn always holds one reference.  This also fixes
> > refcounting when error handling.
> > 
> > Since hci_conn_abort shall handle termination of connections in any
> > state properly, we can handle BT_CONNECT socket state in the same way as
> > BT_CONNECTED.
> > 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >  net/bluetooth/hci_conn.c |  5 +++++
> >  net/bluetooth/iso.c      | 14 +-------------
> >  2 files changed, 6 insertions(+), 13 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index ba339a0eb851..33fbdc8e0748 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -1901,6 +1901,8 @@ struct hci_conn *hci_bind_cis(struct hci_dev *hdev, bdaddr_t *dst,
> >                 return ERR_PTR(-EINVAL);
> >         }
> > 
> > +       hci_conn_hold(cis);
> > +
> >         cis->iso_qos = *qos;
> >         cis->state = BT_BOUND;
> > 
> > @@ -2254,6 +2256,9 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
> >                 return ERR_PTR(-ENOLINK);
> >         }
> > 
> > +       /* Link takes the refcount */
> > +       hci_conn_drop(cis);
> > +
> >         cis->state = BT_CONNECT;
> > 
> >         hci_le_create_cis_pending(hdev);
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index cbe3299b4a41..358954bfbb32 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -628,6 +628,7 @@ static void __iso_sock_close(struct sock *sk)
> >                 iso_sock_cleanup_listen(sk);
> >                 break;
> > 
> > +       case BT_CONNECT:
> >         case BT_CONNECTED:
> >         case BT_CONFIG:
> >                 if (iso_pi(sk)->conn->hcon) {
> > @@ -643,19 +644,6 @@ static void __iso_sock_close(struct sock *sk)
> >                 break;
> > 
> >         case BT_CONNECT2:
> > -               iso_chan_del(sk, ECONNRESET);
> > -               break;
> > -       case BT_CONNECT:
> > -               /* In case of DEFER_SETUP the hcon would be bound to CIG which
> > -                * needs to be removed so just call hci_conn_del so the cleanup
> > -                * callback do what is needed.
> > -                */
> > -               if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags) &&
> > -                   iso_pi(sk)->conn->hcon) {
> > -                       hci_conn_del(iso_pi(sk)->conn->hcon);
> > -                       iso_pi(sk)->conn->hcon = NULL;
> > -               }
> > -
> >                 iso_chan_del(sk, ECONNRESET);
> >                 break;
> >         case BT_DISCONN:
> > --
> > 2.41.0
> 
> I guess this sort of fix can be sent separately which I guess helps
> here since we can prioritize the ones that don't have side effects.

Right, I can send these separately in the actual patch series.

This one requires hci_conn_abort deletes conns with no handle yet
though, otherwise it introduces failure to cleanup in a race condition.

-- 
Pauli Virtanen

Re: [PATCH RFC 5/6] Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting

[Pauli Virtanen]

to, 2023-07-27 kello 14:23 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Wed, Jul 26, 2023 at 2:43 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Dropped CIS that are in state BT_OPEN/BT_BOUND, and in state BT_CONNECT
> > with HCI_CONN_CREATE_CIS unset, should be cleaned up immediately.
> > Closing CIS ISO sockets should result to the hci_conn be deleted, so
> > that potentially pending CIG removal can run.
> > 
> > hci_abort_conn cannot refer to them by handle, since their handle is
> > still unset if Set CIG Parameters has not yet completed.
> > 
> > This fixes CIS not being terminated if the socket is shut down
> > immediately after connection, so that the hci_abort_conn runs before Set
> > CIG Parameters completes. See new BlueZ test "ISO Connect Close - Success"
> > 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >  net/bluetooth/hci_sync.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index 101548fe81da..3926213c29e6 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -5339,6 +5339,10 @@ static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn,
> >                 if (test_bit(HCI_CONN_CREATE_CIS, &conn->flags))
> >                         return hci_disconnect_sync(hdev, conn, reason);
> > 
> > +               /* CIS with no Create CIS sent have nothing to cancel */
> > +               if (bacmp(&conn->dst, BDADDR_ANY))
> > +                       return HCI_ERROR_LOCAL_HOST_TERM;
> > +
> >                 /* There is no way to cancel a BIS without terminating the BIG
> >                  * which is done later on connection cleanup.
> >                  */
> > @@ -5426,9 +5430,17 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
> >         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))
> > +               /* Cleanup failed CIS, and BIS that failed to be established */
> > +               if (bacmp(&conn->dst, BDADDR_ANY) ||
> > +                   test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags))
> 
> bacmp(&conn->dst, BDADDR_ANY) will match connections other than
> ISO_LINK as well so I wonder if this is intentional? If it is then we
> need to update the commands to reflect that we are going to call
> hci_conn_failed, it seems we didn't call it before but perhaps this is
> a side effect of the other changes.

Ack, this was supposed to only do it for ISO_LINK. Whether it makes
sense for all conn types would need more careful look.

Earlier, for BT_OPEN we only call hci_conn_failed if that BIS flag was
set.

> 
> > +                       hci_conn_failed(conn, reason);
> > +               break;
> > +       case BT_BOUND:
> > +               /* Bound CIS should be cleaned up */
> > +               if (conn->type == ISO_LINK && bacmp(&conn->dst, BDADDR_ANY))
> >                         hci_conn_failed(conn, reason);
> > +               else
> > +                       conn->state = BT_CLOSED;
> >                 break;
> >         default:
> >                 conn->state = BT_CLOSED;
> > --
> > 2.41.0
> > 
> 
> 

Re: [PATCH RFC 6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn

[Luiz Augusto von Dentz]

Hi Pauli,

On Thu, Jul 27, 2023 at 2:35 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> to, 2023-07-27 kello 14:14 -0700, Luiz Augusto von Dentz kirjoitti:
> > On Wed, Jul 26, 2023 at 2:37 PM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > Calling hci_conn_del in __iso_sock_close is invalid. It needs
> > > hdev->lock, but it cannot be acquired there due to lock ordering.
> > >
> > > Fix this by doing cleanup via hci_conn_drop.
> > >
> > > Return hci_conn with refcount 1 from hci_bind_cis and hci_connect_cis,
> > > so that the iso_conn always holds one reference.  This also fixes
> > > refcounting when error handling.
> > >
> > > Since hci_conn_abort shall handle termination of connections in any
> > > state properly, we can handle BT_CONNECT socket state in the same way as
> > > BT_CONNECTED.
> > >
> > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > ---
> > >  net/bluetooth/hci_conn.c |  5 +++++
> > >  net/bluetooth/iso.c      | 14 +-------------
> > >  2 files changed, 6 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > > index ba339a0eb851..33fbdc8e0748 100644
> > > --- a/net/bluetooth/hci_conn.c
> > > +++ b/net/bluetooth/hci_conn.c
> > > @@ -1901,6 +1901,8 @@ struct hci_conn *hci_bind_cis(struct hci_dev *hdev, bdaddr_t *dst,
> > >                 return ERR_PTR(-EINVAL);
> > >         }
> > >
> > > +       hci_conn_hold(cis);
> > > +
> > >         cis->iso_qos = *qos;
> > >         cis->state = BT_BOUND;
> > >
> > > @@ -2254,6 +2256,9 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
> > >                 return ERR_PTR(-ENOLINK);
> > >         }
> > >
> > > +       /* Link takes the refcount */
> > > +       hci_conn_drop(cis);
> > > +
> > >         cis->state = BT_CONNECT;
> > >
> > >         hci_le_create_cis_pending(hdev);
> > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > index cbe3299b4a41..358954bfbb32 100644
> > > --- a/net/bluetooth/iso.c
> > > +++ b/net/bluetooth/iso.c
> > > @@ -628,6 +628,7 @@ static void __iso_sock_close(struct sock *sk)
> > >                 iso_sock_cleanup_listen(sk);
> > >                 break;
> > >
> > > +       case BT_CONNECT:
> > >         case BT_CONNECTED:
> > >         case BT_CONFIG:
> > >                 if (iso_pi(sk)->conn->hcon) {
> > > @@ -643,19 +644,6 @@ static void __iso_sock_close(struct sock *sk)
> > >                 break;
> > >
> > >         case BT_CONNECT2:
> > > -               iso_chan_del(sk, ECONNRESET);
> > > -               break;
> > > -       case BT_CONNECT:
> > > -               /* In case of DEFER_SETUP the hcon would be bound to CIG which
> > > -                * needs to be removed so just call hci_conn_del so the cleanup
> > > -                * callback do what is needed.
> > > -                */
> > > -               if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags) &&
> > > -                   iso_pi(sk)->conn->hcon) {
> > > -                       hci_conn_del(iso_pi(sk)->conn->hcon);
> > > -                       iso_pi(sk)->conn->hcon = NULL;
> > > -               }
> > > -
> > >                 iso_chan_del(sk, ECONNRESET);
> > >                 break;
> > >         case BT_DISCONN:
> > > --
> > > 2.41.0
> >
> > I guess this sort of fix can be sent separately which I guess helps
> > here since we can prioritize the ones that don't have side effects.
>
> Right, I can send these separately in the actual patch series.
>
> This one requires hci_conn_abort deletes conns with no handle yet
> though, otherwise it introduces failure to cleanup in a race condition.

I thought we need to lookup by handle to avoid races as well, or are
you doing that because the handle could be updated in the meantime?
Perhaps we could store the temporary handles in case the connection
handles get updated then it can still be looked up by its temporary
handle, either that or we disregard updates to handle when they're in
the process of being aborted.


-- 
Luiz Augusto von Dentz

Re: [PATCH RFC 6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn

[Pauli Virtanen]

Hi Luiz,

to, 2023-07-27 kello 17:27 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Thu, Jul 27, 2023 at 2:35 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Hi Luiz,
> > 
> > to, 2023-07-27 kello 14:14 -0700, Luiz Augusto von Dentz kirjoitti:
> > > On Wed, Jul 26, 2023 at 2:37 PM Pauli Virtanen <pav@iki.fi> wrote:
> > > > 
> > > > Calling hci_conn_del in __iso_sock_close is invalid. It needs
> > > > hdev->lock, but it cannot be acquired there due to lock ordering.
> > > > 
> > > > Fix this by doing cleanup via hci_conn_drop.
> > > > 
> > > > Return hci_conn with refcount 1 from hci_bind_cis and hci_connect_cis,
> > > > so that the iso_conn always holds one reference.  This also fixes
> > > > refcounting when error handling.
> > > > 
> > > > Since hci_conn_abort shall handle termination of connections in any
> > > > state properly, we can handle BT_CONNECT socket state in the same way as
> > > > BT_CONNECTED.
> > > > 
> > > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > > ---
> > > >  net/bluetooth/hci_conn.c |  5 +++++
> > > >  net/bluetooth/iso.c      | 14 +-------------
> > > >  2 files changed, 6 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > > > index ba339a0eb851..33fbdc8e0748 100644
> > > > --- a/net/bluetooth/hci_conn.c
> > > > +++ b/net/bluetooth/hci_conn.c
> > > > @@ -1901,6 +1901,8 @@ struct hci_conn *hci_bind_cis(struct hci_dev *hdev, bdaddr_t *dst,
> > > >                 return ERR_PTR(-EINVAL);
> > > >         }
> > > > 
> > > > +       hci_conn_hold(cis);
> > > > +
> > > >         cis->iso_qos = *qos;
> > > >         cis->state = BT_BOUND;
> > > > 
> > > > @@ -2254,6 +2256,9 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
> > > >                 return ERR_PTR(-ENOLINK);
> > > >         }
> > > > 
> > > > +       /* Link takes the refcount */
> > > > +       hci_conn_drop(cis);
> > > > +
> > > >         cis->state = BT_CONNECT;
> > > > 
> > > >         hci_le_create_cis_pending(hdev);
> > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > > > index cbe3299b4a41..358954bfbb32 100644
> > > > --- a/net/bluetooth/iso.c
> > > > +++ b/net/bluetooth/iso.c
> > > > @@ -628,6 +628,7 @@ static void __iso_sock_close(struct sock *sk)
> > > >                 iso_sock_cleanup_listen(sk);
> > > >                 break;
> > > > 
> > > > +       case BT_CONNECT:
> > > >         case BT_CONNECTED:
> > > >         case BT_CONFIG:
> > > >                 if (iso_pi(sk)->conn->hcon) {
> > > > @@ -643,19 +644,6 @@ static void __iso_sock_close(struct sock *sk)
> > > >                 break;
> > > > 
> > > >         case BT_CONNECT2:
> > > > -               iso_chan_del(sk, ECONNRESET);
> > > > -               break;
> > > > -       case BT_CONNECT:
> > > > -               /* In case of DEFER_SETUP the hcon would be bound to CIG which
> > > > -                * needs to be removed so just call hci_conn_del so the cleanup
> > > > -                * callback do what is needed.
> > > > -                */
> > > > -               if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags) &&
> > > > -                   iso_pi(sk)->conn->hcon) {
> > > > -                       hci_conn_del(iso_pi(sk)->conn->hcon);
> > > > -                       iso_pi(sk)->conn->hcon = NULL;
> > > > -               }
> > > > -
> > > >                 iso_chan_del(sk, ECONNRESET);
> > > >                 break;
> > > >         case BT_DISCONN:
> > > > --
> > > > 2.41.0
> > > 
> > > I guess this sort of fix can be sent separately which I guess helps
> > > here since we can prioritize the ones that don't have side effects.
> > 
> > Right, I can send these separately in the actual patch series.
> > 
> > This one requires hci_conn_abort deletes conns with no handle yet
> > though, otherwise it introduces failure to cleanup in a race condition.
> 
> I thought we need to lookup by handle to avoid races as well, or are
> you doing that because the handle could be updated in the meantime?
> Perhaps we could store the temporary handles in case the connection
> handles get updated then it can still be looked up by its temporary
> handle, either that or we disregard updates to handle when they're in
> the process of being aborted.

Would it be simpler to hold hci_conn_get, and then in the hci_sync
callback check if the conn is still in conn_hash? The purpose of the
HCI_CONN_DELETED flag in the series is to make this check easy.

Also, I don't think just the handle thing protects you from races, as
noted in the cover letter. AFAIK, hci_sync.c callbacks and hci_event.c
processing can run at the same time on different CPU (since they run
from different workqueues), so you have to lock.

Moreover, handle lookup might pick up different connection than
intended if a handle was reused. hci_abort_conn_sync might run quite a
bit after hci_abort_conn queues it and there might be stuff like
another abort and Set CIG Parametrers in the queue...

-- 
Pauli Virtanen

Re: [PATCH RFC 0/6] Locking in hci_sync

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu, 27 Jul 2023 00:25:20 +0300 you wrote:
> The current guarantees that a given hci_conn is not freed concurrently
> appear to be:
> 
> - hci_dev_lock is held, or,
> - rcu_read_lock is held  (doesn't guarantee valid conn state), or,
> - hci_conn_get refcount held (doesn't guarantee valid conn state),
> - current task is running from hdev->workqueue (which is ordered)
> 
> [...]

Here is the summary with links:
  - [RFC,1/6] Bluetooth: hci_conn: hci_conn_cleanup is not needed, combine with del
    (no matching commit)
  - [RFC,2/6] Bluetooth: hci_conn: add hci_conn_is_alive
    (no matching commit)
  - [RFC,3/6] Bluetooth: hci_sync: add hci_conn_sync_queue and hci_cmd_sync_dev_(un)lock
    (no matching commit)
  - [RFC,4/6] Bluetooth: hci_sync: fix locking in hci_conn_abort and hci_disconnect_all_sync
    (no matching commit)
  - [RFC,5/6] Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting
    (no matching commit)
  - [RFC,6/6] Bluetooth: ISO: handle bound CIS cleanup via hci_conn
    https://git.kernel.org/bluetooth/bluetooth-next/c/2dfe76d58d3a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html