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

[Luiz Augusto von Dentz]

Hi Pauli,

On Tue, Apr 14, 2026 at 4:26 PM Pauli Virtanen <pav@xxxxxx> wrote:
>
> ti, 2026-04-14 kello 23:43 +0500, Mikhail Gavrilov kirjoitti:
> > 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);
>
> hci_conn_valid() should be in the same hci_dev_lock() critical section
> as where the conn is dereferenced, otherwise the lock is not doing
> something useful.

This means we should only unlock after we finish using hci_conn,
including conn->handle above.

> > -     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;
> >  }



-- 
Luiz Augusto von Dentz