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

@@ -1577,6 +1577,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
@@ -1587,9 +1589,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

@@ -4649,8 +4649,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;
 	}
@@ -6303,3 +6306,20 @@ 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;
+			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,3 @@
+#!/usr/bin/env bash
+
+exec west build -b nrf52_bsim

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

@@ -0,0 +1,186 @@
+/* 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 "testlib/bs_macro.h"
+#include "testlib/bs_sync.h"
+#include "testlib/adv.h"
+#include "testlib/connect.h"
+#include "testlib/scan.h"
+#include "testlib/security.h"
+#include "testlib/conn_ref.h"
+#include "testlib/att_read.h"
+#include "testlib/att_write.h"
+#include "testlib/conn_wait.h"
+
+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 = buf_len;
+
+	__ASSERT(buf_len < 256, "The EATT buffer is too powerful for this test.");
+
+	/* Note that `bt_gatt_get_mtu` is not useful when EATT is
+	 * established because it may be returning the value for a
+	 * different bearer than this request is serviced on.
+	 */
+	LOG_INF("Server side buf_len %u", buf_len);
+	LOG_INF("Server size bt_gatt_get_mtu %u", bt_gatt_get_mtu(conn));
+
+	/* Send back the whole buffer worth of data the first time. Then
+	 * send all-but-full one to conlude the long read. The
+	 * assumption here is that `buf_len` is `(ATT_MTU - 1)`, which
+	 * is read length that triggers subsequent reads in a long read
+	 * procedure.
+	 */
+	if (offset > 0) {
+		read_len -= 1;
+	}
+
+	/* Fill the read buffer with zero. */
+	memset(buf, 0, read_len);
+
+	/* Echo back the requested read size in the first two bytes of
+	 * each read. */
+	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),
+};
+
+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;
+
+	EZ(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);
+
+	EZ(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);
+}
+
+#include <zephyr/logging/log_ctrl.h>
+#include <zephyr/bluetooth/bluetooth.h>
+static inline void log_level_set(char *module, uint32_t new_level)
+{
+	__ASSERT_NO_MSG(IS_ENABLED(CONFIG_LOG_RUNTIME_FILTERING));
+	int source_id = log_source_id_get(module);
+	__ASSERT(source_id >= 0, "%d", source_id);
+	uint32_t result_level = log_filter_set(NULL, Z_LOG_LOCAL_DOMAIN_ID, source_id, new_level);
+	__ASSERT(result_level == new_level, "%u", result_level);
+}
+
+static inline void bt_enable_quiet()
+{
+	log_level_set("bt_hci_core", LOG_LEVEL_ERR);
+	log_level_set("bt_id", LOG_LEVEL_ERR);
+	EZ(bt_enable(NULL));
+	log_level_set("bt_hci_core", LOG_LEVEL_INF);
+	log_level_set("bt_id", LOG_LEVEL_INF);
+}
+
+void sync(char *log)
+{
+	bt_testlib_bs_sync();
+	if (get_device_nbr() == 0) {
+		LOG_WRN("Sync: %s", log);
+	}
+	bt_testlib_bs_sync();
+}
+
+void the_test(void)
+{
+	bool central = get_device_nbr() == 0;
+	bool peripheral = get_device_nbr() == 1;
+	bt_addr_le_t adva;
+	struct bt_conn *conn = NULL;
+	uint16_t chrc_value_handle = 0;
+
+	if (peripheral) {
+		EZ(bt_gatt_service_register(&svc));
+	}
+
+	bt_enable_quiet();
+
+	if (peripheral) {
+		EZ(bt_set_name("peripheral"));
+		EZ(bt_testlib_adv_conn(&conn, BT_ID_DEFAULT,
+				       (BT_LE_ADV_OPT_USE_NAME | BT_LE_ADV_OPT_FORCE_NAME_IN_AD)));
+	}
+	if (central) {
+		EZ(bt_testlib_scan_find_name(&adva, "peripheral"));
+		EZ(bt_testlib_connect(&adva, &conn));
+
+		/* Establish EATT bearers. */
+		EZ(bt_testlib_secure(conn, BT_SECURITY_L2));
+
+		while (bt_eatt_count(conn) == 0) {
+			k_msleep(100);
+		};
+	}
+
+	sync("Connected");
+
+	if (central) {
+		find_the_chrc(conn, &chrc_value_handle);
+
+		uint16_t actual_read_len = 0;
+		uint16_t remote_send_len = 0;
+
+		/* Buffer is two bytes large. Testlib will truncate the
+		 * rest for us.
+		 */
+		NET_BUF_SIMPLE_DEFINE(attr_value, 512);
+
+		EZ(btt_gatt_long_read(&attr_value, &actual_read_len, conn,
+				      BT_ATT_CHAN_OPT_ENHANCED_ONLY, chrc_value_handle, 0));
+
+		for (size_t i = 0; attr_value.len; i++) {
+			__ASSERT(attr_value.len >= sizeof(remote_send_len),
+				 "Remote was supposed to send an uint16. But there are not enough "
+				 "bytes in the buffer.");
+			remote_send_len = net_buf_simple_pull_le16(&attr_value);
+			LOG_INF("Verifying read %u: %u", i, remote_send_len);
+			__ASSERT((remote_send_len - 2) <= attr_value.len, "Length mismatch. %u %u",
+				 (remote_send_len - 2), attr_value.len);
+			net_buf_simple_pull_mem(&attr_value, (remote_send_len - 2));
+		}
+		LOG_INF("actual_read_len: %u", actual_read_len);
+	}
+
+	sync("Test Complete");
+
+	PASS("Test complete\n");
+}

