Bluetooth: Host: Fix GATT Long Read for EATT

When EATT is established, the value returned from `bt_att_get_mtu` is
not useful to determine the ATT_MTU that applies to a ATT response. This
is because `bt_att_get_mtu` may return the value for a different bearer
than the request is serviced on.

To fix this, the params struct for the GATT read operation is given a
new field that will record the ATT_MTU that applies to this ATT
operation. This value is then used to determine if the GATT long read
operation is concluded.

Fixes: #61741

Signed-off-by: Aleksander Wasaznik <[email protected]>

include/zephyr/bluetooth/gatt.h

@@ -1572,6 +1572,8 @@ struct bt_gatt_read_params {
 #if defined(CONFIG_BT_EATT)
 	enum bt_att_chan_opt chan_opt;
 #endif /* CONFIG_BT_EATT */
+	/** Internal */
+	uint16_t _att_mtu;
 };
 
 /** @brief Read Attribute Value by handle
@@ -1582,9 +1584,21 @@ struct bt_gatt_read_params {
  *  depending on how many instances of given the UUID exists with the
  *  start_handle being updated for each instance.
  *
- *  If an instance does contain a long value which cannot be read entirely the
- *  caller will need to read the remaining data separately using the handle and
- *  offset.
+ *  To perform a GATT Long Read procedure, start with a Characteristic Value
+ *  Read (by setting @c offset @c 0 and @c handle_count @c 1) and then return
+ *  @ref BT_GATT_ITER_CONTINUE from the callback. This is equivalent to calling
+ *  @ref bt_gatt_read again, but with the correct offset to continue the read.
+ *  This may be repeated until the procedure is complete, which is signaled by
+ *  the callback being called with @p data set to @c NULL.
+ *
+ *  Note that returning @ref BT_GATT_ITER_CONTINUE is really starting a new ATT
+ *  operation, so this can fail to allocate resources. However, all API errors
+ *  are reported as if the server returned @ref BT_ATT_ERR_UNLIKELY. There is no
+ *  way to distinguish between this condition and a @ref BT_ATT_ERR_UNLIKELY
+ *  response from the server itself.
+ *
+ *  Note that the effect of returning @ref BT_GATT_ITER_CONTINUE from the
+ *  callback varies depending on the type of read operation.
  *
  *  The Response comes in callback @p params->func. The callback is run from
  *  the context specified by 'config BT_RECV_CONTEXT'.

subsys/bluetooth/host/att.c

@@ -447,12 +447,19 @@ static int chan_req_send(struct bt_att_chan *chan, struct bt_att_req *req)
 	buf = req->buf;
 	req->buf = NULL;
 
+	/* This lock makes sure the value of `bt_att_mtu(chan)` does not
+	 * change.
+	 */
+	k_sched_lock();
 	err = bt_att_chan_send(chan, buf);
 	if (err) {
 		/* We still have the ownership of the buffer */
 		req->buf = buf;
 		chan->req = NULL;
+	} else {
+		bt_gatt_req_set_mtu(req, bt_att_mtu(chan));
 	}
+	k_sched_unlock();
 
 	return err;
 }

subsys/bluetooth/host/att_internal.h

