[Backport v3.5-branch] Bluetooth: ATT: Revert re-use of RX buffer

subsys/bluetooth/host/att.c

@@ -83,18 +83,6 @@ 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 */
@@ -103,8 +91,6 @@ struct bt_att_chan {
 	ATOMIC_DEFINE(flags, ATT_NUM_FLAGS);
 	struct bt_att_req	*req;
 	struct k_fifo		tx_queue;
-	struct net_buf		*rsp_buf;
-	struct bt_att_tx_meta_data rsp_meta;
 	struct k_work_delayable	timeout_work;
 	sys_snode_t		node;
 };
@@ -173,6 +159,18 @@ 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];
@@ -194,22 +192,9 @@ 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));
-
-	if (alloc_from_global) {
-		k_fifo_put(&free_att_tx_meta_data, data);
-	}
+	k_fifo_put(&free_att_tx_meta_data, data);
 }
 
 static int bt_att_chan_send(struct bt_att_chan *chan, struct net_buf *buf);
@@ -660,7 +645,6 @@ 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 re_use = false;
 
 	if (len + sizeof(op) > bt_att_mtu(chan)) {
 		LOG_WRN("ATT MTU exceeded, max %u, wanted %zu", bt_att_mtu(chan),
@@ -670,71 +654,25 @@ static struct net_buf *bt_att_chan_create_pdu(struct bt_att_chan *chan, uint8_t
 
 	switch (att_op_get_type(op)) {
 	case ATT_RESPONSE:
-		/* Use a timeout only when responding */
-		timeout = BT_ATT_TIMEOUT;
-		re_use = true;
-		break;
 	case ATT_CONFIRMATION:
+		/* Use a timeout only when responding/confirming */
 		timeout = BT_ATT_TIMEOUT;
 		break;
 	default:
 		timeout = K_FOREVER;
 	}
 
-	if (IS_ENABLED(CONFIG_BT_GATT_READ_MULTIPLE) &&
-	    (op == BT_ATT_OP_READ_MULT_RSP ||
-	     op == BT_ATT_OP_READ_MULT_VL_RSP)) {
-		/* We can't re-use the REQ buffer (see below) for these two
-		 * opcodes, as the handler will read from it _after_ allocating
-		 * the RSP buffer.
-		 */
-		re_use = false;
+	buf = bt_l2cap_create_pdu_timeout(NULL, 0, timeout);
+	if (!buf) {
+		LOG_ERR("Unable to allocate buffer for op 0x%02x", op);
+		return NULL;
 	}
 
-	if (re_use) {
-		/* 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 REQ/RSP on chan %p", chan);
-
-			return NULL;
-		}
-		data = &chan->rsp_meta;
-
-		/* Re-use REQ buf to avoid dropping the REQ and timing out.
-		 * This only works if the bearer used to RX REQs is the same as
-		 * for sending the RSP. That should always be the case
-		 * (per-spec).
-		 */
-		__ASSERT_NO_MSG(chan->rsp_buf);
-		buf = net_buf_ref(chan->rsp_buf);
-
-		net_buf_reset(buf);
-		net_buf_reserve(buf, BT_L2CAP_BUF_SIZE(0));
-
-		LOG_DBG("re-using REQ buf %p for RSP", buf);
-	} else {
-		LOG_DBG("alloc buf & meta from global pools");
-		buf = bt_l2cap_create_pdu_timeout(NULL, 0, timeout);
-		if (!buf) {
-			LOG_ERR("Unable to allocate buffer for op 0x%02x", op);
-			return NULL;
-		}
-
-		data = tx_meta_data_alloc(timeout);
-		if (!data) {
-			LOG_WRN("Unable to allocate ATT TX meta");
-			net_buf_unref(buf);
-			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_ENABLED(CONFIG_BT_EATT)) {
@@ -812,7 +750,6 @@ static void send_err_rsp(struct bt_att_chan *chan, uint8_t req, uint16_t handle,
 
 	buf = bt_att_chan_create_pdu(chan, BT_ATT_OP_ERROR_RSP, sizeof(*rsp));
 	if (!buf) {
-		LOG_ERR("unable to allocate buf for error response");
 		return;
 	}
 
@@ -2907,40 +2844,26 @@ static int bt_att_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)
 		}
 	}
 
-	/* Thread-local variable, shouldn't be used by anything else */
-	__ASSERT_NO_MSG(!att_chan->rsp_buf);
-
-	/* Mark buffer free for re-use by the opcode handler.
-	 *
-	 * This allows ATT to always be able to send a RSP (or err RSP)
-	 * to the peer, regardless of the TX buffer usage by other stack
-	 * users (e.g. GATT notifications, L2CAP using global pool, SMP,
-	 * etc..), avoiding an ATT timeout due to resource usage.
-	 *
-	 * The ref is taken by `bt_att_chan_create_pdu`.
-	 */
-	att_chan->rsp_buf = net_buf_ref(buf);
-
 	if (!handler) {
 		LOG_WRN("Unhandled ATT code 0x%02x", hdr->code);
 		if (att_op_get_type(hdr->code) != ATT_COMMAND &&
 		    att_op_get_type(hdr->code) != ATT_INDICATION) {
 			send_err_rsp(att_chan, hdr->code, 0,
 				     BT_ATT_ERR_NOT_SUPPORTED);
 		}
-		goto exit;
+		return 0;
 	}
 
 	if (IS_ENABLED(CONFIG_BT_ATT_ENFORCE_FLOW)) {
 		if (handler->type == ATT_REQUEST &&
 		    atomic_test_and_set_bit(att_chan->flags, ATT_PENDING_RSP)) {
 			LOG_WRN("Ignoring unexpected request");
-			goto exit;
+			return 0;
 		} else if (handler->type == ATT_INDICATION &&
 			   atomic_test_and_set_bit(att_chan->flags,
 						   ATT_PENDING_CFM)) {
 			LOG_WRN("Ignoring unexpected indication");
-			goto exit;
+			return 0;
 		}
 	}
 
@@ -2956,10 +2879,6 @@ static int bt_att_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)
 		send_err_rsp(att_chan, hdr->code, 0, err);
 	}
 
-exit:
-	net_buf_unref(att_chan->rsp_buf);
-	att_chan->rsp_buf = NULL;
-
 	return 0;
 }
 

subsys/bluetooth/host/buf.c

@@ -43,15 +43,14 @@ NET_BUF_POOL_FIXED_DEFINE(discardable_pool, CONFIG_BT_BUF_EVT_DISCARDABLE_COUNT,
 #if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
 NET_BUF_POOL_DEFINE(acl_in_pool, CONFIG_BT_BUF_ACL_RX_COUNT,
 		    BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE),
-		    MAX(sizeof(struct bt_buf_data), CONFIG_BT_CONN_TX_USER_DATA_SIZE),
-		    bt_hci_host_num_completed_packets);
+		    sizeof(struct acl_data), bt_hci_host_num_completed_packets);
 
 NET_BUF_POOL_FIXED_DEFINE(evt_pool, CONFIG_BT_BUF_EVT_RX_COUNT,
 			  BT_BUF_EVT_RX_SIZE, 8,
 			  NULL);
 #else
 NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT,
