Bluetooth: Host: Fix L2CAP EINPROGRESS refs

include/zephyr/bluetooth/l2cap.h

@@ -349,6 +349,11 @@ struct bt_l2cap_chan_ops {
 	 *  @kconfig{CONFIG_BT_L2CAP_SEG_RECV} is enabled and seg_recv is
 	 *  supplied.
 	 *
+	 *  If the application returns @c -EINPROGRESS, the application takes
+	 *  ownership of the reference in @p buf. (I.e. This pointer value can
+	 *  simply be given to @ref bt_l2cap_chan_recv_complete without any
+	 *  calls @ref net_buf_ref or @ref net_buf_unref.)
+	 *
 	 *  @return 0 in case of success or negative value in case of error.
 	 *  @return -EINPROGRESS in case where user has to confirm once the data
 	 *                       has been processed by calling

include/zephyr/bluetooth/testing.h

@@ -0,0 +1,27 @@
+/* Copyright (c) 2024 Nordic Semiconductor ASA
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/** @brief Internal testing interfaces for Bluetooth
+ *  @file
+ *  @internal
+ *
+ *  The interfaces in this file are internal and not stable.
+ */
+
+#ifndef ZEPHYR_INCLUDE_BLUETOOTH_TESTING_H_
+#define ZEPHYR_INCLUDE_BLUETOOTH_TESTING_H_
+
+#include <zephyr/net_buf.h>
+
+/** @brief Hook for `acl_in_pool.destroy`
+ *
+ *  Weak-function interface. The user can simply define this
+ *  function, and it will automatically become the event
+ *  listener.
+ *
+ *  @kconfig_dep{CONFIG_BT_TESTING}
+ */
+void bt_testing_trace_event_acl_pool_destroy(struct net_buf *buf);
+
+#endif /* ZEPHYR_INCLUDE_BLUETOOTH_TESTING_H_ */

subsys/bluetooth/host/hci_core.c

@@ -14,6 +14,7 @@
 #include <zephyr/net_buf.h>
 #include <zephyr/sys/atomic.h>
 #include <zephyr/sys/check.h>
+#include <zephyr/sys/util_macro.h>
 #include <zephyr/sys/util.h>
 #include <zephyr/sys/slist.h>
 #include <zephyr/sys/byteorder.h>
@@ -28,6 +29,7 @@
 #include <zephyr/bluetooth/l2cap.h>
 #include <zephyr/bluetooth/hci.h>
 #include <zephyr/bluetooth/hci_vs.h>
+#include <zephyr/bluetooth/testing.h>
 #if DT_HAS_CHOSEN(zephyr_bt_hci)
 #include <zephyr/drivers/bluetooth.h>
 #else
@@ -263,13 +265,23 @@ void bt_send_one_host_num_completed_packets(uint16_t handle)
 	BT_ASSERT_MSG(err == 0, "Unable to send Host NCP (err %d)", err);
 }
 
