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 1/5] Bluetooth: hci_conn: add hci_conn_is_alive
Date:   Fri, 23 Jun 2023 20:18:38 +0300
Message-ID: <45455ee45ccb3313618a48c01be714e14d372257.1687525956.git.pav@iki.fi>
In-Reply-To: <cover.1687525956.git.pav@iki.fi>
References: <cover.1687525956.git.pav@iki.fi>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

A delayed operation such as hci_sync on a given hci_conn needs to take
hci_conn_get, so that the hci_conn doesn't get freed in the meantime.
This does not guarantee the conn is still alive in a valid state, as it
may be cleaned up in the meantime, so one needs to check if it is still
in conn_hash to know if it's still alive.

Simplify this alive check, using HCI_CONN_DELETED flag. This is also
meaningful with RCU lock only, but with slightly 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 cannot complete before rcu_read_unlock.

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

Notes:
    This probably can be done with RCU primitives setting list.prev, but
    that's maybe more magical...

 include/net/bluetooth/hci_core.h | 18 ++++++++++++++++++
 net/bluetooth/hci_conn.c         | 10 +---------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 05a9b3ab3f56..cab39bdd0592 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -978,6 +978,7 @@ enum {
 	HCI_CONN_PER_ADV,
 	HCI_CONN_BIG_CREATED,
 	HCI_CONN_CREATE_CIS,
+	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:
@@ -1023,6 +1025,10 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c)
 static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c)
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
+	bool deleted;
+
+	deleted = test_and_set_bit(HCI_CONN_DELETED, &c->flags);
+	WARN_ON(deleted);
 
 	list_del_rcu(&c->list);
 	synchronize_rcu();
@@ -1049,6 +1055,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 62a7ccfdfe63..d489a4829be7 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -183,21 +183,13 @@ static void le_scan_cleanup(struct work_struct *work)
 	struct hci_conn *conn = container_of(work, struct hci_conn,
 					     le_scan_cleanup);
 	struct hci_dev *hdev = conn->hdev;
-	struct hci_conn *c = NULL;
 
 	BT_DBG("%s hcon %p", hdev->name, conn);
 
 	hci_dev_lock(hdev);
 
 	/* Check that the hci_conn is still around */
-	rcu_read_lock();
-	list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) {
-		if (c == conn)
-			break;
-	}
-	rcu_read_unlock();
-
-	if (c == conn) {
+	if (hci_conn_is_alive(hdev, conn)) {
 		hci_connect_le_scan_cleanup(conn, 0x00);
 		hci_conn_cleanup(conn);
 	}
-- 
2.41.0