tests/bsim/bluetooth/host/att/long_read/prj.conf

@@ -0,0 +1,26 @@
+CONFIG_ASSERT=y
+CONFIG_BOOT_BANNER=n
+CONFIG_BT_BUF_ACL_RX_SIZE=204
+CONFIG_BT_CENTRAL=y
+CONFIG_BT_DEVICE_NAME_DYNAMIC=y
+CONFIG_BT_EATT=y
+CONFIG_BT_EXT_ADV=y
+CONFIG_BT_GATT_CLIENT=y
+CONFIG_BT_GATT_DYNAMIC_DB=y
+CONFIG_BT_L2CAP_DYNAMIC_CHANNEL=y
+CONFIG_BT_L2CAP_ECRED=y
+CONFIG_BT_L2CAP_TX_MTU=200
+CONFIG_BT_MAX_CONN=3
+CONFIG_BT_MAX_PAIRED=2
+CONFIG_BT_PERIPHERAL=y
+CONFIG_BT_PRIVACY=n
+CONFIG_BT_SMP=y
+CONFIG_BT_TESTING=y
+CONFIG_BT=y
+CONFIG_FLASH_MAP=y
+CONFIG_FLASH_PAGE_LAYOUT=y
+CONFIG_FLASH=y
+CONFIG_LOG_BACKEND_FORMAT_TIMESTAMP=n
+CONFIG_LOG_RUNTIME_FILTERING=y
+CONFIG_LOG_TAG_MAX_LEN=20
+CONFIG_LOG=y

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

@@ -0,0 +1,19 @@
+#!/usr/bin/env bash
+
+set -eu
+dotslash="$(realpath "$(dirname "${BASH_SOURCE[0]}")")"
+
+args_all=(-s=long_read -D=2)
+args_dev=(-v=2 -RealEncryption=1 -testid=the_test)
+sim_seconds=60
+
+echo "Simulation time: $sim_seconds seconds"
+
+# Required for
+cd "${BSIM_OUT_PATH}/bin"
+
+("$dotslash"/build/zephyr/zephyr.exe "${args_all[@]}" "${args_dev[@]}" -d=0 || echo d0 $? ) &
+("$dotslash"/build/zephyr/zephyr.exe "${args_all[@]}" "${args_dev[@]}" -d=1 || echo d1 $? ) &
+(./bs_2G4_phy_v1 "${args_all[@]}" -v=6 -sim_length=$((sim_seconds * 10**6)) || echo phy $?) &
+
+wait