Bluetooth: Host: Fix connection establishment upon RPA timeout

Before this commit, the following bugs were present:
- When `CONFIG_BT_FILTER_ACCEPT_LIST` was set, connection establishment
  was cancelled upon RPA timeout. This required the application
  to restart the initiator every RPA timeout.
- When `CONFIG_BT_FILTER_ACCEPT_LIST` was not set, the RPA was not updated
  while the initiator was running.

This commit unifies the RPA timeout handling for both these cases.
Upon RPA timeout the initiator is cancelled and restarted when
the controller raises the LE Connection Complete event.
The workqueue state is checked when restarting the initiator to prevent
it being restarted when the timeout is hit.

Corresponding test cases have been added to ensure that this
feature works.

Signed-off-by: Rubin Gerritsen <rubin.gerritsen@nordicsemi.no>

subsys/bluetooth/host/hci_core.c

@@ -1159,6 +1159,7 @@ static void conn_auto_initiate(struct bt_conn *conn)
 
 static void le_conn_complete_cancel(uint8_t err)
 {
+	int ret;
 	struct bt_conn *conn;
 
 	/* Handle create connection cancel.
@@ -1172,27 +1173,30 @@ static void le_conn_complete_cancel(uint8_t err)
 		return;
 	}
 
-	conn->err = err;
-
-	/* Handle cancellation of outgoing connection attempt. */
-	if (!IS_ENABLED(CONFIG_BT_FILTER_ACCEPT_LIST)) {
-		/* We notify before checking autoconnect flag
-		 * as application may choose to change it from
-		 * callback.
-		 */
-		bt_conn_set_state(conn, BT_CONN_DISCONNECTED);
-		/* Check if device is marked for autoconnect. */
 	if (atomic_test_bit(conn->flags, BT_CONN_AUTO_CONNECT)) {
+		if (!IS_ENABLED(CONFIG_BT_FILTER_ACCEPT_LIST)) {
 			/* Restart passive scanner for device */
 			bt_conn_set_state(conn, BT_CONN_SCAN_BEFORE_INITIATING);
+		} else {
+			/* Restart FAL initiator after RPA timeout. */
+			ret = bt_le_create_conn(conn);
+			if (ret) {
+				LOG_ERR("Failed to restart initiator");
+			}
 		}
 	} else {
-		if (atomic_test_bit(conn->flags, BT_CONN_AUTO_CONNECT)) {
+		int busy_status = k_work_delayable_busy_get(&conn->deferred_work);
-			/* Restart FAL initiator after RPA timeout. */
+
-			bt_le_create_conn(conn);
+		if (!(busy_status & (K_WORK_QUEUED | K_WORK_DELAYED))) {
-		} else {
+			/* Connection initiation timeout triggered. */
-			/* Create connection canceled by timeout */
+			conn->err = err;
 			bt_conn_set_state(conn, BT_CONN_DISCONNECTED);
+		} else {
+			/* Restart initiator after RPA timeout. */
+			ret = bt_le_create_conn(conn);
+			if (ret) {
+				LOG_ERR("Failed to restart initiator");
+			}
 		}
 	}
 

subsys/bluetooth/host/id.c

@@ -606,7 +606,6 @@ static void le_update_private_addr(void)
 	}
 #endif
 	if (IS_ENABLED(CONFIG_BT_CENTRAL) &&
-	    IS_ENABLED(CONFIG_BT_FILTER_ACCEPT_LIST) &&
 	    atomic_test_bit(bt_dev.flags, BT_DEV_INITIATING)) {
 		/* Canceled initiating procedure will be restarted by
 		 * connection complete event.

tests/bsim/bluetooth/host/privacy/central/CMakeLists.txt

@@ -5,6 +5,12 @@ cmake_minimum_required(VERSION 3.20.0)
 find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
 project(bsim_test_rpa_central)
 
+add_subdirectory(${ZEPHYR_BASE}/tests/bluetooth/common/testlib testlib)
+target_link_libraries(app PRIVATE testlib)
+
+add_subdirectory(${ZEPHYR_BASE}/tests/bsim/babblekit babblekit)
+target_link_libraries(app PRIVATE babblekit)
+
 target_sources(app PRIVATE
 	src/bs_bt_utils.c
 	src/tester.c

tests/bsim/bluetooth/host/privacy/central/prj.conf

@@ -11,3 +11,8 @@ CONFIG_BT_RPA_TIMEOUT=10
 CONFIG_BT_CTLR_ADV_SET=2
 CONFIG_BT_CTLR_SCAN_REQ_NOTIFY=y
 CONFIG_BT_SCAN_WITH_IDENTITY=y
+
+# Enable the possibility to set the RPA such that the RPA
+# refreshes before the connection attempt has completed
+CONFIG_BT_RPA_TIMEOUT_DYNAMIC=y
+CONFIG_BT_CREATE_CONN_TIMEOUT=10

tests/bsim/bluetooth/host/privacy/central/src/dut.c

@@ -12,6 +12,10 @@
 #include <zephyr/bluetooth/conn.h>
 #include <zephyr/bluetooth/bluetooth.h>
 
+#include <babblekit/testcase.h>
+#include <testlib/conn.h>
+#include <testlib/scan.h>
+
 void start_scanning(void)
 {
 	int err;
@@ -46,3 +50,69 @@ void dut_procedure(void)
 
 	PASS("PASS\n");
 }
+
+void dut_procedure_connect_short_rpa_timeout(void)
+{
+	backchannel_init(1);
+
+	const uint16_t rpa_timeout_s = 1;
+
+	int err;
+	bt_addr_le_t peer = {};
+	struct bt_conn *conn = NULL;
+
+	/* Enable bluetooth */
+	err = bt_enable(NULL);
+	TEST_ASSERT(!err, "Failed to enable bluetooth (err %d)");
+
+	/* Central to use a short RPA timeout */
+	err = bt_le_set_rpa_timeout(rpa_timeout_s);
+	TEST_ASSERT(!err, "Failed to set RPA timeout (err %d)", err);
+
+	err = bt_testlib_scan_find_name(&peer, CONFIG_BT_DEVICE_NAME);
+	TEST_ASSERT(!err, "Failed to start scan (err %d)", err);
+
+	/* Indicate to the peer device that we have found the advertiser. */
+	backchannel_sync_send();
+
+	/* Create a connection using that address */
+	err = bt_testlib_connect(&peer, &conn);
+	TEST_ASSERT(!err, "Failed to initiate connection (err %d)", err);
+
+	PASS("PASS\n");
+}
+
+void dut_procedure_connect_timeout(void)
+{
+	const uint16_t rpa_timeout_s = 1;
+
+	int err;
+	bt_addr_le_t peer = {.type = BT_ADDR_LE_RANDOM, .a = {.val = {1, 2, 3, 4, 5, 6}}};
+	struct bt_conn *conn = NULL;
+
+	/* Enable bluetooth */
+	err = bt_enable(NULL);
+	TEST_ASSERT(!err, "Failed to enable bluetooth (err %d)", err);
+
+	/* Central to use a short RPA timeout */
+	err = bt_le_set_rpa_timeout(rpa_timeout_s);
+	TEST_ASSERT(!err, "Failed to set RPA timeout (err %d)", err);
+
+	int64_t old_time = k_uptime_get();
+
+	/* Create a connection using that address */
+	err = bt_testlib_connect(&peer, &conn);
+	TEST_ASSERT(err == BT_HCI_ERR_UNKNOWN_CONN_ID,
+		    "Expected connection establishment to time out (err %d)", err);
+
+	int64_t new_time = k_uptime_get();
+	int64_t time_diff_ms = new_time - old_time;
+	int64_t expected_conn_timeout_ms = CONFIG_BT_CREATE_CONN_TIMEOUT * 1000;
+	int64_t diff_to_expected_ms = abs(time_diff_ms - expected_conn_timeout_ms);
+
+	printk("Connection creation timed out after %d ms\n", time_diff_ms);
+	TEST_ASSERT(diff_to_expected_ms < 0.1 * expected_conn_timeout_ms,
+		    "Connection timeout not within 10 \%% of expected timeout");
+
+	PASS("PASS\n");
+}

tests/bsim/bluetooth/host/privacy/central/src/main.c

@@ -8,21 +8,51 @@
 #include "bstests.h"
 
 void tester_procedure(void);
+void tester_procedure_periph_delayed_start_of_conn_adv(void);
 void dut_procedure(void);
+void dut_procedure_connect_short_rpa_timeout(void);
+void dut_procedure_connect_timeout(void);
 
 static const struct bst_test_instance test_to_add[] = {
 	{
 		.test_id = "central",
+		.test_descr = "Central performs active scanning using RPA",
 		.test_post_init_f = test_init,
 		.test_tick_f = test_tick,
 		.test_main_f = dut_procedure,
 	},
+	{
+		.test_id = "central_connect_short_rpa_timeout",
+		.test_descr = "Central connects to a peripheral using a short RPA timeout",
+		.test_post_init_f = test_init,
+		.test_tick_f = test_tick,
+		.test_main_f = dut_procedure_connect_short_rpa_timeout,
+	},
+	{
+		.test_id = "central_connect_fails_with_short_rpa_timeout",
+		.test_descr = "Central connects to a peripheral using a short RPA timeout"
+			      " but expects connection establishment to time out.",
+		.test_post_init_f = test_init,
+		.test_tick_f = test_tick,
+		.test_main_f = dut_procedure_connect_timeout,
+	},
 	{
 		.test_id = "peripheral",
+		.test_descr = "Performs scannable advertising, validates that the scanner"
+			      " RPA address refreshes",
 		.test_post_init_f = test_init,
 		.test_tick_f = test_tick,
 		.test_main_f = tester_procedure,
 	},
+	{
+		.test_id = "periph_delayed_start_of_conn_adv",
+		.test_descr = "Performs connectable advertising. "
+			      "The advertiser is stopped for 10 seconds when instructed by the DUT"
+			      " to allow it to run the initiator for longer than its RPA timeout.",
+		.test_post_init_f = test_init,
+		.test_tick_f = test_tick,
+		.test_main_f = tester_procedure_periph_delayed_start_of_conn_adv,
+	},
 	BSTEST_END_MARKER,
 };
 

tests/bsim/bluetooth/host/privacy/central/src/tester.c

@@ -14,7 +14,10 @@
 #include <zephyr/bluetooth/conn.h>
 #include <zephyr/toolchain.h>
 
+#include <babblekit/testcase.h>
+
 DEFINE_FLAG(flag_new_address);
+DEFINE_FLAG(flag_connected);
 
 void scanned_cb(struct bt_le_ext_adv *adv, struct bt_le_ext_adv_scanned_info *info)
 {
@@ -64,8 +67,14 @@ void scanned_cb(struct bt_le_ext_adv *adv, struct bt_le_ext_adv_scanned_info *in
 	old_addr = new_addr;
 }
 
+static void connected_cb(struct bt_le_ext_adv *adv, struct bt_le_ext_adv_connected_info *info)
+{
+	SET_FLAG(flag_connected);
+}
+
 static struct bt_le_ext_adv_cb adv_callbacks = {
 	.scanned = scanned_cb,
+	.connected = connected_cb
 };
 
 void start_advertising(void)
@@ -125,3 +134,54 @@ void tester_procedure(void)
 
 	PASS("PASS\n");
 }
+
+void tester_procedure_periph_delayed_start_of_conn_adv(void)
+{
+	backchannel_init(0);
+
+	int err;
+	struct bt_le_adv_param params =
+		BT_LE_ADV_PARAM_INIT(BT_LE_ADV_OPT_CONNECTABLE | BT_LE_ADV_OPT_USE_IDENTITY,
+				     BT_GAP_ADV_FAST_INT_MIN_2,
+				     BT_GAP_ADV_FAST_INT_MAX_2, NULL);
+	struct bt_data ad;
+	struct bt_le_ext_adv *adv;
+
+	/* Enable bluetooth */
+	err = bt_enable(NULL);
+	TEST_ASSERT(!err, "Failed to enable bluetooth (err %d)");
+
+	/* Advertiser to use a long RPA timeout */
+	err = bt_le_set_rpa_timeout(100);
+	TEST_ASSERT(!err, "Failed to set RPA timeout (err %d)", err);
+
+	err = bt_le_ext_adv_create(&params, &adv_callbacks, &adv);
+	TEST_ASSERT(!err, "Failed to create advertising set (err %d)", err);
+
+	ad.type = BT_DATA_NAME_COMPLETE;
+	ad.data_len = strlen(CONFIG_BT_DEVICE_NAME);
+	ad.data = (const uint8_t *)CONFIG_BT_DEVICE_NAME;
+
+	err = bt_le_ext_adv_set_data(adv, &ad, 1, NULL, 0);
+	TEST_ASSERT(!err, "Failed to set advertising data (err %d)", err);
+
+	err = bt_le_ext_adv_start(adv, BT_LE_EXT_ADV_START_DEFAULT);
+	TEST_ASSERT(!err, "Failed to start advertiser (err %d)", err);
+
+	backchannel_sync_wait();
+
+	err = bt_le_ext_adv_stop(adv);
+	TEST_ASSERT(!err, "Failed to stop advertiser (err %d)", err);
+
+	/* Wait a few RPA cycles before restaring the advertiser to force RPA timeout
+	 * on the DUT.
+	 */
+	k_sleep(K_SECONDS(7));
+
+	err = bt_le_ext_adv_start(adv, BT_LE_EXT_ADV_START_DEFAULT);
+	TEST_ASSERT(!err, "Failed to restart advertiser (err %d)", err);
+
+	WAIT_FOR_FLAG(flag_connected);
+
+	PASS("PASS\n");
+}

tests/bsim/bluetooth/host/privacy/central/test_scripts/run_test_short_conn_timeout.sh

@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+# Copyright 2024 Nordic Semiconductor ASA
+# SPDX-License-Identifier: Apache-2.0
+set -eu
+
+source ${ZEPHYR_BASE}/tests/bsim/sh_common.source
+
+EXECUTE_TIMEOUT=100
+verbosity_level=2
+
+# Test that connection establishment times out when RPA
+# timeout is shorter than the connection establishment timeout
+simulation_id="test_central_connect_fails_with_short_rpa_timeout"
+
+central_exe="${BSIM_OUT_PATH}/bin/bs_${BOARD_TS}_$(guess_test_long_name)_prj_conf"
+
+cd ${BSIM_OUT_PATH}/bin
+
+Execute "$central_exe" \
+    -v=${verbosity_level} -s=${simulation_id} -d=0 \
+    -testid=central_connect_fails_with_short_rpa_timeout -RealEncryption=1
+
+Execute ./bs_2G4_phy_v1 -v=${verbosity_level} -s=${simulation_id} \
+    -D=1 -sim_length=60e6 $@
+
+wait_for_background_jobs

tests/bsim/bluetooth/host/privacy/central/test_scripts/run_test_short_rpa_timeout.sh

@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+# Copyright 2024 Nordic Semiconductor ASA
+# SPDX-License-Identifier: Apache-2.0
+set -eu
+
+source ${ZEPHYR_BASE}/tests/bsim/sh_common.source
+
+EXECUTE_TIMEOUT=100
+verbosity_level=2
+
+# Test connection establishment when the RPA times out before
+# we expect the connection to be established
+simulation_id="test_central_connect_short_rpa_timeout"
+
+central_exe="${BSIM_OUT_PATH}/bin/bs_${BOARD_TS}_$(guess_test_long_name)_prj_conf"
+peripheral_exe="${central_exe}"
+
+cd ${BSIM_OUT_PATH}/bin
+
+Execute "$central_exe" \
+    -v=${verbosity_level} -s=${simulation_id} -d=0 \
+    -testid=central_connect_short_rpa_timeout -RealEncryption=1
+
+Execute "$peripheral_exe" \
+    -v=${verbosity_level} -s=${simulation_id} -d=1 \
+    -testid=periph_delayed_start_of_conn_adv -RealEncryption=1
+
+Execute ./bs_2G4_phy_v1 -v=${verbosity_level} -s=${simulation_id} \
+    -D=2 -sim_length=60e6 $@
+
+wait_for_background_jobs