Re: [PATCH v2] Bluetooth: hci_sync: wait for directed advertising completion

[Luiz Augusto von Dentz]

Hi Chengfeng,

On Sat, Aug 1, 2026 at 10:54 AM Chengfeng Ye <nicoyip.dev@xxxxxxxxx> wrote:
>
> le_conn_timeout is embedded in struct hci_conn, but queuing the work does
> not hold a reference to the connection.  hci_conn_del() uses
> cancel_delayed_work() because synchronous cancellation would deadlock when
> le_conn_timeout() itself calls hci_conn_del() while holding hdev->lock.
>
> This leaves the following interleaving possible:
>
>   CPU 0                               CPU 1
>   le_conn_timeout()
>                                       hci_conn_del()
>                                         cancel_delayed_work() = false
>                                         hci_conn_cleanup()
>                                           put_device()
>                                             kfree(conn)
>   hci_conn_failed(conn, ...)
>
> The callback then dereferences the released connection.  KASAN reported:
>
>   BUG: KASAN: slab-use-after-free in hci_conn_failed+0x232/0x250
>   Read of size 8 at addr ffff8881180e8e20 by task kworker/u33:1/111
>   Workqueue: hci0 le_conn_timeout
>   Call Trace:
>    hci_conn_failed+0x232/0x250
>    le_conn_timeout+0x23e/0x2c0
>    process_one_work+0x61b/0xf50
>    worker_thread+0x45b/0xd10
>
> Remove le_conn_timeout instead of adding another connection reference.
> Have the legacy and extended directed-advertising enable commands wait for
> the appropriate LE Connection Complete event in hci_le_create_conn_sync().
> The command-sync entry already holds a connection reference until its
> completion callback returns.
>
> Mark directed advertising as an in-flight connection attempt so teardown
> can cancel the wait.  Disable advertising synchronously when that wait
> fails, and preserve HCI_ERROR_ADVERTISING_TIMEOUT for a software timeout.
> There is then no delayed callback that can race with connection deletion.
>
> Fixes: 980ffc0a2cec ("Bluetooth: Fix LE connection timeout deadlock")
> Cc: stable@xxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/linux-bluetooth/20260730104103.2080325-1-nicoyip.dev@xxxxxxxxx/
> Suggested-by: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Signed-off-by: Chengfeng Ye <nicoyip.dev@xxxxxxxxx>
> ---
> Changes in v2:
> - Remove le_conn_timeout instead of adding references around delayed work.
> - Wait for LE Connection Complete from both directed-advertising enable paths.
> - Make the wait cancellable and disable advertising after a failed wait.
>
> Link: https://lore.kernel.org/linux-bluetooth/20260730104103.2080325-1-nicoyip.dev@xxxxxxxxx/ [v1]
>
>  include/net/bluetooth/hci_core.h |  1 -
>  net/bluetooth/hci_conn.c         | 45 --------------------------------
>  net/bluetooth/hci_event.c        | 25 ++----------------
>  net/bluetooth/hci_sync.c         | 42 ++++++++++++++++++++---------
>  4 files changed, 32 insertions(+), 81 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 3df59849dcbe..937c9ba1e6b7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -761,7 +761,6 @@ struct hci_conn {
>         struct delayed_work disc_work;
>         struct delayed_work auto_accept_work;
>         struct delayed_work idle_work;
> -       struct delayed_work le_conn_timeout;
>
>         struct device   dev;
>         struct dentry   *debugfs;
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b1f911fd4ad6..74bf0686f428 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -687,48 +687,6 @@ static void hci_conn_auto_accept(struct work_struct *work)
>                      &conn->dst);
>  }
>
> -static void le_disable_advertising(struct hci_dev *hdev)
> -{
> -       if (ext_adv_capable(hdev)) {
> -               struct hci_cp_le_set_ext_adv_enable cp;
> -
> -               cp.enable = 0x00;
> -               cp.num_of_sets = 0x00;
> -
> -               hci_send_cmd(hdev, HCI_OP_LE_SET_EXT_ADV_ENABLE, sizeof(cp),
> -                            &cp);
> -       } else {
> -               u8 enable = 0x00;
> -               hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable),
> -                            &enable);
> -       }
> -}
> -
> -static void le_conn_timeout(struct work_struct *work)
> -{
> -       struct hci_conn *conn = container_of(work, struct hci_conn,
> -                                            le_conn_timeout.work);
> -       struct hci_dev *hdev = conn->hdev;
> -
> -       BT_DBG("");
> -
> -       /* We could end up here due to having done directed advertising,
> -        * so clean up the state if necessary. This should however only
> -        * happen with broken hardware or if low duty cycle was used
> -        * (which doesn't have a timeout of its own).
> -        */
> -       if (conn->role == HCI_ROLE_SLAVE) {
> -               /* Disable LE Advertising */
> -               le_disable_advertising(hdev);
> -               hci_dev_lock(hdev);
> -               hci_conn_failed(conn, HCI_ERROR_ADVERTISING_TIMEOUT);
> -               hci_dev_unlock(hdev);
> -               return;
> -       }
> -
> -       hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> -}
> -
>  struct iso_list_data {
>         union {
>                 u8  cig;
> @@ -1121,7 +1079,6 @@ static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type,
>         INIT_DELAYED_WORK(&conn->disc_work, hci_conn_timeout);
>         INIT_DELAYED_WORK(&conn->auto_accept_work, hci_conn_auto_accept);
>         INIT_DELAYED_WORK(&conn->idle_work, hci_conn_idle);
> -       INIT_DELAYED_WORK(&conn->le_conn_timeout, le_conn_timeout);
>
>         spin_lock_init(&conn->proto_lock);
>
> @@ -1269,8 +1226,6 @@ void hci_conn_del(struct hci_conn *conn)
>                         hdev->acl_cnt += conn->sent;
>                 break;
>         case LE_LINK:
> -               cancel_delayed_work(&conn->le_conn_timeout);
> -
>                 if (hdev->le_pkts) {
>                         if (!hci_conn_num(hdev, LE_LINK) ||
>                             hdev->le_cnt + conn->sent > hdev->le_pkts)
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 741d658e9630..b6b279b5a103 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1557,22 +1557,10 @@ static u8 hci_cc_le_set_adv_enable(struct hci_dev *hdev, void *data,
>
>         hci_dev_lock(hdev);
>
> -       /* If we're doing connection initiation as peripheral. Set a
> -        * timeout in case something goes wrong.
> -        */
> -       if (*sent) {
> -               struct hci_conn *conn;
> -
> +       if (*sent)
>                 hci_dev_set_flag(hdev, HCI_LE_ADV);
> -
> -               conn = hci_lookup_le_connect(hdev);
> -               if (conn)
> -                       queue_delayed_work(hdev->workqueue,
> -                                          &conn->le_conn_timeout,
> -                                          conn->conn_timeout);
> -       } else {
> +       else
>                 hci_dev_clear_flag(hdev, HCI_LE_ADV);
> -       }
>
>         hci_dev_unlock(hdev);
>
> @@ -1604,8 +1592,6 @@ static u8 hci_cc_le_set_ext_adv_enable(struct hci_dev *hdev, void *data,
>                 adv = hci_find_adv_instance(hdev, set->handle);
>
>         if (cp->enable) {
> -               struct hci_conn *conn;
> -
>                 hci_dev_set_flag(hdev, HCI_LE_ADV);
>
>                 if (adv)
> @@ -1613,11 +1599,6 @@ static u8 hci_cc_le_set_ext_adv_enable(struct hci_dev *hdev, void *data,
>                 else if (!set->handle)
>                         hci_dev_set_flag(hdev, HCI_LE_ADV_0);
>
> -               conn = hci_lookup_le_connect(hdev);
> -               if (conn)
> -                       queue_delayed_work(hdev->workqueue,
> -                                          &conn->le_conn_timeout,
> -                                          conn->conn_timeout);
>         } else {
>                 if (cp->num_of_sets) {
>                         if (adv)
> @@ -5770,8 +5751,6 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
>                                                           &conn->init_addr_type);
>                         }
>                 }
> -       } else {
> -               cancel_delayed_work(&conn->le_conn_timeout);
>         }
>
>         /* The HCI_LE_Connection_Complete event is only sent once per connection.
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index c8d14128c363..714bb1d55926 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1616,7 +1616,9 @@ int hci_update_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance)
>         return __hci_set_scan_rsp_data_sync(hdev, instance);
>  }
>
> -int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance)
> +static int hci_enable_ext_advertising_sync_ev(struct hci_dev *hdev,
> +                                             u8 instance, u8 event,
> +                                             u32 timeout)
>  {
>         struct hci_cp_le_set_ext_adv_enable *cp;
>         struct hci_cp_ext_adv_set *set;
> @@ -1656,10 +1658,16 @@ int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance)
>                 set->duration = cpu_to_le16(duration / 10);
>         }
>
> -       return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_ENABLE,
> -                                    sizeof(*cp) +
> -                                    sizeof(*set) * cp->num_of_sets,
> -                                    data, HCI_CMD_TIMEOUT);
> +       return __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_SET_EXT_ADV_ENABLE,
> +                                       sizeof(*cp) +
> +                                       sizeof(*set) * cp->num_of_sets,
> +                                       data, event, timeout, NULL);
> +}
> +
> +int hci_enable_ext_advertising_sync(struct hci_dev *hdev, u8 instance)
> +{
> +       return hci_enable_ext_advertising_sync_ev(hdev, instance, 0,
> +                                                 HCI_CMD_TIMEOUT);
>  }
>
>  int hci_start_ext_adv_sync(struct hci_dev *hdev, u8 instance)
> @@ -6605,7 +6613,11 @@ static int hci_le_ext_directed_advertising_sync(struct hci_dev *hdev,
>                         return err;
>         }
>
> -       return hci_enable_ext_advertising_sync(hdev, 0x00);
> +       return hci_enable_ext_advertising_sync_ev(hdev, 0x00,
> +                               use_enhanced_conn_complete(hdev) ?
> +                               HCI_EV_LE_ENHANCED_CONN_COMPLETE :
> +                               HCI_EV_LE_CONN_COMPLETE,
> +                               conn->conn_timeout);
>  }
>
>  static int hci_le_directed_advertising_sync(struct hci_dev *hdev,
> @@ -6656,8 +6668,12 @@ static int hci_le_directed_advertising_sync(struct hci_dev *hdev,
>
>         enable = 0x01;
>
> -       return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> -                                    sizeof(enable), &enable, HCI_CMD_TIMEOUT);
> +       return __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> +                                       sizeof(enable), &enable,
> +                                       use_enhanced_conn_complete(hdev) ?
> +                                       HCI_EV_LE_ENHANCED_CONN_COMPLETE :
> +                                       HCI_EV_LE_CONN_COMPLETE,
> +                                       conn->conn_timeout, NULL);
>  }
>
>  static void set_ext_conn_params(struct hci_conn *conn,
> @@ -6761,6 +6777,7 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>                 /* Pause advertising while doing directed advertising. */
>                 hci_pause_advertising_sync(hdev);
>
> +               set_bit(HCI_CONN_CREATE, &conn->flags);
>                 err = hci_le_directed_advertising_sync(hdev, conn);
>                 goto done;
>         }
> @@ -6847,7 +6864,9 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
>  done:
>         clear_bit(HCI_CONN_CREATE, &conn->flags);
>
> -       if (err == -ETIMEDOUT)
> +       if (err && conn->role == HCI_ROLE_SLAVE)
> +               hci_disable_advertising_sync(hdev);
> +       else if (err == -ETIMEDOUT)
>                 hci_le_connect_cancel_sync(hdev, conn, 0x00);
>
>         /* Re-enable advertising after the connection attempt is finished. */
> @@ -7184,9 +7203,8 @@ static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
>         if (conn != hci_lookup_le_connect(hdev))
>                 goto unlock;
>
> -       /* Flush to make sure we send create conn cancel command if needed */
> -       flush_delayed_work(&conn->le_conn_timeout);
> -       hci_conn_failed(conn, bt_status(err));
> +       hci_conn_failed(conn, conn->role == HCI_ROLE_SLAVE && err == -ETIMEDOUT ?
> +                        HCI_ERROR_ADVERTISING_TIMEOUT : bt_status(err));
>
>  unlock:
>         hci_dev_unlock(hdev);
> --
> 2.43.0

Looks like there is a problem when the command generates a command
complete it bypass the custom event matching, so we will need to fix
that first:

https://sashiko.dev/#/patchset/20260801145430.3560911-1-nicoyip.dev%40gmail.com

-- 
Luiz Augusto von Dentz