@@ -347,3 +347,5 @@ bool bt_att_tx_meta_data_match(const struct net_buf *buf, bt_gatt_complete_func_
 #endif /* CONFIG_BT_EATT */
 
 bool bt_att_chan_opt_valid(struct bt_conn *conn, enum bt_att_chan_opt chan_opt);
+
+void bt_gatt_req_set_mtu(struct bt_att_req *req, uint16_t mtu);

subsys/bluetooth/host/gatt.c

@@ -2,6 +2,7 @@
 
 /*
  * Copyright (c) 2015-2016 Intel Corporation
+ * Copyright (c) 2023 Nordic Semiconductor ASA
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -4621,8 +4622,11 @@ static void gatt_read_rsp(struct bt_conn *conn, uint8_t err, const void *pdu,
 	 * If the Characteristic Value is greater than (ATT_MTU - 1) octets
 	 * in length, the Read Long Characteristic Value procedure may be used
 	 * if the rest of the Characteristic Value is required.
+	 *
+	 * Note: Both BT_ATT_OP_READ_RSP and BT_ATT_OP_READ_BLOB_RSP
+	 * have an overhead of one octet.
 	 */
-	if (length < (bt_att_get_mtu(conn) - 1)) {
+	if (length < (params->_att_mtu - 1)) {
 		params->func(conn, 0, params, NULL, 0);
 		return;
 	}
@@ -6277,3 +6281,22 @@ void bt_gatt_disconnected(struct bt_conn *conn)
 	remove_cf_cfg(conn);
 #endif
 }
+
+void bt_gatt_req_set_mtu(struct bt_att_req *req, uint16_t mtu)
+{
+	IF_ENABLED(CONFIG_BT_GATT_CLIENT, ({
+		if (req->func == gatt_read_rsp) {
+			struct bt_gatt_read_params *params = req->user_data;
+
+			__ASSERT_NO_MSG(params);
+			params->_att_mtu = mtu;
+			return;
+		}
+	}));
+
+	/* Otherwise: This request type does not have an `_att_mtu`
+	 * params field or any other method to get this value, so we can
+	 * just drop it here. Feel free to add this capability to other
+	 * request types if needed.
+	 */
+}

tests/bsim/bluetooth/host/att/long_read/CMakeLists.txt

@@ -0,0 +1,24 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.20.0)
+
+find_package(Zephyr HINTS $ENV{ZEPHYR_BASE})
+project(app)
+
+target_sources(app PRIVATE
+  testlib/bs_main.c
+  testlib/adv.c
+  testlib/connect.c
+  testlib/scan.c
+  testlib/security.c
+  testlib/att_read.c
+  testlib/att_write.c
+  testlib/bs_sync.c
+  testlib/conn_wait.c
+  main.c
+)
+
+zephyr_include_directories(
+  ${BSIM_COMPONENTS_PATH}/libPhyComv1/src/
+  ${BSIM_COMPONENTS_PATH}/libUtilv1/src/
+)

tests/bsim/bluetooth/host/att/long_read/_build.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+
+set -eu
+dotslash="$(realpath "$(dirname "${BASH_SOURCE[0]}")")"
+bin_dir="${BSIM_OUT_PATH}/bin"
+BOARD="${BOARD:-nrf52_bsim}"
+
+cd "${dotslash}"
+
+compile_path="${bin_dir}/bs_${BOARD}_"
+compile_path+="$(realpath --relative-to "$(west topdir)"/zephyr prj.conf | tr /. _)"
+
+west build -b nrf52_bsim
+cp -v build/zephyr/zephyr.exe "${compile_path}"

tests/bsim/bluetooth/host/att/long_read/main.c

