bluetooth: host: Use critical section when making channel ready to send

A public API call that sends ACL data — for example, `bt_gatt_notify` —
can be invoked from a preemptive thread context. This API, in turn,
calls the `raise_data_ready` function, which adds an L2CAP channel to
the `l2cap_data_ready` list.

The atomic variable used to create a critical section around
`l2cap_data_ready` did not work, because a preemptive thread can be
pre-empted at any time. This means that regardless of the `atomic_set`
result, we cannot trust it — the thread could be preempted after the
call, and the atomic variable state could be changed (specifically by
the TX processor in this case). The same issue applies to
`bt_conn_data_ready`, which is called next.

When making an L2CAP channel ready for sending data, we need to use a
critical section when appending a channel to the `l2cap_data_ready`
list. The same applies to the `conn_ready` list.

Since cooperative threads can only be rescheduled explicitly, we must
ensure there are no rescheduling points when accessing either the
`l2cap_data_ready` or `conn_ready` lists.

For `l2cap_data_ready`, this occurs in `get_ready_chan`, which is called
from `l2cap_data_pull`, which in turn is called by the TX processor. For
`conn_ready`, it occurs in `get_conn_ready`, which is also called from
the TX processor. Both functions have no rescheduling points when
working with their respective lists, so they do not require a critical
section.

This change removes the atomic variables previously used to create a
critical section for both lists, as they were ineffective. Instead,
`k_sched_lock`/`k_sched_unlock` are used where code may be executed from
a preemptive thread context. The `sys_slist_find` function is used to
check whether a channel or connection is already in the corresponding
list.

Fixes #89705

Signed-off-by: Pavel Vasilyev <[email protected]>

Author: PavelVPV

include/zephyr/bluetooth/l2cap.h

@@ -275,8 +275,6 @@ struct bt_l2cap_le_chan {
 
 	/** @internal To be used with @ref bt_conn.upper_data_ready */
 	sys_snode_t			_pdu_ready;
-	/** @internal To be used with @ref bt_conn.upper_data_ready */
-	atomic_t			_pdu_ready_lock;
 	/** @internal Holds the length of the current PDU/segment */
 	size_t				_pdu_remaining;
 };

subsys/bluetooth/host/conn.c

