Bluetooth: L2CAP: Handle REJECT_RSP for ECRED

We can (and do) open multiple channels with a single L2CAP command. If the
remote doesn't support dynamic channels at all, then it sends back a
REJECT_RSP.

We only destroyed the first channel that matched the command PDU
identifier. Fix that and remove all channels that match.

Also add a test that verifies the patch.

Signed-off-by: Jonathan Rico <jonathan.rico@nordicsemi.no>

subsys/bluetooth/host/l2cap.c

@@ -2101,13 +2101,9 @@ static void reject_cmd(struct bt_l2cap *l2cap, uint8_t ident,
 	struct bt_conn *conn = l2cap->chan.chan.conn;
 	struct bt_l2cap_le_chan *chan;
 
-	/* Check if there is a outstanding channel */
+	while ((chan = l2cap_remove_ident(conn, ident))) {
-	chan = l2cap_remove_ident(conn, ident);
+		bt_l2cap_chan_del(&chan->chan);
-	if (!chan) {
-		return;
 	}
-
-	bt_l2cap_chan_del(&chan->chan);
 }
 #endif /* CONFIG_BT_L2CAP_DYNAMIC_CHANNEL */
 

tests/bsim/bluetooth/host/l2cap/compile.sh

@@ -17,6 +17,8 @@ app=tests/bsim/bluetooth/host/l2cap/stress conf_file=prj_nofrag.conf compile
 app=tests/bsim/bluetooth/host/l2cap/stress conf_file=prj_syswq.conf compile
 app=tests/bsim/bluetooth/host/l2cap/split/dut compile
 app=tests/bsim/bluetooth/host/l2cap/split/tester compile
+app=tests/bsim/bluetooth/host/l2cap/ecred/dut compile
+app=tests/bsim/bluetooth/host/l2cap/ecred/peer compile
 app=tests/bsim/bluetooth/host/l2cap/credits compile
 app=tests/bsim/bluetooth/host/l2cap/credits conf_file=prj_ecred.conf compile
 app=tests/bsim/bluetooth/host/l2cap/credits_seg_recv compile

tests/bsim/bluetooth/host/l2cap/ecred/dut/CMakeLists.txt

@@ -0,0 +1,23 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.20.0)
+
+find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
+
+project(ecred_dut)
+
+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)
+
+zephyr_include_directories(
+  ${BSIM_COMPONENTS_PATH}/libUtilv1/src/
+  ${BSIM_COMPONENTS_PATH}/libPhyComv1/src/
+)
+
+target_sources(app PRIVATE
+  src/main.c
+  src/dut.c
+)

tests/bsim/bluetooth/host/l2cap/ecred/dut/Kconfig

@@ -0,0 +1,18 @@
+# Kconfig options for the test
+#
+# Only used as single point for log level configuration.
+# Can be extended with any new kconfig, really.
+#
+# Copyright (c) 2024 Nordic Semiconductor ASA
+# SPDX-License-Identifier: Apache-2.0
+
+menu "Test configuration"
+
+module = APP
+module-str = app
+
+source "subsys/logging/Kconfig.template.log_config"
+
+endmenu
+
+source "Kconfig.zephyr"

tests/bsim/bluetooth/host/l2cap/ecred/dut/prj.conf

@@ -0,0 +1,29 @@
+CONFIG_BT=y
+CONFIG_BT_DEVICE_NAME="ecred"
+CONFIG_BT_CENTRAL=y
+
+CONFIG_BT_SMP=y # Next config depends on it
+CONFIG_BT_L2CAP_DYNAMIC_CHANNEL=y
+CONFIG_BT_L2CAP_ECRED=y
+
+# Disable auto-initiated procedures so they don't
+# mess with the test's execution.
+CONFIG_BT_AUTO_PHY_UPDATE=n
+CONFIG_BT_AUTO_DATA_LEN_UPDATE=n
+CONFIG_BT_GAP_AUTO_UPDATE_CONN_PARAMS=n
+
+# Dependency of testlib/adv and testlib/scan.
+CONFIG_BT_EXT_ADV=y
+
+CONFIG_ASSERT=y
+CONFIG_LOG=y
+CONFIG_THREAD_NAME=y
+CONFIG_LOG_THREAD_ID_PREFIX=y
+
+CONFIG_ARCH_POSIX_TRAP_ON_FATAL=y
+
+# CONFIG_APP_LOG_LEVEL_DBG=y
+# CONFIG_BT_L2CAP_LOG_LEVEL_DBG=y
+
+# Don't print the bt init stuff
+CONFIG_BT_HCI_CORE_LOG_LEVEL_WRN=y

