From b6dd3ef91e9b340edb83f8028d53a4f1c2c8b27a Mon Sep 17 00:00:00 2001 From: Jonathan Rico Date: Thu, 8 Feb 2024 14:35:13 +0100 Subject: [PATCH 1/2] Bluetooth: conn: check for disconnected earlier when sending Verify the connection is active before popping the buffer from the TX queue. The current behavior enables a race condition between `create_frag` and the connection being torn down, as `buf` can be popped from the TX queue but not destroyed by `bt_conn_process_tx`. In that case, `buf` will be leaked. Original analysis and fix proposal by @watsug. Signed-off-by: Jonathan Rico Reported-by: Adam Augustyn --- subsys/bluetooth/host/conn.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index 36b2692ee9d4..f03a41c6a00b 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -586,12 +586,6 @@ static int do_send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags unsigned int key; int err = 0; - /* Check for disconnection while waiting for pkts_sem */ - if (conn->state != BT_CONN_CONNECTED) { - err = -ENOTCONN; - goto fail; - } - LOG_DBG("conn %p buf %p len %u flags 0x%02x", conn, buf, buf->len, flags); @@ -669,6 +663,14 @@ static int send_frag(struct bt_conn *conn, return -ENOBUFS; } + /* Check for disconnection. It can't be done higher up (ie `send_buf`) + * as `create_frag` blocks with K_FOREVER and the connection could + * change state after waiting. + */ + if (conn->state != BT_CONN_CONNECTED) { + return -ENOTCONN; + } + /* Add the data to the buffer */ if (frag) { uint16_t frag_len = MIN(conn_mtu(conn), net_buf_tailroom(frag)); From bde60d52126307f61fcea473d72da5b4ae343c8f Mon Sep 17 00:00:00 2001 From: Jonathan Rico Date: Thu, 8 Feb 2024 14:37:39 +0100 Subject: [PATCH 2/2] Bluetooth: conn: document obfuscated function: `send_buf()` It is unclear from a cursory glance at the code what the caller of `send_buf(buf)` should do with `buf` based on the returned error codes. Document when ownership is and isn't transferred to `send_buf()`. Signed-off-by: Jonathan Rico --- subsys/bluetooth/host/conn.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index f03a41c6a00b..344b10de5b86 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -722,6 +722,23 @@ static struct net_buf *create_frag(struct bt_conn *conn, struct net_buf *buf) return frag; } +/* Tentatively send a buffer to the HCI driver. + * + * This is designed to be async, as in most failures due to lack of resources + * are not fatal. The caller should call `send_buf()` again later. + * + * Return values: + * + * - 0: `buf` sent. `buf` ownership transferred to lower layers. + * + * - -EIO: buffer failed to send due to HCI error. `buf` ownership returned to + * caller BUT `buf` is popped from the TX queue. The caller shall destroy + * `buf` and its TX context. + * + * - Any other error: buffer failed to send. `buf` ownership returned to caller + * and `buf` is still the head of the TX queue + * + */ static int send_buf(struct bt_conn *conn, struct net_buf *buf) { struct net_buf *frag; @@ -931,6 +948,12 @@ void bt_conn_process_tx(struct bt_conn *conn) err = send_buf(conn, buf); net_buf_unref(buf); + /* HCI driver error. `buf` may have been popped from `tx_queue` and + * should be destroyed. + * + * TODO: In that case we might want to disable Bluetooth or at the very + * least tear down the connection. + */ if (err == -EIO) { struct bt_conn_tx *tx = tx_data(buf)->tx;