public inbox for linux-bluetooth@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] Bluetooth: l2cap: defer conn param update to avoid conn->lock/hdev->lock inversion
@ 2026-04-13 23:08 Mikhail Gavrilov
  2026-04-13 23:52 ` [v3] " bluez.test.bot
  2026-04-14 14:46 ` [PATCH v3] " Luiz Augusto von Dentz
  0 siblings, 2 replies; 3+ messages in thread
From: Mikhail Gavrilov @ 2026-04-13 23:08 UTC (permalink / raw)
  To: luiz.dentz, marcel
  Cc: pmenzel, linux-bluetooth, linux-kernel, Mikhail Gavrilov

When a BLE peripheral sends an L2CAP Connection Parameter Update Request
the processing path is:

  process_pending_rx()          [takes conn->lock]
    l2cap_le_sig_channel()
      l2cap_conn_param_update_req()
        hci_le_conn_update()    [takes hdev->lock]

Meanwhile other code paths take the locks in the opposite order:

  l2cap_chan_connect()          [takes hdev->lock]
    ...
      mutex_lock(&conn->lock)

  l2cap_conn_ready()            [hdev->lock via hci_cb_list_lock]
    ...
      mutex_lock(&conn->lock)

This is a classic AB/BA deadlock which lockdep reports as a circular
locking dependency when connecting a BLE MIDI keyboard (Carry-On FC-49).

Fix this by making hci_le_conn_update() defer the HCI command through
hci_cmd_sync_queue() so it no longer needs to take hdev->lock in the
caller context.

The connection parameter storage (hci_conn_params update) and userspace
notification (mgmt_new_conn_param) are moved into
hci_le_conn_update_complete_evt() where they are handled when the
controller confirms the update, using hci_sent_cmd_data() to retrieve
the originally requested min/max interval values.

Fixes: f044eb0524a0 ("Bluetooth: Store latency and supervision timeout in connection params")
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
---

Changes in v3 (Luiz Augusto von Dentz):
- Move hci_cmd_sync_queue into hci_le_conn_update itself instead of open-coding
  the deferral in l2cap_core.c
- Move conn_params update and mgmt_new_conn_param into
  hci_le_conn_update_complete_evt, using hci_sent_cmd_data to retrieve
  the originally requested parameters
 
Changes in v2 (Paul Menzel, Sashiko/Gemini AI review):
- Allocate before sending ACCEPTED response to avoid state mismatch on OOM
- Verify connection handle and address in sync callback against reuse race
- Expand commit message with implementation details

 include/net/bluetooth/hci_core.h |  2 +-
 net/bluetooth/hci_conn.c         | 66 ++++++++++++++++++++++----------
 net/bluetooth/hci_event.c        | 33 ++++++++++++++++
 net/bluetooth/l2cap_core.c       | 12 +-----
 4 files changed, 81 insertions(+), 32 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a7bffb908c1e..aa600fbf9a53 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -2495,7 +2495,7 @@ void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
 				  bdaddr_t *bdaddr, u8 addr_type);
 
 int hci_abort_conn(struct hci_conn *conn, u8 reason);
-u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
+void hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
 		      u16 to_multiplier);
 void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __le64 rand,
 		      __u8 ltk[16], __u8 key_size);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 11d3ad8d2551..207221560a02 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -480,40 +480,64 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 	return hci_setup_sync_conn(conn, handle);
 }
 