@@ -872,31 +872,34 @@ static bool should_stop_tx(struct bt_conn *conn)
 
 void bt_conn_data_ready(struct bt_conn *conn)
 {
+	bool added;
+
 	LOG_DBG("DR");
 
-	/* The TX processor will call the `pull_cb` to get the buf */
-	if (!atomic_set(&conn->_conn_ready_lock, 1)) {
-		/* Attach a reference to the `bt_dev.le.conn_ready` list.
-		 *
-		 * This reference will be consumed when the conn is popped off
-		 * the list (in `get_conn_ready`).
-		 *
-		 * The `bt_dev.le.conn_ready` list is accessed by the tx_processor
-		 * which runs in a workqueue, but `bt_conn_data_ready` can be called
-		 * from different threads so we need to make sure that nothing will
-		 * trigger a thread switch while we are manipulating the list since
-		 * sys_slist_*() functions are not thread safe.
-		 */
-		bt_conn_ref(conn);
-		k_sched_lock();
-		sys_slist_append(&bt_dev.le.conn_ready,
-				 &conn->_conn_ready);
-		k_sched_unlock();
-		LOG_DBG("raised");
+	bt_conn_ref(conn);
+
+	/* This function is the only function which accesses conn_ready list  that can be called
+	 * from a preemptive thread context, therefore requires a critical section to ensure that
+	 * the conn_ready list is not modified while we are checking and appending to it.
+	 */
+	k_sched_lock();
+
+	if (!sys_slist_find(&bt_dev.le.conn_ready, &conn->_conn_ready, NULL)) {
+		sys_slist_append(&bt_dev.le.conn_ready, &conn->_conn_ready);
+
+		added = true;
 	} else {
-		LOG_DBG("already in list");
+		added = false;
+	}
+
+	k_sched_unlock();
+
+	if (!added) {
+		bt_conn_unref(conn);
 	}
 
+	LOG_DBG("Connection %p %s conn_ready list", conn, added ? "added to" : "already in");
+
 	/* Kick the TX processor */
 	bt_tx_irq_raise();
 }
@@ -977,7 +980,6 @@ struct bt_conn *get_conn_ready(void)
 			/* Move reference off the list */
 			__ASSERT_NO_MSG(prev != &conn->_conn_ready);
 			sys_slist_remove(&bt_dev.le.conn_ready, prev, &conn->_conn_ready);
-			(void)atomic_set(&conn->_conn_ready_lock, 0);
 
 			/* Append connection to list if it is connected and still has data */
 			if (conn->has_data(conn) && (conn->state == BT_CONN_CONNECTED)) {

subsys/bluetooth/host/conn_internal.h

@@ -324,7 +324,6 @@ struct bt_conn {
 	 * This will be used by the TX processor to then fetch HCI frags from it.
 	 */
 	sys_snode_t		_conn_ready;
-	atomic_t		_conn_ready_lock;
 
 	/* Holds the number of packets that have been sent to the controller but
 	 * not yet ACKd (by receiving an Number of Completed Packets). This

subsys/bluetooth/host/l2cap.c

@@ -353,7 +353,6 @@ static void init_le_chan_private(struct bt_l2cap_le_chan *le_chan)
 #endif /* CONFIG_BT_L2CAP_SEG_RECV */
 #endif /* CONFIG_BT_L2CAP_DYNAMIC_CHANNEL */
 	memset(&le_chan->_pdu_ready, 0, sizeof(le_chan->_pdu_ready));
-	le_chan->_pdu_ready_lock = 0;
 	le_chan->_pdu_remaining = 0;
 }
 
@@ -701,14 +700,28 @@ struct net_buf *bt_l2cap_create_pdu_timeout(struct net_buf_pool *pool,
 
 static void raise_data_ready(struct bt_l2cap_le_chan *le_chan)
 {
-	if (!atomic_set(&le_chan->_pdu_ready_lock, 1)) {
+	__maybe_unused bool added;
+
+	/* This function is the only function which accesses l2cap_data_ready list that can be
+	 * called from a preemptive thread context, therefore requires a critical section to ensure
+	 * that the data ready list is not modified while we are checking and appending to it.
+	 */
+	k_sched_lock();
+
+	if (!sys_slist_find(&le_chan->chan.conn->l2cap_data_ready,
+				 &le_chan->_pdu_ready, NULL)) {
 		sys_slist_append(&le_chan->chan.conn->l2cap_data_ready,
 				 &le_chan->_pdu_ready);
-		LOG_DBG("data ready raised %p", le_chan);
+
+		added = true;
 	} else {
-		LOG_DBG("data ready already %p", le_chan);
+		added = false;
 	}
 
+	k_sched_unlock();
+
+	LOG_DBG("L2CAP channel %p data ready %s", le_chan, added ? "added" : "already added");
+
 	bt_conn_data_ready(le_chan->chan.conn);
 }
 
@@ -720,10 +733,6 @@ static void lower_data_ready(struct bt_l2cap_le_chan *le_chan)
 	LOG_DBG("%p", le_chan);
 
 	__ASSERT_NO_MSG(s == &le_chan->_pdu_ready);
-
-	__maybe_unused atomic_t old = atomic_set(&le_chan->_pdu_ready_lock, 0);
-
-	__ASSERT_NO_MSG(old);
 }
 
 static void cancel_data_ready(struct bt_l2cap_le_chan *le_chan)
@@ -732,9 +741,16 @@ static void cancel_data_ready(struct bt_l2cap_le_chan *le_chan)
 
 	LOG_DBG("%p", le_chan);
 
+	/* Use critical section here as this function can be called from
+	 * a preemptive thread context and we need to ensure that the data ready list is not
+	 * modified while we are removing the channel from it.
+	 */
+	k_sched_lock();
+
 	sys_slist_find_and_remove(&conn->l2cap_data_ready,
 				  &le_chan->_pdu_ready);
-	atomic_set(&le_chan->_pdu_ready_lock, 0);
+
+	k_sched_unlock();
 }
 
 int bt_l2cap_send_pdu(struct bt_l2cap_le_chan *le_chan, struct net_buf *pdu,