net/bluetooth/l2cap_core.c | 93 ++++++++++++++++++++++++++++++++++---- 1 file changed, 85 insertions(+), 8 deletions(-)
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 introducing l2cap_conn_param_update_sync() which is scheduled
via hci_cmd_sync_queue() to run on the hci_cmd_sync workqueue, outside
any of the involved locks. The necessary connection parameters are
captured into a heap-allocated struct conn_param_update_data and the
sync callback performs the hci_conn_params update, sends the HCI LE
Connection Update command, and notifies mgmt of the new parameters.
To guard against a theoretical handle-reuse race (the connection could
drop and its handle be reassigned between queuing and execution), the
sync callback verifies the connection still exists and its destination
address matches the original request before proceeding.
The allocation is performed before sending the L2CAP_CONN_PARAM_ACCEPTED
response so that on OOM the peer receives a REJECTED response instead of
an inconsistent state where the peer applies new parameters but the local
controller does not.
Fixes: f044eb0524a0 ("Bluetooth: Store latency and supervision timeout in connection params")
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
---
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
net/bluetooth/l2cap_core.c | 93 ++++++++++++++++++++++++++++++++++----
1 file changed, 85 insertions(+), 8 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 95c65fece39b..94dfef9df498 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4670,6 +4670,70 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn,
return 0;
}
+struct conn_param_update_data {
+ u16 handle;
+ bdaddr_t dst;
+ u8 dst_type;
+ u16 min;
+ u16 max;
+ u16 latency;
+ u16 to_multiplier;
+};
+
+static int l2cap_conn_param_update_sync(struct hci_dev *hdev, void *data)
+{
+ struct conn_param_update_data *d = data;
+ struct hci_conn *conn;
+ struct hci_conn_params *params;
+ struct hci_cp_le_conn_update cp;
+ u8 store_hint = 0x00;
+
+ /* Verify the connection is still alive and matches the original
+ * request, since the handle could theoretically be reused after
+ * a disconnect/reconnect cycle.
+ */
+ hci_dev_lock(hdev);
+
+ conn = hci_conn_hash_lookup_handle(hdev, d->handle);
+ if (!conn || bacmp(&conn->dst, &d->dst)) {
+ hci_dev_unlock(hdev);
+ return 0;
+ }
+
+ params = hci_conn_params_lookup(hdev, &d->dst, d->dst_type);
+ if (params) {
+ params->conn_min_interval = d->min;
+ params->conn_max_interval = d->max;
+ params->conn_latency = d->latency;
+ params->supervision_timeout = d->to_multiplier;
+ store_hint = 0x01;
+ }
+
+ hci_dev_unlock(hdev);
+
+ memset(&cp, 0, sizeof(cp));
+ cp.handle = cpu_to_le16(d->handle);
+ 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);
+
+ mgmt_new_conn_param(hdev, &d->dst, d->dst_type, store_hint,
+ d->min, d->max, d->latency, d->to_multiplier);
+
+ return 0;
+}
+
+static void l2cap_conn_param_update_destroy(struct hci_dev *hdev, void *data,
+ int err)
+{
+ kfree(data);
+}
+
static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
struct l2cap_cmd_hdr *cmd,
u16 cmd_len, u8 *data)
@@ -4677,6 +4741,7 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
struct hci_conn *hcon = conn->hcon;
struct l2cap_conn_param_update_req *req;
struct l2cap_conn_param_update_rsp rsp;
+ struct conn_param_update_data *d;
u16 min, max, latency, to_multiplier;
int err;
@@ -4703,18 +4768,30 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
else
rsp.result = cpu_to_le16(L2CAP_CONN_PARAM_ACCEPTED);
+ if (!err) {
+ d = kmalloc(sizeof(*d), GFP_KERNEL);
+ if (!d) {
+ rsp.result = cpu_to_le16(L2CAP_CONN_PARAM_REJECTED);
+ err = -ENOMEM;
+ }
+ }
+
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);
-
+ d->handle = hcon->handle;
+ bacpy(&d->dst, &hcon->dst);
+ d->dst_type = hcon->dst_type;
+ d->min = min;
+ d->max = max;
+ d->latency = latency;
+ d->to_multiplier = to_multiplier;
+
+ if (hci_cmd_sync_queue(hcon->hdev,
+ l2cap_conn_param_update_sync, d,
+ l2cap_conn_param_update_destroy) < 0)
+ kfree(d);
}
return 0;
--
2.53.0Hi Mikhail,
On Fri, Apr 10, 2026 at 2:30 AM Mikhail Gavrilov
<mikhail.v.gavrilov@gmail.com> wrote:
>
> 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 introducing l2cap_conn_param_update_sync() which is scheduled
> via hci_cmd_sync_queue() to run on the hci_cmd_sync workqueue, outside
> any of the involved locks. The necessary connection parameters are
> captured into a heap-allocated struct conn_param_update_data and the
> sync callback performs the hci_conn_params update, sends the HCI LE
> Connection Update command, and notifies mgmt of the new parameters.
>
> To guard against a theoretical handle-reuse race (the connection could
> drop and its handle be reassigned between queuing and execution), the
> sync callback verifies the connection still exists and its destination
> address matches the original request before proceeding.
>
> The allocation is performed before sending the L2CAP_CONN_PARAM_ACCEPTED
> response so that on OOM the peer receives a REJECTED response instead of
> an inconsistent state where the peer applies new parameters but the local
> controller does not.
>
> Fixes: f044eb0524a0 ("Bluetooth: Store latency and supervision timeout in connection params")
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> ---
>
> 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
>
> net/bluetooth/l2cap_core.c | 93 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 85 insertions(+), 8 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 95c65fece39b..94dfef9df498 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4670,6 +4670,70 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn,
> return 0;
> }
>
> +struct conn_param_update_data {
> + u16 handle;
> + bdaddr_t dst;
> + u8 dst_type;
> + u16 min;
> + u16 max;
> + u16 latency;
> + u16 to_multiplier;
> +};
> +
> +static int l2cap_conn_param_update_sync(struct hci_dev *hdev, void *data)
> +{
> + struct conn_param_update_data *d = data;
> + struct hci_conn *conn;
> + struct hci_conn_params *params;
> + struct hci_cp_le_conn_update cp;
> + u8 store_hint = 0x00;
> +
> + /* Verify the connection is still alive and matches the original
> + * request, since the handle could theoretically be reused after
> + * a disconnect/reconnect cycle.
> + */
> + hci_dev_lock(hdev);
> +
> + conn = hci_conn_hash_lookup_handle(hdev, d->handle);
> + if (!conn || bacmp(&conn->dst, &d->dst)) {
> + hci_dev_unlock(hdev);
> + return 0;
> + }
> +
> + params = hci_conn_params_lookup(hdev, &d->dst, d->dst_type);
> + if (params) {
> + params->conn_min_interval = d->min;
> + params->conn_max_interval = d->max;
> + params->conn_latency = d->latency;
> + params->supervision_timeout = d->to_multiplier;
> + store_hint = 0x01;
> + }
> +
> + hci_dev_unlock(hdev);
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.handle = cpu_to_le16(d->handle);
> + 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);
> +
> + mgmt_new_conn_param(hdev, &d->dst, d->dst_type, store_hint,
> + d->min, d->max, d->latency, d->to_multiplier);
Actually most of this can be defered directly to
hci_event.c:hci_le_conn_update_complete_evt, then use
hci_sent_cmd_data(hdev, HCI_OP_LE_CONN_UPDATE) to fetch the command
parameters.
> +
> + return 0;
> +}
> +
> +static void l2cap_conn_param_update_destroy(struct hci_dev *hdev, void *data,
> + int err)
> +{
> + kfree(data);
> +}
> +
> static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
> struct l2cap_cmd_hdr *cmd,
> u16 cmd_len, u8 *data)
> @@ -4677,6 +4741,7 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
> struct hci_conn *hcon = conn->hcon;
> struct l2cap_conn_param_update_req *req;
> struct l2cap_conn_param_update_rsp rsp;
> + struct conn_param_update_data *d;
> u16 min, max, latency, to_multiplier;
> int err;
>
> @@ -4703,18 +4768,30 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
> else
> rsp.result = cpu_to_le16(L2CAP_CONN_PARAM_ACCEPTED);
>
> + if (!err) {
> + d = kmalloc(sizeof(*d), GFP_KERNEL);
> + if (!d) {
> + rsp.result = cpu_to_le16(L2CAP_CONN_PARAM_REJECTED);
> + err = -ENOMEM;
> + }
> + }
> +
> 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);
Just make hci_le_conn_update internally use hci_cmd_sync_queue and
then handle the update values at hci_le_conn_update_complete_evt.
> -
> + d->handle = hcon->handle;
> + bacpy(&d->dst, &hcon->dst);
> + d->dst_type = hcon->dst_type;
> + d->min = min;
> + d->max = max;
> + d->latency = latency;
> + d->to_multiplier = to_multiplier;
> +
> + if (hci_cmd_sync_queue(hcon->hdev,
> + l2cap_conn_param_update_sync, d,
> + l2cap_conn_param_update_destroy) < 0)
> + kfree(d);
> }
>
> return 0;
> --
> 2.53.0
>
--
Luiz Augusto von Dentz
Dear Mikhail,
Thank you for the improved version. (Just a note, that there are two
spaces in the summary after *to avoid*. No need to resend.)
Am 10.04.26 um 08:30 schrieb 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).
No need to resend, but out of curiosity, is the result, that the device
does not work?
> Fix this by introducing l2cap_conn_param_update_sync() which is scheduled
> via hci_cmd_sync_queue() to run on the hci_cmd_sync workqueue, outside
> any of the involved locks. The necessary connection parameters are
> captured into a heap-allocated struct conn_param_update_data and the
> sync callback performs the hci_conn_params update, sends the HCI LE
> Connection Update command, and notifies mgmt of the new parameters.
>
> To guard against a theoretical handle-reuse race (the connection could
> drop and its handle be reassigned between queuing and execution), the
> sync callback verifies the connection still exists and its destination
> address matches the original request before proceeding.
>
> The allocation is performed before sending the L2CAP_CONN_PARAM_ACCEPTED
> response so that on OOM the peer receives a REJECTED response instead of
> an inconsistent state where the peer applies new parameters but the local
> controller does not.
>
> Fixes: f044eb0524a0 ("Bluetooth: Store latency and supervision timeout in connection params")
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> ---
>
> 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
>
> net/bluetooth/l2cap_core.c | 93 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 85 insertions(+), 8 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 95c65fece39b..94dfef9df498 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4670,6 +4670,70 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn,
> return 0;
> }
>
> +struct conn_param_update_data {
> + u16 handle;
> + bdaddr_t dst;
> + u8 dst_type;
> + u16 min;
> + u16 max;
> + u16 latency;
> + u16 to_multiplier;
> +};
> +
> +static int l2cap_conn_param_update_sync(struct hci_dev *hdev, void *data)
> +{
> + struct conn_param_update_data *d = data;
> + struct hci_conn *conn;
> + struct hci_conn_params *params;
> + struct hci_cp_le_conn_update cp;
> + u8 store_hint = 0x00;
> +
> + /* Verify the connection is still alive and matches the original
> + * request, since the handle could theoretically be reused after
> + * a disconnect/reconnect cycle.
> + */
> + hci_dev_lock(hdev);
> +
> + conn = hci_conn_hash_lookup_handle(hdev, d->handle);
> + if (!conn || bacmp(&conn->dst, &d->dst)) {
> + hci_dev_unlock(hdev);
> + return 0;
> + }
> +
> + params = hci_conn_params_lookup(hdev, &d->dst, d->dst_type);
> + if (params) {
> + params->conn_min_interval = d->min;
> + params->conn_max_interval = d->max;
> + params->conn_latency = d->latency;
> + params->supervision_timeout = d->to_multiplier;
> + store_hint = 0x01;
> + }
> +
> + hci_dev_unlock(hdev);
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.handle = cpu_to_le16(d->handle);
> + 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);
> +
> + mgmt_new_conn_param(hdev, &d->dst, d->dst_type, store_hint,
> + d->min, d->max, d->latency, d->to_multiplier);
> +
> + return 0;
> +}
> +
> +static void l2cap_conn_param_update_destroy(struct hci_dev *hdev, void *data,
> + int err)
> +{
> + kfree(data);
> +}
> +
> static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
> struct l2cap_cmd_hdr *cmd,
> u16 cmd_len, u8 *data)
> @@ -4677,6 +4741,7 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
> struct hci_conn *hcon = conn->hcon;
> struct l2cap_conn_param_update_req *req;
> struct l2cap_conn_param_update_rsp rsp;
> + struct conn_param_update_data *d;
> u16 min, max, latency, to_multiplier;
> int err;
>
> @@ -4703,18 +4768,30 @@ static inline int l2cap_conn_param_update_req(struct l2cap_conn *conn,
> else
> rsp.result = cpu_to_le16(L2CAP_CONN_PARAM_ACCEPTED);
>
> + if (!err) {
> + d = kmalloc(sizeof(*d), GFP_KERNEL);
> + if (!d) {
> + rsp.result = cpu_to_le16(L2CAP_CONN_PARAM_REJECTED);
> + err = -ENOMEM;
> + }
> + }
> +
> 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);
> -
> + d->handle = hcon->handle;
> + bacpy(&d->dst, &hcon->dst);
> + d->dst_type = hcon->dst_type;
> + d->min = min;
> + d->max = max;
> + d->latency = latency;
> + d->to_multiplier = to_multiplier;
> +
> + if (hci_cmd_sync_queue(hcon->hdev,
> + l2cap_conn_param_update_sync, d,
> + l2cap_conn_param_update_destroy) < 0)
> + kfree(d);
> }
>
> return 0;
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
On Fri, Apr 10, 2026 at 12:29 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > No need to resend, but out of curiosity, is the result, that the device > does not work? Hi Paul, Thanks for the review! The lockdep warning did not noticeably affect the FC-49 functionality — MIDI events were received fine via `aseqdump -p 128` both before and after the patch. The warning is about a potential deadlock that lockdep detected through lock ordering analysis, not one that was actually hit during my testing. That said, under the right timing conditions the deadlock is real and could hang the workqueue. -- Thanks, Mike Gavrilov.
© 2016 - 2026 Red Hat, Inc.