@@ -0,0 +1,232 @@
+/* Copyright (c) 2023 Nordic Semiconductor ASA
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <argparse.h>
+#include <zephyr/bluetooth/gatt.h>
+#include <zephyr/logging/log.h>
+#include <zephyr/sys/__assert.h>
+#include <zephyr/settings/settings.h>
+#include <zephyr/sys/byteorder.h>
+#include <zephyr/bluetooth/bluetooth.h>
+
+#include "testlib/adv.h"
+#include "testlib/att_read.h"
+#include "testlib/att_write.h"
+#include "testlib/bs_macro.h"
+#include "testlib/bs_sync.h"
+#include "testlib/conn_ref.h"
+#include "testlib/conn_wait.h"
+#include "testlib/connect.h"
+#include "testlib/log_utils.h"
+#include "testlib/scan.h"
+#include "testlib/security.h"
+
+/* This test uses system asserts to fail tests. */
+BUILD_ASSERT(__ASSERT_ON);
+
+#define CENTRAL_DEVICE_NBR 0
+#define PERIPHERAL_DEVICE_NBR 1
+
+LOG_MODULE_REGISTER(main, LOG_LEVEL_DBG);
+
+#define UUID_1                                                                                     \
+	BT_UUID_DECLARE_128(0xdb, 0x1f, 0xe2, 0x52, 0xf3, 0xc6, 0x43, 0x66, 0xb3, 0x92, 0x5d,      \
+			    0xc6, 0xe7, 0xc9, 0x59, 0x9d)
+
+#define UUID_2                                                                                     \
+	BT_UUID_DECLARE_128(0x3f, 0xa4, 0x7f, 0x44, 0x2e, 0x2a, 0x43, 0x05, 0xab, 0x38, 0x07,      \
+			    0x8d, 0x16, 0xbf, 0x99, 0xf1)
+
+static ssize_t read_mtu_validation_chrc(struct bt_conn *conn, const struct bt_gatt_attr *attr,
+					void *buf, uint16_t buf_len, uint16_t offset)
+{
+	ssize_t read_len;
+
+	LOG_INF("Server side buf_len %u", buf_len);
+
+	/* Note: We assume `buf_len` is equal to the usable payload
+	 * capacity of the response PDU. I.e. `(ATT_MTU - 1)` for
+	 * BT_ATT_OP_READ_RSP and BT_ATT_OP_READ_BLOB_RSP.
+	 */
+
+	/* Send back a full PDU on the first read (on offset 0). Then an
+	 * not full one for the second read to conlude the long read..
+	 */
+	read_len = buf_len;
+	if (offset > 0) {
+		__ASSERT_NO_MSG(read_len > 0);
+		/* The second PDU is one-less-than-full to test for off
+		 * by one errors.
+		 */
+		read_len -= 1;
+	}
+
+	/* If the ATT_MTU is too large, sending a one-less-than-full
+	 * response would exeed the max attribute length limit.
+	 */
+	__ASSERT(buf_len < (BT_ATT_MAX_ATTRIBUTE_LEN / 2),
+		 "The EATT buffer is too large for this test.");
+
+	/* Ensure the padding bytes (that are not overwritten later in
+	 * this function) are initialized.
+	 */
+	memset(buf, 0, read_len);
+
+	/* Echo back the requested read size in the first two bytes of
+	 * each read.
+	 */
+	__ASSERT_NO_MSG(read_len >= 2);
+	sys_put_le16(read_len, buf);
+
+	return read_len;
+}
+
+static struct bt_gatt_attr attrs[] = {
+	BT_GATT_PRIMARY_SERVICE(UUID_1),
+	BT_GATT_CHARACTERISTIC(UUID_2, BT_GATT_CHRC_READ, BT_GATT_PERM_READ,
+			       read_mtu_validation_chrc, NULL, NULL),
+};
+
+static struct bt_gatt_service svc = {
+	.attrs = attrs,
+	.attr_count = ARRAY_SIZE(attrs),
+};
+
+static void find_the_chrc(struct bt_conn *conn, uint16_t *chrc_value_handle)
+{
+	uint16_t svc_handle;
+	uint16_t svc_end_handle;
+	uint16_t chrc_end_handle;
+
+	EXPECT_ZERO(bt_testlib_gatt_discover_primary(&svc_handle, &svc_end_handle, conn, UUID_1, 1,
+						     0xffff));
+
+	LOG_INF("svc_handle: %u, svc_end_handle: %u", svc_handle, svc_end_handle);
+
+	EXPECT_ZERO(bt_testlib_gatt_discover_characteristic(chrc_value_handle, &chrc_end_handle,
+							    NULL, conn, UUID_2, (svc_handle + 1),
+							    svc_end_handle));
+
+	LOG_INF("chrc_value_handle: %u, chrc_end_handle: %u", *chrc_value_handle, chrc_end_handle);
+}
+
+static void bs_sync_all_log(char *log_msg)
+{
+	/* Everyone meets here. */
+	bt_testlib_bs_sync_all();
+
+	if (get_device_nbr() == 0) {
+		LOG_WRN("Sync point: %s", log_msg);
+	}
+
+	/* Everyone waits for d0 to finish logging. */
+	bt_testlib_bs_sync_all();
+}
+
+static inline void bt_enable_quiet(void)
+{
+	bt_testlib_log_level_set("bt_hci_core", LOG_LEVEL_ERR);
+	bt_testlib_log_level_set("bt_id", LOG_LEVEL_ERR);
+
+	EXPECT_ZERO(bt_enable(NULL));
+
+	bt_testlib_log_level_set("bt_hci_core", LOG_LEVEL_INF);
+	bt_testlib_log_level_set("bt_id", LOG_LEVEL_INF);
+}
+
+static void test_long_read(enum bt_att_chan_opt bearer, uint16_t chrc_value_handle,
+			   struct bt_conn *conn)
+{
+	bool central = (get_device_nbr() == CENTRAL_DEVICE_NBR);
+
+	if (central) {
+		size_t read_count;
+
+		NET_BUF_SIMPLE_DEFINE(attr_value_buf, BT_ATT_MAX_ATTRIBUTE_LEN);
+
+		/* Perform the whole long read operation. */
+		EXPECT_ZERO(btt_gatt_long_read(&attr_value_buf, NULL, conn, bearer,
+					       chrc_value_handle, 0));
+
+		/* Parse the read attribute value to verify the
+		 * integrity of the transfer.
+		 *
+		 * Each response starts with the length of the whole
+		 * response and the rest is zero-padded.
+		 */
+		for (read_count = 0; attr_value_buf.len; read_count++) {
+			uint16_t encoded_len;
+			uint16_t padding_size;
+
+			LOG_INF("Verifying read %u", read_count);
+
+			__ASSERT(attr_value_buf.len >= sizeof(encoded_len),
+				 "Incomplete encoded length");
+			encoded_len = net_buf_simple_pull_le16(&attr_value_buf);
+
+			padding_size = (encoded_len - sizeof(uint16_t));
+			LOG_INF("Padding size %u", padding_size);
+
+			/* Check and discard padding. */
+			for (uint16_t i = 0; i < padding_size; i++) {
+				__ASSERT(attr_value_buf.len, "Unexpected end of buffer");
+				__ASSERT(net_buf_simple_pull_u8(&attr_value_buf) == 0,
+					 "Expected a padding byte at %u", i);
+			}
+		}
+		LOG_INF("Verified %u reads", read_count);
+		__ASSERT(read_count > 1, "Expected at least two reads");
+	}
+}
+
+void the_test(void)
+{
+	bool central = (get_device_nbr() == CENTRAL_DEVICE_NBR);
+	bool peripheral = (get_device_nbr() == PERIPHERAL_DEVICE_NBR);
+	bt_addr_le_t adva;
+	struct bt_conn *conn = NULL;
+	uint16_t chrc_value_handle = 0;
+
+	if (peripheral) {
+		EXPECT_ZERO(bt_gatt_service_register(&svc));
+	}
+
+	bt_enable_quiet();
+
+	if (peripheral) {
+		EXPECT_ZERO(bt_set_name("peripheral"));
+		EXPECT_ZERO(bt_testlib_adv_conn(
+			&conn, BT_ID_DEFAULT,
+			(BT_LE_ADV_OPT_USE_NAME | BT_LE_ADV_OPT_FORCE_NAME_IN_AD)));
+	}
+
+	if (central) {
+		EXPECT_ZERO(bt_testlib_scan_find_name(&adva, "peripheral"));
+		EXPECT_ZERO(bt_testlib_connect(&adva, &conn));
+
+		/* Establish EATT bearers. */
+		EXPECT_ZERO(bt_testlib_secure(conn, BT_SECURITY_L2));
+
+		while (bt_eatt_count(conn) == 0) {
+			k_msleep(100);
+		};
+	}
+
+	bs_sync_all_log("Connected");
+
+	/* Perform discovery. */
+	if (central) {
+		find_the_chrc(conn, &chrc_value_handle);
+	}
+
+	bs_sync_all_log("Testing UATT");
+	test_long_read(BT_ATT_CHAN_OPT_UNENHANCED_ONLY, chrc_value_handle, conn);
+
+	bs_sync_all_log("Testing EATT");
+	test_long_read(BT_ATT_CHAN_OPT_ENHANCED_ONLY, chrc_value_handle, conn);
+
+	bs_sync_all_log("Test Complete");
+
+	PASS("Test complete\n");
+}