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

[Luiz Augusto von Dentz]

Hi Mikhail,

On Fri, Apr 10, 2026 at 2:30 AM 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 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@xxxxxxxxx>
> ---
>
> 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