+#if defined(CONFIG_BT_TESTING)
+__weak void bt_testing_trace_event_acl_pool_destroy(struct net_buf *buf)
+{
+}
+#endif
+
 #if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
 void bt_hci_host_num_completed_packets(struct net_buf *buf)
 {
 	uint16_t handle = acl(buf)->handle;
 	struct bt_conn *conn;
 	uint8_t index = acl(buf)->index;
 
+	if (IS_ENABLED(CONFIG_BT_TESTING)) {
+		bt_testing_trace_event_acl_pool_destroy(buf);
+	}
+
 	net_buf_destroy(buf);
 
 	if (acl(buf)->host_ncp_sent) {

subsys/bluetooth/host/l2cap.c

@@ -17,6 +17,7 @@
 #include <zephyr/sys/byteorder.h>
 #include <zephyr/sys/math_extras.h>
 #include <zephyr/sys/util.h>
+#include <zephyr/net_buf.h>
 
 #include <zephyr/bluetooth/hci.h>
 #include <zephyr/bluetooth/bluetooth.h>
@@ -2566,6 +2567,7 @@ static void l2cap_chan_le_recv_seg_direct(struct bt_l2cap_le_chan *chan, struct
 static void l2cap_chan_le_recv(struct bt_l2cap_le_chan *chan,
 			       struct net_buf *buf)
 {
+	struct net_buf *owned_ref;
 	uint16_t sdu_len;
 	int err;
 
@@ -2639,7 +2641,13 @@ static void l2cap_chan_le_recv(struct bt_l2cap_le_chan *chan,
 		return;
 	}
 
-	err = chan->chan.ops->recv(&chan->chan, buf);
+	owned_ref = net_buf_ref(buf);
+	err = chan->chan.ops->recv(&chan->chan, owned_ref);
+	if (err != -EINPROGRESS) {
+		net_buf_unref(owned_ref);
+		owned_ref = NULL;
+	}
+
 	if (err < 0) {
 		if (err != -EINPROGRESS) {
 			LOG_ERR("err %d", err);
@@ -3139,7 +3147,7 @@ static int bt_l2cap_dyn_chan_send(struct bt_l2cap_le_chan *le_chan, struct net_b
 		/* Call `net_buf_reserve(buf, BT_L2CAP_SDU_CHAN_SEND_RESERVE)`
 		 * when allocating buffers intended for bt_l2cap_chan_send().
 		 */
-		LOG_DBG("Not enough headroom in buf %p", buf);
+		LOG_ERR("Not enough headroom in buf %p", buf);
 		return -EINVAL;
 	}
 
@@ -3187,7 +3195,7 @@ int bt_l2cap_chan_send(struct bt_l2cap_chan *chan, struct net_buf *buf)
 	LOG_DBG("chan %p buf %p len %zu", chan, buf, buf->len);
 
 	if (buf->ref != 1) {
-		LOG_DBG("Expecting 1 ref, got %d", buf->ref);
+		LOG_WRN("Expecting 1 ref, got %d", buf->ref);
 		return -EINVAL;
 	}
 

tests/bluetooth/common/testlib/include/testlib/addr.h

@@ -0,0 +1,20 @@
+/* Copyright (c) 2024 Nordic Semiconductor ASA
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#ifndef ZEPHYR_TESTS_BLUETOOTH_COMMON_TESTLIB_INCLUDE_TESTLIB_ADDR_H_
+#define ZEPHYR_TESTS_BLUETOOTH_COMMON_TESTLIB_INCLUDE_TESTLIB_ADDR_H_
+
+#include <stdint.h>
+
+#include <zephyr/bluetooth/addr.h>
+#include <zephyr/bluetooth/byteorder.h>
+
+/** Bluetooth LE static random address */
+#define BT_TESTLIB_ADDR_LE_RANDOM_C0_00_00_00_00_(last)                                            \
+	((bt_addr_le_t){                                                                           \
+		.type = BT_ADDR_LE_RANDOM,                                                         \
+		.a = {{last, 0x00, 0x00, 0x00, 0x00, 0xc0}},                                       \
+	})
+
+#endif /* ZEPHYR_TESTS_BLUETOOTH_COMMON_TESTLIB_INCLUDE_TESTLIB_ADDR_H_ */

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

@@ -19,6 +19,7 @@ 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
 run_in_background ${ZEPHYR_BASE}/tests/bsim/bluetooth/host/l2cap/split/compile.sh
 run_in_background ${ZEPHYR_BASE}/tests/bsim/bluetooth/host/l2cap/reassembly/compile.sh
+run_in_background ${ZEPHYR_BASE}/tests/bsim/bluetooth/host/l2cap/einprogress/compile.sh
 run_in_background ${ZEPHYR_BASE}/tests/bsim/bluetooth/host/l2cap/ecred/compile.sh
 app=tests/bsim/bluetooth/host/l2cap/credits compile
 app=tests/bsim/bluetooth/host/l2cap/credits conf_file=prj_ecred.conf compile

tests/bsim/bluetooth/host/l2cap/einprogress/CMakeLists.txt

@@ -0,0 +1,24 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.20.0)
+
+find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
+
+project(test_l2cap_einprogress)
+
+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
+  src/tester.c
+)

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

@@ -0,0 +1,13 @@
+#!/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)" compile
+
+wait_for_background_jobs

tests/bsim/bluetooth/host/l2cap/einprogress/prj.conf

@@ -0,0 +1,25 @@
+CONFIG_LOG=y
+CONFIG_ASSERT=y
+CONFIG_THREAD_NAME=y
+CONFIG_LOG_THREAD_ID_PREFIX=y
+CONFIG_ARCH_POSIX_TRAP_ON_FATAL=y
+CONFIG_BT_TESTING=y
+
+CONFIG_BT_HCI_ACL_FLOW_CONTROL=y
+
+CONFIG_BT=y
+CONFIG_BT_CENTRAL=y
+CONFIG_BT_PERIPHERAL=y
+
+# Dependency of testlib/adv and testlib/scan.
+CONFIG_BT_EXT_ADV=y
+
+# Dynamic channel depends on SMP
+CONFIG_BT_SMP=y
+CONFIG_BT_L2CAP_DYNAMIC_CHANNEL=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

tests/bsim/bluetooth/host/l2cap/einprogress/src/data.h

@@ -0,0 +1,11 @@
+/* Copyright (c) 2024 Nordic Semiconductor ASA
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#ifndef ZEPHYR_TESTS_BSIM_BLUETOOTH_HOST_L2CAP_EINPROGRESS_SRC_DATA_H_
+#define ZEPHYR_TESTS_BSIM_BLUETOOTH_HOST_L2CAP_EINPROGRESS_SRC_DATA_H_
+
+#define TEST_DATA_L2CAP_PSM 0x0080
+#define TEST_DATA_DUT_ADDR  BT_TESTLIB_ADDR_LE_RANDOM_C0_00_00_00_00_(0x01)
+
+#endif /* ZEPHYR_TESTS_BSIM_BLUETOOTH_HOST_L2CAP_EINPROGRESS_SRC_DATA_H_ */

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

@@ -0,0 +1,124 @@
+/* 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/bluetooth/testing.h>
+#include <zephyr/logging/log.h>
+#include <zephyr/net_buf.h>
+#include <zephyr/sys/__assert.h>
+#include <zephyr/sys/atomic.h>
+#include <zephyr/sys/util_macro.h>
+
+#include <testlib/addr.h>
+#include <testlib/adv.h>
+#include <testlib/conn.h>
+#include <testlib/scan.h>
+
+#include <babblekit/flags.h>
+#include <babblekit/testcase.h>
+
+#include "data.h"
+
+LOG_MODULE_REGISTER(dut, LOG_LEVEL_INF);
+
+/** Here we keep track of the reference count in the test
+ *  application. This allows us to notice if the stack has freed
+ *  references that were ours.
+ */
+static atomic_t acl_pool_refs_held[CONFIG_BT_BUF_ACL_RX_COUNT];
+
+BUILD_ASSERT(IS_ENABLED(CONFIG_BT_TESTING));
+BUILD_ASSERT(IS_ENABLED(CONFIG_BT_HCI_ACL_FLOW_CONTROL));
+void bt_testing_trace_event_acl_pool_destroy(struct net_buf *destroyed_buf)
+{
+	int buf_id = net_buf_id(destroyed_buf);
+
+	__ASSERT_NO_MSG(0 <= buf_id && buf_id < ARRAY_SIZE(acl_pool_refs_held));
+	TEST_ASSERT(acl_pool_refs_held[buf_id] == 0,
+		    "ACL buf was destroyed while tester still held a reference");
+}
+
+static void acl_pool_refs_held_add(struct net_buf *buf)
+{
+	int buf_id = net_buf_id(buf);
+
+	__ASSERT_NO_MSG(0 <= buf_id && buf_id < CONFIG_BT_BUF_ACL_RX_COUNT);
+	atomic_inc(&acl_pool_refs_held[buf_id]);
+}
+
+static void acl_pool_refs_held_remove(struct net_buf *buf)
+{
+	int buf_id = net_buf_id(buf);
+
+	__ASSERT_NO_MSG(0 <= buf_id && buf_id < ARRAY_SIZE(acl_pool_refs_held));
+	atomic_val_t old = atomic_dec(&acl_pool_refs_held[buf_id]);
+
+	__ASSERT(old != 0, "Tester error: releasing a reference that was not held");
+}
+
+struct k_fifo ack_todo;
+
+static int dut_chan_recv_cb(struct bt_l2cap_chan *chan, struct net_buf *buf)
+{
+	/* Move buf. Ownership is ours if we return -EINPROGRESS. */
+	acl_pool_refs_held_add(buf);
+	k_fifo_put(&ack_todo, buf);
+
+	return -EINPROGRESS;
+}
+
+static const struct bt_l2cap_chan_ops ops = {
+	.recv = dut_chan_recv_cb,
+};
+
+static struct bt_l2cap_le_chan le_chan = {
+	.chan.ops = &ops,
+};
+
+static int dut_server_accept_cb(struct bt_conn *conn, struct bt_l2cap_server *server,
+				struct bt_l2cap_chan **chan)
+{
+	*chan = &le_chan.chan;
+	return 0;
+}
+
+static struct bt_l2cap_server test_l2cap_server = {
+	.accept = dut_server_accept_cb,
+	.psm = TEST_DATA_L2CAP_PSM,
+};
+
+void entrypoint_dut(void)
+{
+	struct net_buf *ack_buf;
+	struct bt_conn *conn = NULL;
+	int err;
+
+	TEST_START("dut");
+
+	k_fifo_init(&ack_todo);
+
+	err = bt_id_create(&TEST_DATA_DUT_ADDR, NULL);
+	__ASSERT_NO_MSG(!err);
+
+	err = bt_enable(NULL);
+	__ASSERT_NO_MSG(!err);
+
+	err = bt_l2cap_server_register(&test_l2cap_server);
+	__ASSERT_NO_MSG(!err);
+
+	err = bt_testlib_adv_conn(&conn, BT_ID_DEFAULT, NULL);
+	__ASSERT_NO_MSG(!err);
+
+	ack_buf = k_fifo_get(&ack_todo, K_FOREVER);
+	__ASSERT_NO_MSG(ack_buf);
+
+	acl_pool_refs_held_remove(ack_buf);
+	err = bt_l2cap_chan_recv_complete(&le_chan.chan, ack_buf);
+	TEST_ASSERT(!err);
+
+	TEST_PASS_AND_EXIT("dut");
+}