Bluetooth: att: use a dedicated metadata struct for RSP PDUs

The ATT module has provisions to queue a packet/buffer for sending later if
it can't send it right away. For example if the conn.c tx context
allocation fails.

This unfortunately doesn't work if the buffer can't get allocated in the
first place, or if the ATT metadata can't also be allocated.

The metadata is allocated from a global pool set to the same number as
conn.c TX contexts. That can lead to a situation where other users of ATT
manage to queue a bunch of buffers (e.g. the app spamming GATT
notifications), depleting the number of ATT metadata slots so that none are
available.

When none are available, and we receive an ATT REQ, we try to allocate one,
fail, and drop the buffer (!). That pretty much guarantees an ATT timeout.

As a workaround for this, use a per-channel metadata slot, that is only
used for completing transactions.

Signed-off-by: Jonathan Rico <[email protected]>

subsys/bluetooth/host/att.c

@@ -83,6 +83,18 @@ enum {
 	ATT_NUM_FLAGS,
 };
 
+struct bt_att_tx_meta_data {
+	struct bt_att_chan *att_chan;
+	uint16_t attr_count;
+	bt_gatt_complete_func_t func;
+	void *user_data;
+	enum bt_att_chan_opt chan_opt;
+};
+
+struct bt_att_tx_meta {
+	struct bt_att_tx_meta_data *data;
+};
+
 /* ATT channel specific data */
 struct bt_att_chan {
 	/* Connection this channel is associated with */
@@ -91,6 +103,7 @@ struct bt_att_chan {
 	ATOMIC_DEFINE(flags, ATT_NUM_FLAGS);
 	struct bt_att_req	*req;
 	struct k_fifo		tx_queue;
+	struct bt_att_tx_meta_data rsp_meta;
 	struct k_work_delayable	timeout_work;
 	sys_snode_t		node;
 };
@@ -159,18 +172,6 @@ static struct bt_att_req cancel;
  */
 static k_tid_t att_handle_rsp_thread;
 
-struct bt_att_tx_meta_data {
-	struct bt_att_chan *att_chan;
-	uint16_t attr_count;
-	bt_gatt_complete_func_t func;
-	void *user_data;
-	enum bt_att_chan_opt chan_opt;
-};
-
-struct bt_att_tx_meta {
-	struct bt_att_tx_meta_data *data;
-};
-
 #define bt_att_tx_meta_data(buf) (((struct bt_att_tx_meta *)net_buf_user_data(buf))->data)
 
 static struct bt_att_tx_meta_data tx_meta_data[CONFIG_BT_CONN_TX_MAX];
@@ -192,9 +193,22 @@ static struct bt_att_tx_meta_data *tx_meta_data_alloc(k_timeout_t timeout)
 static inline void tx_meta_data_free(struct bt_att_tx_meta_data *data)
 {
 	__ASSERT_NO_MSG(data);
+	bool alloc_from_global = PART_OF_ARRAY(tx_meta_data, data);
+
+	if (data == &data->att_chan->rsp_meta) {
+		/* "Free-ness" is kept by remote: There can only ever be one
+		 * transaction per-bearer.
+		 */
+		__ASSERT_NO_MSG(!alloc_from_global);
+	} else {
+		__ASSERT_NO_MSG(alloc_from_global);
+	}
 
 	(void)memset(data, 0, sizeof(*data));
-	k_fifo_put(&free_att_tx_meta_data, data);
+
+	if (alloc_from_global) {
+		k_fifo_put(&free_att_tx_meta_data, data);
+	}
 }
 
 static int bt_att_chan_send(struct bt_att_chan *chan, struct net_buf *buf);
@@ -644,6 +658,7 @@ static struct net_buf *bt_att_chan_create_pdu(struct bt_att_chan *chan, uint8_t
 	struct net_buf *buf;
 	struct bt_att_tx_meta_data *data;
 	k_timeout_t timeout;
+	bool is_rsp = false;
 
 	if (len + sizeof(op) > bt_att_mtu(chan)) {
 		LOG_WRN("ATT MTU exceeded, max %u, wanted %zu", bt_att_mtu(chan),
@@ -656,6 +671,7 @@ static struct net_buf *bt_att_chan_create_pdu(struct bt_att_chan *chan, uint8_t
 	case ATT_CONFIRMATION:
 		/* Use a timeout only when responding/confirming */
 		timeout = BT_ATT_TIMEOUT;
+		is_rsp = true;
 		break;
 	default:
 		timeout = K_FOREVER;
@@ -667,13 +683,34 @@ static struct net_buf *bt_att_chan_create_pdu(struct bt_att_chan *chan, uint8_t
 		return NULL;
 	}
 
-	data = tx_meta_data_alloc(timeout);
-	if (!data) {
-		LOG_WRN("Unable to allocate ATT TX meta");
-		net_buf_unref(buf);
-		return NULL;
+	if (is_rsp) {
+		/* There can only ever be one transaction at a time on a
+		 * bearer/channel. Use a dedicated channel meta-data to ensure
+		 * we can always queue an (error) RSP for each REQ. The ATT
+		 * module can then reschedule the RSP if it is not able to send
+		 * it immediately.
+		 */
+		if (chan->rsp_meta.att_chan) {
+			/* Returning a NULL here will trigger an ATT timeout.
+			 * This is better than an assert as an assert would
+			 * allow a peer to DoS us.
+			 */
+			LOG_ERR("already processing a transaction on chan %p", chan);
+
+			return NULL;
+		}
+		data = &chan->rsp_meta;
+		LOG_INF("alloc rsp meta");
+	} else {
+		data = tx_meta_data_alloc(timeout);
+		if (!data) {
+			LOG_WRN("Unable to allocate ATT TX meta");
+			net_buf_unref(buf);
+			return NULL;
+		}
 	}
 
+	data->att_chan = chan;
 	bt_att_tx_meta_data(buf) = data;
 
 	hdr = net_buf_add(buf, sizeof(*hdr));