Re: [PATCH v4] Bluetooth: l2cap: defer conn param update to avoid conn->lock/hdev->lock inversion

[Luiz Augusto von Dentz]

Hi Mikhail,

On Tue, Apr 14, 2026 at 2:44 PM Mikhail Gavrilov
<mikhail.v.gavrilov@xxxxxxxxx> 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 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 sync callback uses __hci_cmd_sync_status_sk() to
> wait for the HCI_EV_LE_CONN_UPDATE_COMPLETE event, then updates the
> stored connection parameters (hci_conn_params) and notifies userspace
> (mgmt_new_conn_param) only after the controller has confirmed the update.
>
> A reference on hci_conn is held via hci_conn_get()/hci_conn_put() for
> the lifetime of the queued work to prevent use-after-free, and
> hci_conn_valid() is checked before proceeding in case the connection was
> removed while the work was pending.
>
> Fixes: f044eb0524a0 ("Bluetooth: Store latency and supervision timeout in connection params")
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@xxxxxxxxx>
> Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> ---
>
> Changes in v4 (Luiz Augusto von Dentz, Sashiko/Gemini AI review):
> - Use hci_conn_get()/hci_conn_put() to hold a reference while work is queued
> - Use __hci_cmd_sync_status_sk() to wait for HCI_EV_LE_CONN_UPDATE_COMPLETE,
>   then do params update + mgmt notification in the sync callback — removes the
>   need for hci_event.c changes and hci_sent_cmd_data()
> - Use kzalloc_obj() per checkpatch recommendation
> - Fix checkpatch alignment issues
>
> 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         | 98 +++++++++++++++++++++++++-------
>  net/bluetooth/l2cap_core.c       | 12 +---
>  3 files changed, 82 insertions(+), 30 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..c9cc18fab1f2 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -480,40 +480,100 @@ 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 le_conn_update_data *d = data;
> +       struct hci_conn *conn = d->conn;
>         struct hci_conn_params *params;
>         struct hci_cp_le_conn_update cp;
> +       u8 store_hint;
> +       int err;
>
> +       /* Verify connection is still alive. */
>         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;
> +       if (!hci_conn_valid(hdev, conn)) {
> +               hci_dev_unlock(hdev);
> +               return -ECANCELED;
>         }
> -
>         hci_dev_unlock(hdev);
>
>         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);
> +       err = __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_CONN_UPDATE,
> +                                      sizeof(cp), &cp,
> +                                      HCI_EV_LE_CONN_UPDATE_COMPLETE,
> +                                      HCI_CMD_TIMEOUT, NULL);
> +       if (err)
> +               return err;
> +
> +       /* Update stored connection parameters after the controller has
> +        * confirmed the update via the LE Connection Update Complete event.
> +        */
> +       hci_dev_lock(hdev);
>
> -       if (params)
> -               return 0x01;
> +       params = hci_conn_params_lookup(hdev, &conn->dst, conn->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;
> +       } else {
> +               store_hint = 0x00;
> +       }
>
> -       return 0x00;
> +       hci_dev_unlock(hdev);
> +
> +       mgmt_new_conn_param(hdev, &conn->dst, conn->dst_type, store_hint,
> +                           d->min, d->max, d->latency, d->to_multiplier);
> +
> +       return 0;
> +}
> +
> +static void le_conn_update_complete(struct hci_dev *hdev, void *data, int err)
> +{
> +       struct le_conn_update_data *d = data;
> +
> +       hci_conn_put(d->conn);
> +       kfree(d);
> +}
> +
> +void hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> +                       u16 to_multiplier)
> +{
> +       struct le_conn_update_data *d;
> +
> +       d = kzalloc_obj(*d);
> +       if (!d)
> +               return;
> +
> +       hci_conn_get(conn);
> +       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) {
> +               hci_conn_put(conn);
> +               kfree(d);
> +       }
>  }
>
>  void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __le64 rand,
> 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

https://sashiko.dev/#/patchset/20260414184415.17114-1-mikhail.v.gavrilov%40gmail.com

1. I don't think it really matters because connection intervals are
typically 60ms at most, so 6*60 = 360ms. That said, perhaps we should
use 6 * max_interval as the timeout since that is how long the
controller has to update the connection interval, well if the 6
consecutive intervals are actually a thing since I don't recall
anything like that being mentioned in the command, but perhaps it is
in the link layer. But if it times out I don't think there is a way to
cancel the change either, so I'm not sure how we'll be able to
synchronize if the controller misbehaves.
2. Well I guess that is would need to defer the response and wait the
HCI proceedure to complete to really respond properly, but that would
mean the remote could get a LL procedure to change the connection
interval before the L2CAP response, so I can see this going wrong
either way.


-- 
Luiz Augusto von Dentz