Re: [PATCH v2 4/4] Bluetooth: hci_sync: hold hdev->lock for hci_conn_params lookups

[Pauli Virtanen]

Hi,

ma, 2026-07-06 kello 12:56 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sun, Jul 5, 2026 at 6:43 PM Pauli Virtanen <pav@xxxxxx> wrote:
> > 
> > hci_conn_params_lookup requires hdev->lock be held, otherwise the list
> > iteration or param access is not safe.
> > 
> > Hold hdev->lock for params lookups in hci_sync.
> > 
> > Fixes: c530569adc19 ("Bluetooth: hci_core: Introduce HCI_CONN_FLAG_PAST")
> > Signed-off-by: Pauli Virtanen <pav@xxxxxx>
> > ---
> > 
> > Notes:
> >     v2:
> >     - no change
> > 
> >  net/bluetooth/hci_sync.c | 20 +++++++++++++++++---
> >  1 file changed, 17 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index 5dddeed11372..edcaf72baf87 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -6677,6 +6677,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
> >         if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES))
> >                 hci_pause_advertising_sync(hdev);
> > 
> > +       hci_dev_lock(hdev);
> > +
> >         params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
> >         if (params) {
> >                 conn->le_conn_min_interval = params->conn_min_interval;
> > @@ -6690,6 +6692,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
> >                 conn->le_supv_timeout = hdev->le_supv_timeout;
> >         }
> > 
> > +       hci_dev_unlock(hdev);
> 
> Couldn't we make the hci_conn_params_lookup behave like
> hci_pend_le_action_lookup and take a RCU read lock and then make
> hci_conn_params_del use synchronize_rcu?

Some of these fields are mutable, so to do conversion to RCU someone
has to first think through data races in field access. In the
hci_pend_le only few fields were accessed (as copied to struct
conn_params in hci_sync.c) and it was in code that is rerun if
something changes, so lockless READ_ONCE/WRITE_ONCE for mutable fields
appeared OK there. Here, we'd maybe need RCU + spinlock for field
access.

It seems to me hdev->lock would be the simplest option here.

> >         /* If controller is scanning, we stop it since some controllers are
> >          * not able to scan and connect at the same time. Also set the
> >          * HCI_LE_SCAN_INTERRUPTED flag so that the command complete
> > @@ -7247,13 +7251,13 @@ static void create_pa_complete(struct hci_dev *hdev, void *data, int err)
> >  }
> > 
> >  static int hci_le_past_params_sync(struct hci_dev *hdev, struct hci_conn *conn,
> > -                                  struct hci_conn *acl, struct bt_iso_qos *qos)
> > +                                  u16 acl_handle, struct bt_iso_qos *qos)
> >  {
> >         struct hci_cp_le_past_params cp;
> >         int err;
> > 
> >         memset(&cp, 0, sizeof(cp));
> > -       cp.handle = cpu_to_le16(acl->handle);
> > +       cp.handle = cpu_to_le16(acl_handle);
> >         /* An HCI_LE_Periodic_Advertising_Sync_Transfer_Received event is sent
> >          * to the Host. HCI_LE_Periodic_Advertising_Report events will be
> >          * enabled with duplicate filtering enabled.
> > @@ -7318,16 +7322,26 @@ static int hci_le_pa_create_sync(struct hci_dev *hdev, void *data)
> >          * 2. Check if that HCI_CONN_FLAG_PAST has been set which indicates that
> >          *    user really intended to use PAST.
> >          */
> > +       hci_dev_lock(hdev);
> > +
> >         le = hci_conn_hash_lookup_le(hdev, &conn->dst, conn->dst_type);
> >         if (le) {
> >                 struct hci_conn_params *params;
> > +               u16 le_handle = le->handle;
> > 
> >                 params = hci_conn_params_lookup(hdev, &le->dst, le->dst_type);
> 
> Aside from the RCU, couldn't we just unlock here so we don't have to
> do it in every branch below?

Yes, we could copy params->flags and then unlock to make it simpler

-> v3

> >                 if (params && params->flags & HCI_CONN_FLAG_PAST) {
> > -                       err = hci_le_past_params_sync(hdev, conn, le, qos);
> > +                       hci_dev_unlock(hdev);
> > +
> > +                       err = hci_le_past_params_sync(hdev, conn, le_handle,
> > +                                                     qos);
> >                         if (!err)
> >                                 goto done;
> > +               } else {
> > +                       hci_dev_unlock(hdev);
> >                 }
> > +       } else {
> > +               hci_dev_unlock(hdev);
> >         }
> > 
> >         /* SID has not been set listen for HCI_EV_LE_EXT_ADV_REPORT to update
> > --
> > 2.55.0
> > 
> 

-- 
Pauli Virtanen