Bluetooth: host: Fix ATT indication deadlock in RX thread

subsys/bluetooth/host/Kconfig.gatt

@@ -24,16 +24,6 @@ config BT_ATT_PREPARE_COUNT
 	  Number of buffers available for ATT prepare write, setting
 	  this to 0 disables GATT long/reliable writes.
 
-config BT_ATT_TX_MAX
-	int "Maximum number of queued outgoing ATT PDUs"
-	default BT_L2CAP_TX_BUF_COUNT
-	range 1 BT_L2CAP_TX_BUF_COUNT
-	help
-	  Number of ATT PDUs that can be at a single moment queued for
-	  transmission. If the application tries to send more than this
-	  amount the calls will block until an existing queued PDU gets
-	  sent.
-
 config BT_EATT
 	bool "Enhanced ATT Bearers support [EXPERIMENTAL]"
 	depends on BT_L2CAP_ECRED

subsys/bluetooth/host/att.c

@@ -66,7 +66,7 @@ NET_BUF_POOL_DEFINE(prep_pool, CONFIG_BT_ATT_PREPARE_COUNT, BT_ATT_MTU,
 #endif /* CONFIG_BT_ATT_PREPARE_COUNT */
 
 K_MEM_SLAB_DEFINE(req_slab, sizeof(struct bt_att_req),
-		  CONFIG_BT_ATT_TX_MAX, __alignof__(struct bt_att_req));
+		  CONFIG_BT_L2CAP_TX_BUF_COUNT, __alignof__(struct bt_att_req));
 
 enum {
 	ATT_PENDING_RSP,
@@ -88,7 +88,6 @@ struct bt_att_chan {
 	struct bt_att_req	*req;
 	struct k_fifo		tx_queue;
 	struct k_delayed_work	timeout_work;
-	struct k_sem            tx_sem;
 	void (*sent)(struct bt_att_chan *chan);
 	sys_snode_t		node;
 };
@@ -113,21 +112,6 @@ K_MEM_SLAB_DEFINE(chan_slab, sizeof(struct bt_att_chan),
 		  __alignof__(struct bt_att_chan));
 static struct bt_att_req cancel;
 
-static void att_req_destroy(struct bt_att_req *req)
-{
-	BT_DBG("req %p", req);
-
-	if (req->buf) {
-		net_buf_unref(req->buf);
-	}
-
-	if (req->destroy) {
-		req->destroy(req);
-	}
-
-	bt_att_req_free(req);
-}
-
 typedef void (*bt_att_chan_sent_t)(struct bt_att_chan *chan);
 
 static bt_att_chan_sent_t chan_cb(struct net_buf *buf);
@@ -205,19 +189,14 @@ static int chan_send(struct bt_att_chan *chan, struct net_buf *buf,
 
 	chan->sent = cb ? cb : chan_cb(buf);
 
-	/* Take a ref since bt_l2cap_send_cb takes ownership of the buffer */
 	err = bt_l2cap_send_cb(chan->att->conn, BT_L2CAP_CID_ATT,
-				net_buf_ref(buf), att_cb(chan->sent),
-				&chan->chan.chan);
-	if (!err) {
-		net_buf_unref(buf);
-		return 0;
+			       buf, att_cb(chan->sent),
+			       &chan->chan.chan);
+	if (err) {
+		net_buf_simple_restore(&buf->b, &state);
 	}
 
-	net_buf_simple_restore(&buf->b, &state);
-
 	return err;
-
 }
 
 static int process_queue(struct bt_att_chan *chan, struct k_fifo *queue)
@@ -243,6 +222,7 @@ static int process_queue(struct bt_att_chan *chan, struct k_fifo *queue)
 /* Send requests without taking tx_sem */
 static int chan_req_send(struct bt_att_chan *chan, struct bt_att_req *req)
 {
+	struct net_buf *buf;
 	int err;
 
 	if (chan->chan.tx.mtu < net_buf_frags_len(req->buf)) {
@@ -254,19 +234,14 @@ static int chan_req_send(struct bt_att_chan *chan, struct bt_att_req *req)
 
 	chan->req = req;
 
-	/* Save request state so it can be resent */
-	net_buf_simple_save(&req->buf->b, &req->state);
+	/* Release since bt_l2cap_send_cb takes ownership of the buffer */
+	buf = req->buf;
+	req->buf = NULL;
 
-	/* Keep a reference for resending the req in case the security
-	 * needs to be changed.
-	 */
-	err = chan_send(chan, net_buf_ref(req->buf), NULL);
+	err = chan_send(chan, buf, NULL);
 	if (err) {
-		/* Drop the extra reference if buffer could not be sent but
-		 * don't reset the buffer as it will likelly be pushed back to
-		 * request queue to be send later.
-		 */
-		net_buf_unref(req->buf);
+		/* We still have the ownership of the buffer */
+		req->buf = buf;
 	}
 
 	return err;
@@ -314,12 +289,7 @@ static void bt_att_sent(struct bt_l2cap_chan *ch)
 	}
 
 	/* Process global queue */
-	err = process_queue(chan, &att->tx_queue);
-	if (!err) {
-		return;
-	}
-
-	k_sem_give(&chan->tx_sem);
+	(void)process_queue(chan, &att->tx_queue);
 }
 
 static void chan_cfm_sent(struct bt_att_chan *chan)
@@ -463,13 +433,6 @@ static int bt_att_chan_send(struct bt_att_chan *chan, struct net_buf *buf,
 	BT_DBG("chan %p flags %u code 0x%02x", chan, atomic_get(chan->flags),
 	       hdr->code);
 
-	/* Don't use tx_sem if caller has set it own callback */
-	if (!cb) {
-		if (k_sem_take(&chan->tx_sem, K_NO_WAIT) < 0) {
-			return -EAGAIN;
-		}
-	}
-
 	return chan_send(chan, buf, cb);
 }
 
@@ -564,25 +527,14 @@ static uint8_t att_mtu_req(struct bt_att_chan *chan, struct net_buf *buf)
 static int bt_att_chan_req_send(struct bt_att_chan *chan,
 				struct bt_att_req *req)
 {
-	int err;
-
 	__ASSERT_NO_MSG(chan);
 	__ASSERT_NO_MSG(req);
 	__ASSERT_NO_MSG(req->func);
 	__ASSERT_NO_MSG(!chan->req);
 
 	BT_DBG("req %p", req);
 
-	if (k_sem_take(&chan->tx_sem, K_NO_WAIT) < 0) {
-		return -EAGAIN;
-	}
-
-	err = chan_req_send(chan, req);
-	if (err < 0) {
-		k_sem_give(&chan->tx_sem);
-	}
-
-	return err;
+	return chan_req_send(chan, req);
 }
 
 static void att_process(struct bt_att *att)
@@ -634,19 +586,13 @@ static uint8_t att_handle_rsp(struct bt_att_chan *chan, void *pdu, uint16_t len,
 		goto process;
 	}
 
-	/* Release original buffer */
-	if (chan->req->buf) {
-		net_buf_unref(chan->req->buf);
-		chan->req->buf = NULL;
-	}
-
 	/* Reset func so it can be reused by the callback */
 	func = chan->req->func;
 	chan->req->func = NULL;
 	params = chan->req->user_data;
 
 	/* free allocated request so its memory can be reused */
-	att_req_destroy(chan->req);
+	bt_att_req_free(chan->req);
 	chan->req = NULL;
 
 process:
@@ -2067,21 +2013,11 @@ static uint8_t att_error_rsp(struct bt_att_chan *chan, struct net_buf *buf)
 		goto done;
 	}
 