tests/bsim/bluetooth/host/l2cap/ecred/dut/src/dut.c

@@ -0,0 +1,117 @@
+/*
+ * Copyright (c) 2024 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <zephyr/kernel.h>
+#include <zephyr/bluetooth/bluetooth.h>
+#include <zephyr/bluetooth/conn.h>
+#include <zephyr/bluetooth/l2cap.h>
+#include <zephyr/logging/log.h>
+
+#include "testlib/conn.h"
+#include "testlib/scan.h"
+
+#include "babblekit/flags.h"
+#include "babblekit/testcase.h"
+
+LOG_MODULE_REGISTER(dut, CONFIG_APP_LOG_LEVEL);
+
+static atomic_t disconnected_channels;
+
+static struct bt_l2cap_le_chan chans[4];
+
+void sent_cb(struct bt_l2cap_chan *chan)
+{
+	LOG_DBG("%p", chan);
+}
+
+int recv_cb(struct bt_l2cap_chan *chan, struct net_buf *buf)
+{
+	LOG_DBG("%p", chan);
+
+	return 0;
+}
+
+void l2cap_chan_connected_cb(struct bt_l2cap_chan *l2cap_chan)
+{
+	TEST_ASSERT("This shouldn't happen");
+}
+
+void l2cap_chan_disconnected_cb(struct bt_l2cap_chan *chan)
+{
+	atomic_inc(&disconnected_channels);
+}
+
+static struct bt_l2cap_chan_ops ops = {
+	.connected = l2cap_chan_connected_cb,
+	.disconnected = l2cap_chan_disconnected_cb,
+	.recv = recv_cb,
+	.sent = sent_cb,
+};
+
+void entrypoint_dut(void)
+{
+	/* Test purpose:
+	 *
+	 * Verify that a peer that doesn't support ECRED channels doesn't result
+	 * in us keeping half-open channels.
+	 *
+	 * Two devices:
+	 * - `dut`: tries to establish 4 ecred chans
+	 * - `peer`: rejects the request
+	 *
+	 * Initial conditions:
+	 * - Both devices are connected
+	 *
+	 * Procedure:
+	 * - [dut] request to establish 4 ecred channels
+	 * - [peer] reject command as unknown
+	 * - [dut] get `disconnected` called on all 4 channels
+	 *
+	 * [verdict]
+	 * - each channel gets the `disconnected` callback called
+	 */
+	int err;
+	bt_addr_le_t peer = {};
+	struct bt_conn *conn = NULL;
+
+	TEST_START("dut");
+
+	/* Initialize Bluetooth */
+	err = bt_enable(NULL);
+	TEST_ASSERT(err == 0, "Can't enable Bluetooth (err %d)", err);
+	LOG_DBG("Bluetooth initialized");
+
+	err = bt_testlib_scan_find_name(&peer, "ecred_peer");
+	TEST_ASSERT(!err, "Failed to start scan (err %d)", err);
+
+	/* Create a connection using that address */
+	err = bt_testlib_connect(&peer, &conn);
+	TEST_ASSERT(!err, "Failed to initiate connection (err %d)", err);
+
+	LOG_DBG("Connected");
+
+	LOG_INF("Send ECRED connection request");
+	struct bt_l2cap_chan *chan_list[5] = {0};
+
+	for (int i = 0; i < 4; i++) {
+		/* Register the callbacks */
+		chans[i].chan.ops = &ops;
+
+		/* Add the channel to the connection request list */
+		chan_list[i] = &chans[i].chan;
+	}
+
+	/* The PSM doesn't matter, as the peer doesn't support the command */
+	err = bt_l2cap_ecred_chan_connect(conn, chan_list, 0x0080);
+	TEST_ASSERT(!err, "Error connecting l2cap channels (err %d)\n", err);
+
+	LOG_INF("Wait until peer rejects the channel establishment request");
+	while (atomic_get(&disconnected_channels) < 4) {
+		k_sleep(K_MSEC(10));
+	}
+
+	TEST_PASS_AND_EXIT("dut");
+}