-			  BT_BUF_RX_SIZE, CONFIG_BT_CONN_TX_USER_DATA_SIZE,
+			  BT_BUF_RX_SIZE, 8,
 			  NULL);
 #endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 

subsys/bluetooth/host/conn_internal.h

@@ -146,6 +146,17 @@ struct bt_conn_tx {
 	uint32_t pending_no_cb;
 };
 
+struct acl_data {
+	/* Extend the bt_buf user data */
+	struct bt_buf_data buf_data;
+
+	/* Index into the bt_conn storage array */
+	uint8_t  index;
+
+	/** ACL connection handle */
+	uint16_t handle;
+};
+
 struct bt_conn {
 	uint16_t			handle;
 	enum bt_conn_type	type;

subsys/bluetooth/host/hci_core.c

@@ -111,16 +111,8 @@ struct cmd_data {
 
 static struct cmd_data cmd_data[CONFIG_BT_BUF_CMD_TX_COUNT];
 
-#if defined(CONFIG_BT_CONN)
-struct acl_data {
-	uint16_t acl_handle;
-};
-
-static struct acl_data acl_data[CONFIG_BT_BUF_ACL_RX_COUNT];
-#endif
-
 #define cmd(buf) (&cmd_data[net_buf_id(buf)])
-#define acl(buf) (&acl_data[net_buf_id(buf)])
+#define acl(buf) ((struct acl_data *)net_buf_user_data(buf))
 
 void bt_hci_cmd_state_set_init(struct net_buf *buf,
 			       struct bt_hci_cmd_state_set *state,
@@ -209,9 +201,10 @@ void bt_hci_host_num_completed_packets(struct net_buf *buf)
 {
 
 	struct bt_hci_cp_host_num_completed_packets *cp;
-	uint16_t handle = acl(buf)->acl_handle;
+	uint16_t handle = acl(buf)->handle;
 	struct bt_hci_handle_count *hc;
 	struct bt_conn *conn;
+	uint8_t index = acl(buf)->index;
 
 	net_buf_destroy(buf);
 
@@ -220,9 +213,9 @@ void bt_hci_host_num_completed_packets(struct net_buf *buf)
 		return;
 	}
 
-	conn = bt_conn_lookup_handle(handle, BT_CONN_TYPE_ALL);
+	conn = bt_conn_lookup_index(index);
 	if (!conn) {
-		LOG_WRN("Unable to look up conn with ACL handle %u", handle);
+		LOG_WRN("Unable to look up conn with index 0x%02x", index);
 		return;
 	}
 
@@ -519,23 +512,26 @@ static void hci_acl(struct net_buf *buf)
 	handle = sys_le16_to_cpu(hdr->handle);
 	flags = bt_acl_flags(handle);
 
-	acl(buf)->acl_handle = bt_acl_handle(handle);
+	acl(buf)->handle = bt_acl_handle(handle);
+	acl(buf)->index = BT_CONN_INDEX_INVALID;
 
-	LOG_DBG("handle %u len %u flags %u", acl(buf)->acl_handle, len, flags);
+	LOG_DBG("handle %u len %u flags %u", acl(buf)->handle, len, flags);
 
 	if (buf->len != len) {
 		LOG_ERR("ACL data length mismatch (%u != %u)", buf->len, len);
 		net_buf_unref(buf);
 		return;
 	}
 
-	conn = bt_conn_lookup_handle(acl(buf)->acl_handle, BT_CONN_TYPE_ALL);
+	conn = bt_conn_lookup_handle(acl(buf)->handle, BT_CONN_TYPE_ALL);
 	if (!conn) {
-		LOG_ERR("Unable to find conn for handle %u", acl(buf)->acl_handle);
+		LOG_ERR("Unable to find conn for handle %u", acl(buf)->handle);
 		net_buf_unref(buf);
 		return;
 	}
 
+	acl(buf)->index = bt_conn_index(conn);
+
 	bt_conn_recv(conn, buf, flags);
 	bt_conn_unref(conn);
 }