-u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
-		      u16 to_multiplier)
+struct le_conn_update_data {
+	struct hci_conn *conn;
+	u16	min;
+	u16	max;
+	u16	latency;
+	u16	to_multiplier;
+};
+
+static int le_conn_update_sync(struct hci_dev *hdev, void *data)
 {
-	struct hci_dev *hdev = conn->hdev;
-	struct hci_conn_params *params;
+	struct le_conn_update_data *d = data;
+	struct hci_conn *conn;
 	struct hci_cp_le_conn_update cp;
 
 	hci_dev_lock(hdev);
-
-	params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
-	if (params) {
-		params->conn_min_interval = min;
-		params->conn_max_interval = max;
-		params->conn_latency = latency;
-		params->supervision_timeout = to_multiplier;
-	}
-
+	/* Verify connection is still alive. */
+	conn = hci_conn_valid(hdev, d->conn) ? d->conn : NULL;
 	hci_dev_unlock(hdev);
+	if (!conn)
+		return -ECANCELED;
 
 	memset(&cp, 0, sizeof(cp));
 	cp.handle		= cpu_to_le16(conn->handle);
-	cp.conn_interval_min	= cpu_to_le16(min);
-	cp.conn_interval_max	= cpu_to_le16(max);
-	cp.conn_latency		= cpu_to_le16(latency);
-	cp.supervision_timeout	= cpu_to_le16(to_multiplier);
+	cp.conn_interval_min	= cpu_to_le16(d->min);
+	cp.conn_interval_max	= cpu_to_le16(d->max);
+	cp.conn_latency		= cpu_to_le16(d->latency);
+	cp.supervision_timeout	= cpu_to_le16(d->to_multiplier);
 	cp.min_ce_len		= cpu_to_le16(0x0000);
 	cp.max_ce_len		= cpu_to_le16(0x0000);
 
-	hci_send_cmd(hdev, HCI_OP_LE_CONN_UPDATE, sizeof(cp), &cp);
+	return __hci_cmd_sync_status(hdev, HCI_OP_LE_CONN_UPDATE,
+				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
+}
+
+static void le_conn_update_complete(struct hci_dev *hdev, void *data,
+					int err)
+{
+	kfree(data);
+}
 
-	if (params)
-		return 0x01;
+void hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
+		        u16 to_multiplier)
+{
+	struct le_conn_update_data *d;
 
-	return 0x00;
+	d = kmalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return;
+
+	d->conn = conn;
+	d->min = min;
+	d->max = max;
+	d->latency = latency;
+	d->to_multiplier = to_multiplier;
+
+	if (hci_cmd_sync_queue(conn->hdev, le_conn_update_sync, d,
+			       le_conn_update_complete) < 0)
+		kfree(d);
 }
 
 void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __le64 rand,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 3ebc5e6d45d9..9df8fc762a84 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6065,6 +6065,8 @@ static void hci_le_conn_update_complete_evt(struct hci_dev *hdev, void *data,
 					    struct sk_buff *skb)
 {
 	struct hci_ev_le_conn_update_complete *ev = data;
+	struct hci_cp_le_conn_update *sent;
+	struct hci_conn_params *params;
 	struct hci_conn *conn;
 
 	bt_dev_dbg(hdev, "status 0x%2.2x", ev->status);
@@ -6079,6 +6081,37 @@ static void hci_le_conn_update_complete_evt(struct hci_dev *hdev, void *data,
 		conn->le_conn_interval = le16_to_cpu(ev->interval);
 		conn->le_conn_latency = le16_to_cpu(ev->latency);
 		conn->le_supv_timeout = le16_to_cpu(ev->supervision_timeout);
+
+		/* Update stored connection parameters and notify userspace
+		 * using the parameters from the original HCI command.
+		 */
+		sent = hci_sent_cmd_data(hdev, HCI_OP_LE_CONN_UPDATE);
+		if (sent) {
+			u8 store_hint;
+
+			params = hci_conn_params_lookup(hdev, &conn->dst,
+							conn->dst_type);
+			if (params) {
+				params->conn_min_interval =
+					le16_to_cpu(sent->conn_interval_min);
+				params->conn_max_interval =
+					le16_to_cpu(sent->conn_interval_max);
+				params->conn_latency =
+					le16_to_cpu(sent->conn_latency);
+				params->supervision_timeout =
+					le16_to_cpu(sent->supervision_timeout);
+				store_hint = 0x01;
+			} else {
+				store_hint = 0x00;
+			}
+
+			mgmt_new_conn_param(hdev, &conn->dst, conn->dst_type,
+					    store_hint,
+					    le16_to_cpu(sent->conn_interval_min),
+					    le16_to_cpu(sent->conn_interval_max),
+					    le16_to_cpu(sent->conn_latency),
+					    le16_to_cpu(sent->supervision_timeout));
+		}
 	}
 
 	hci_dev_unlock(hdev);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 95c65fece39b..aac2db1d6fbb 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4706,16 +4706,8 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_CONN_PARAM_UPDATE_RSP,
 		       sizeof(rsp), &rsp);
 
-	if (!err) {
-		u8 store_hint;
-
-		store_hint = hci_le_conn_update(hcon, min, max, latency,
-						to_multiplier);
-		mgmt_new_conn_param(hcon->hdev, &hcon->dst, hcon->dst_type,
-				    store_hint, min, max, latency,
-				    to_multiplier);
-
-	}
+	if (!err)
+		hci_le_conn_update(hcon, min, max, latency, to_multiplier);
 
 	return 0;
 }
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-04-14 14:46 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-13 23:08 [PATCH v3] Bluetooth: l2cap: defer conn param update to avoid conn->lock/hdev->lock inversion Mikhail Gavrilov
2026-04-13 23:52 ` [v3] " bluez.test.bot
2026-04-14 14:46 ` [PATCH v3] " Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox