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

[Luiz Augusto von Dentz]

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?

>         /* 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?

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


-- 
Luiz Augusto von Dentz