tests/bsim/bluetooth/host/l2cap/ecred/dut/src/main.c

@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2024 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <zephyr/kernel.h>
+
+#include "bs_tracing.h"
+#include "bstests.h"
+#include "babblekit/testcase.h"
+
+extern void entrypoint_dut(void);
+extern enum bst_result_t bst_result;
+
+
+static void test_end_cb(void)
+{
+	if (bst_result != Passed) {
+		TEST_PRINT("Test has not passed.");
+	}
+}
+
+static const struct bst_test_instance entrypoints[] = {
+	{
+		.test_id = "dut",
+		.test_delete_f = test_end_cb,
+		.test_main_f = entrypoint_dut,
+	},
+	BSTEST_END_MARKER,
+};
+
+static struct bst_test_list *install(struct bst_test_list *tests)
+{
+	return bst_add_tests(tests, entrypoints);
+};
+
+bst_test_install_t test_installers[] = {install, NULL};
+
+int main(void)
+{
+	bst_main();
+
+	return 0;
+}

tests/bsim/bluetooth/host/l2cap/ecred/peer/CMakeLists.txt

@@ -0,0 +1,23 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.20.0)
+
+find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
+
+project(ecred_peer)
+
+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)
+
+zephyr_include_directories(
+  ${BSIM_COMPONENTS_PATH}/libUtilv1/src/
+  ${BSIM_COMPONENTS_PATH}/libPhyComv1/src/
+)
+
+target_sources(app PRIVATE
+  src/main.c
+  src/peer.c
+)

tests/bsim/bluetooth/host/l2cap/ecred/peer/prj.conf

@@ -0,0 +1,29 @@
+CONFIG_BT=y
+CONFIG_BT_DEVICE_NAME="ecred_peer"
+CONFIG_BT_PERIPHERAL=y
+
+CONFIG_BT_SMP=y
+CONFIG_BT_L2CAP_DYNAMIC_CHANNEL=y
+CONFIG_BT_L2CAP_ECRED=n
+
+# Disable auto-initiated procedures so they don't
+# mess with the test's execution.
+CONFIG_BT_AUTO_PHY_UPDATE=n
+CONFIG_BT_AUTO_DATA_LEN_UPDATE=n
+CONFIG_BT_GAP_AUTO_UPDATE_CONN_PARAMS=n
+
+# Dependency of testlib/adv and testlib/scan.
+CONFIG_BT_EXT_ADV=y
+
+CONFIG_ASSERT=y
+CONFIG_LOG=y
+CONFIG_THREAD_NAME=y
+CONFIG_LOG_THREAD_ID_PREFIX=y
+
+CONFIG_ARCH_POSIX_TRAP_ON_FATAL=y
+
+# CONFIG_APP_LOG_LEVEL_DBG=y
+# CONFIG_BT_L2CAP_LOG_LEVEL_DBG=y
+
+# Don't print the bt init stuff
+CONFIG_BT_HCI_CORE_LOG_LEVEL_WRN=y

tests/bsim/bluetooth/host/l2cap/ecred/peer/src/main.c

