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;
From:   Pauli Virtanen <pav@iki.fi>
To:     linux-bluetooth@vger.kernel.org
Cc:     Pauli Virtanen <pav@iki.fi>
Subject: [PATCH RFC 2/6] Bluetooth: hci_conn: add hci_conn_is_alive
Date:   Thu, 27 Jul 2023 00:25:22 +0300
Message-ID: <053df33e4e3b35982b257238edcde451780fc832.1690399379.git.pav@iki.fi>
In-Reply-To: <cover.1690399379.git.pav@iki.fi>
References: <cover.1690399379.git.pav@iki.fi>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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