-	if (chan->req->buf) {
-		/* Restore state to be resent */
-		net_buf_simple_restore(&chan->req->buf->b, &chan->req->state);
-	}
-
 	err = rsp->error;
 #if defined(CONFIG_BT_SMP)
-	if (chan->req->retrying) {
-		goto done;
-	}
-
-	/* Check if security needs to be changed */
+	/* Check if error can be handled by elevating security. */
 	if (!att_change_security(chan->chan.chan.conn, err)) {
 		chan->req->retrying = true;
-		/* Wait security_changed: TODO: Handle fail case */
 		return 0;
 	}
 #endif /* CONFIG_BT_SMP */
@@ -2562,7 +2498,7 @@ static void att_reset(struct bt_att *att)
 				  req->user_data);
 		}
 
-		att_req_destroy(req);
+		bt_att_req_free(req);
 	}
 
 	k_mem_slab_free(&att_slab, (void **)&att);
@@ -2571,17 +2507,11 @@ static void att_reset(struct bt_att *att)
 static void att_chan_detach(struct bt_att_chan *chan)
 {
 	struct net_buf *buf;
-	int i;
 
 	BT_DBG("chan %p", chan);
 
 	sys_slist_find_and_remove(&chan->att->chans, &chan->node);
 
-	/* Ensure that any waiters are woken up */
-	for (i = 0; i < CONFIG_BT_ATT_TX_MAX; i++) {
-		k_sem_give(&chan->tx_sem);
-	}
-
 	/* Release pending buffers */
 	while ((buf = net_buf_get(&chan->tx_queue, K_NO_WAIT))) {
 		net_buf_unref(buf);
@@ -2684,12 +2614,43 @@ static void bt_att_disconnected(struct bt_l2cap_chan *chan)
 }
 
 #if defined(CONFIG_BT_SMP)
+static uint8_t att_req_retry(struct bt_att_chan *att_chan)
+{
+	struct bt_att_req *req = att_chan->req;
+	struct net_buf *buf;
+
+	/* Resend buffer */
+	if (!req->encode) {
+		/* This request does not support resending */
+		return BT_ATT_ERR_AUTHENTICATION;
+	}
+
+
+	buf = bt_att_chan_create_pdu(att_chan, req->att_op, req->len);
+	if (!buf) {
+		return BT_ATT_ERR_UNLIKELY;
+	}
+
+	if (req->encode(buf, req->len, req->user_data)) {
+		net_buf_unref(buf);
+		return BT_ATT_ERR_UNLIKELY;
+	}
+
+	if (chan_send(att_chan, buf, NULL)) {
+		net_buf_unref(buf);
+		return BT_ATT_ERR_UNLIKELY;
+	}
+
+	return BT_ATT_ERR_SUCCESS;
+}
+
 static void bt_att_encrypt_change(struct bt_l2cap_chan *chan,
 				  uint8_t hci_status)
 {
 	struct bt_att_chan *att_chan = ATT_CHAN(chan);
 	struct bt_l2cap_le_chan *ch = BT_L2CAP_LE_CHAN(chan);
 	struct bt_conn *conn = ch->chan.conn;
+	uint8_t err;
 
 	BT_DBG("chan %p conn %p handle %u sec_level 0x%02x status 0x%02x", ch,
 	       conn, conn->handle, conn->sec_level, hci_status);
@@ -2724,11 +2685,11 @@ static void bt_att_encrypt_change(struct bt_l2cap_chan *chan,
 
 	BT_DBG("Retrying");
 
-	/* Resend buffer */
-	bt_att_chan_send_rsp(att_chan, att_chan->req->buf,
-			     chan_cb(att_chan->req->buf));
-
-	att_chan->req->buf = NULL;
+	err = att_req_retry(att_chan);
+	if (err) {
+		BT_DBG("Retry failed (%d)", err);
+		att_handle_rsp(att_chan, NULL, 0, err);
+	}
 }
 #endif /* CONFIG_BT_SMP */
 
@@ -2811,7 +2772,6 @@ static struct bt_att_chan *att_chan_new(struct bt_att *att, atomic_val_t flags)
 	(void)memset(chan, 0, sizeof(*chan));
 	chan->chan.chan.ops = &ops;
 	k_fifo_init(&chan->tx_queue);
-	k_sem_init(&chan->tx_sem, CONFIG_BT_ATT_TX_MAX, CONFIG_BT_ATT_TX_MAX);
 	atomic_set(chan->flags, flags);
 	chan->att = att;
 
@@ -2967,6 +2927,7 @@ struct bt_att_req *bt_att_req_alloc(k_timeout_t timeout)
 
 	/* Reserve space for request */
 	if (k_mem_slab_alloc(&req_slab, (void **)&req, timeout)) {
+		BT_DBG("No space for req");
 		return NULL;
 	}
 
@@ -2981,6 +2942,11 @@ void bt_att_req_free(struct bt_att_req *req)
 {
 	BT_DBG("req %p", req);
 
+	if (req->buf) {
+		net_buf_unref(req->buf);
+		req->buf = NULL;
+	}
+
 	k_mem_slab_free(&req_slab, (void **)&req);
 }
 
@@ -3038,8 +3004,6 @@ int bt_att_req_send(struct bt_conn *conn, struct bt_att_req *req)
 
 	att = att_get(conn);
 	if (!att) {
-		net_buf_unref(req->buf);
-		req->buf = NULL;
 		return -ENOTCONN;
 	}
 
@@ -3072,7 +3036,7 @@ static bool bt_att_chan_req_cancel(struct bt_att_chan *chan,
 
 	chan->req = &cancel;
 
-	att_req_destroy(req);
+	bt_att_req_free(req);
 
 	return true;
 }
@@ -3103,5 +3067,5 @@ void bt_att_req_cancel(struct bt_conn *conn, struct bt_att_req *req)
 	/* Remove request from the list */
 	sys_slist_find_and_remove(&att->reqs, &req->node);
 
-	att_req_destroy(req);
+	bt_att_req_free(req);
 }

subsys/bluetooth/host/att_internal.h

@@ -262,17 +262,20 @@ struct bt_att_signed_write_cmd {
 typedef void (*bt_att_func_t)(struct bt_conn *conn, uint8_t err,
 			      const void *pdu, uint16_t length,
 			      void *user_data);
-typedef void (*bt_att_destroy_t)(void *user_data);
+
+typedef int (*bt_att_encode_t)(struct net_buf *buf, size_t len,
+			       void *user_data);
 
 /* ATT request context */
 struct bt_att_req {
 	sys_snode_t node;
 	bt_att_func_t func;
-	bt_att_destroy_t destroy;
-	struct net_buf_simple_state state;
 	struct net_buf *buf;
 #if defined(CONFIG_BT_SMP)
-	bool retrying;
+	bt_att_encode_t encode;
+	uint8_t retrying : 1;
+	uint8_t att_op;
+	size_t len;
 #endif /* CONFIG_BT_SMP */
 	void *user_data;
 };