@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2024 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <zephyr/kernel.h>
+
+#include "bs_tracing.h"
+#include "bstests.h"
+#include "babblekit/testcase.h"
+
+extern void entrypoint_peer(void);
+extern enum bst_result_t bst_result;
+
+
+static void test_end_cb(void)
+{
+	if (bst_result != Passed) {
+		TEST_PRINT("Test has not passed.");
+	}
+}
+
+static const struct bst_test_instance entrypoints[] = {
+	{
+		.test_id = "peer",
+		.test_delete_f = test_end_cb,
+		.test_main_f = entrypoint_peer,
+	},
+	BSTEST_END_MARKER,
+};
+
+static struct bst_test_list *install(struct bst_test_list *tests)
+{
+	return bst_add_tests(tests, entrypoints);
+};
+
+bst_test_install_t test_installers[] = {install, NULL};
+
+int main(void)
+{
+	bst_main();
+
+	return 0;
+}

tests/bsim/bluetooth/host/l2cap/ecred/peer/src/peer.c

@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2024 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <zephyr/kernel.h>
+#include <zephyr/bluetooth/bluetooth.h>
+#include <zephyr/bluetooth/conn.h>
+#include <zephyr/bluetooth/att.h>
+#include <zephyr/bluetooth/gatt.h>
+#include <zephyr/logging/log.h>
+
+#include "testlib/adv.h"
+#include "testlib/att_read.h"
+#include "testlib/att_write.h"
+#include "testlib/conn.h"
+
+#include "babblekit/flags.h"
+#include "babblekit/testcase.h"
+
+void entrypoint_peer(void)
+{
+	int err;
+	struct bt_conn *conn;
+
+	/* Mark test as in progress. */
+	TEST_START("peer");
+
+	/* Initialize Bluetooth */
+	err = bt_enable(NULL);
+	TEST_ASSERT(err == 0, "Can't enable Bluetooth (err %d)", err);
+
+	err = bt_testlib_adv_conn(&conn, BT_ID_DEFAULT, bt_get_name());
+	TEST_ASSERT(!err, "Failed to start connectable advertising (err %d)", err);
+
+	TEST_PASS("peer");
+}

tests/bsim/bluetooth/host/l2cap/ecred/test_scripts/_compile.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+# Copyright 2023 Nordic Semiconductor ASA
+# SPDX-License-Identifier: Apache-2.0
+set -eu
+: "${ZEPHYR_BASE:?ZEPHYR_BASE must be defined}"
+
+INCR_BUILD=1
+
+source ${ZEPHYR_BASE}/tests/bsim/compile.source
+
+app="$(guess_test_relpath)/dut" compile
+app="$(guess_test_relpath)/peer" compile
+
+wait_for_background_jobs

tests/bsim/bluetooth/host/l2cap/ecred/test_scripts/run.sh

@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+# Copyright (c) 2024 Nordic Semiconductor
+# SPDX-License-Identifier: Apache-2.0
+
+set -eu
+
+source ${ZEPHYR_BASE}/tests/bsim/sh_common.source
+
+test_name="$(guess_test_long_name)"
+
+simulation_id=${test_name}
+verbosity_level=2
+
+SIM_LEN_US=$((1 * 1000 * 1000))
+
+dut_exe="${BSIM_OUT_PATH}/bin/bs_${BOARD_TS}_${test_name}_dut_prj_conf"
+peer_exe="${BSIM_OUT_PATH}/bin/bs_${BOARD_TS}_${test_name}_peer_prj_conf"
+
+cd ${BSIM_OUT_PATH}/bin
+
+Execute "${dut_exe}" -v=${verbosity_level} -s=${simulation_id} -d=0 -rs=420 -testid=dut
+Execute "${peer_exe}" -v=${verbosity_level} -s=${simulation_id} -d=1 -rs=69  -testid=peer
+
+Execute ./bs_2G4_phy_v1 -v=${verbosity_level} -s=${simulation_id} -D=2 -sim_length=${SIM_LEN_US} $@
+
+wait_for_background_jobs