Re: [PATCH 3/5] Bluetooth: L2CAP: drain channel timers on connection teardown

[Luiz Augusto von Dentz]

Hi Cen,

On Mon, Sep 21, 2026 at 11:39 AM Cen Zhang <zzzccc427@xxxxxxxxx> wrote:
>
> L2CAP channel timers run on system_percpu_wq. Connection deletion cancels
> them without waiting because callbacks may need conn->lock, but HCI
> unregister does not drain that workqueue after releasing the lock. A
> running callback can therefore outlive HCI-driver unregister. If it drops
> the last channel reference and releases a protocol module, it still has
> to return through Bluetooth code after those dependencies are gone.
>
> Individual channel deletion has another cancellation race. It cancels
> timers before calling the socket teardown callback, which can wait for
> sk lock. A concurrent recvmsg holding that lock can clear local busy and
> rearm the monitor timer. Deletion then unlinks the channel, so connection
> teardown can no longer find that delayed work through the channel list.

How about disabling the work instead of introducing another workqueue for it?

> Give each connection an ordered timer workqueue. Serialize timer queueing
> with channel and connection stop flags, and stop each channel before
> canceling its timers or calling teardown. At connection deletion, stop
> queueing for the connection, cancel all four timer types, then drop
> conn->lock and drain running callbacks before releasing the connection.
> This lets callbacks acquire their locks, observe FLAG_DEL and return
> before HCI unregister completes.
>
> The channel stop flag also ensures that no unlinked channel can leave
> delayed work behind on the new queue. The queue remains allocated until
> the drained connection is freed. This adds one workqueue per connection
> and serializes that connection's channel timers.
>
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> ---
>  include/net/bluetooth/l2cap.h | 23 ++++++++++++++++++++++-
>  net/bluetooth/l2cap_core.c    | 34 ++++++++++++++++++++++++++++++----
>  2 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index efb9b7f422d1..b4af087a0a81 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -611,6 +611,7 @@ struct l2cap_chan {
>
>         void                    *data;
>         const struct l2cap_ops  *ops;
> +       bool                    timers_stopped; /* protected by conn->timer_lock */
>         struct mutex            lock;
>  };
>
> @@ -636,6 +637,10 @@ struct l2cap_conn {
>
>         struct sk_buff_head     pending_rx;
>         struct work_struct      pending_rx_work;
> +       struct workqueue_struct *timer_workqueue;
> +       spinlock_t              timer_lock; /* protects timer scheduling */
> +
> +       bool                    timers_stopped __guarded_by(&timer_lock);
>
>         struct delayed_work     id_addr_timer;
>
> @@ -856,13 +861,29 @@ static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
>  static inline void l2cap_set_timer(struct l2cap_chan *chan,
>                                    struct delayed_work *work, long timeout)
>  {
> +       struct l2cap_conn *conn = chan->conn;
> +       unsigned long flags;
> +       bool pending;
> +
>         BT_DBG("chan %p state %s timeout %ld", chan,
>                state_to_string(chan->state), timeout);
>
> +       if (WARN_ON_ONCE(!conn))
> +               return;
> +
> +       spin_lock_irqsave(&conn->timer_lock, flags);
> +       if (conn->timers_stopped || chan->timers_stopped) {
> +               spin_unlock_irqrestore(&conn->timer_lock, flags);
> +               return;
> +       }
> +
>         l2cap_chan_hold(chan);
>
>         /* put(chan) if timer was already queued so it already has a ref */
> -       if (mod_delayed_work(system_percpu_wq, work, timeout))
> +       pending = mod_delayed_work(conn->timer_workqueue, work, timeout);
> +       spin_unlock_irqrestore(&conn->timer_lock, flags);
> +
> +       if (pending)
>                 l2cap_chan_put(chan);
>  }
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 65e957fdc7ae..0df7bda54473 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -686,9 +686,21 @@ void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>
>  void l2cap_chan_del(struct l2cap_chan *chan, int err)
>  {
> +       struct l2cap_conn *conn = chan->conn;
> +       unsigned long flags;
> +
>         lockdep_assert(!chan->conn || lockdep_is_held(&chan->conn->lock));
>
> +       if (conn) {
> +               spin_lock_irqsave(&conn->timer_lock, flags);
> +               chan->timers_stopped = true;
> +               spin_unlock_irqrestore(&conn->timer_lock, flags);
> +       }
> +
>         __clear_chan_timer(chan);
> +       __clear_retrans_timer(chan);
> +       __clear_monitor_timer(chan);
> +       __clear_ack_timer(chan);
>
>         BT_DBG("chan %p, err %d, state %s", chan, err,
>                state_to_string(chan->state));
> @@ -723,10 +735,6 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
>                 break;
>
>         case L2CAP_MODE_ERTM:
> -               __clear_retrans_timer(chan);
> -               __clear_monitor_timer(chan);
> -               __clear_ack_timer(chan);
> -
>                 skb_queue_purge(&chan->srej_q);
>
>                 l2cap_seq_list_free(&chan->srej_list);
> @@ -1879,6 +1887,7 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>  {
>         struct l2cap_conn *conn = hcon->l2cap_data;
>         struct l2cap_chan *chan, *l;
> +       unsigned long flags;
>
>         if (!conn)
>                 return;
> @@ -1891,6 +1900,9 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>         cancel_work_sync(&conn->pending_rx_work);
>
>         mutex_lock(&conn->lock);
> +       spin_lock_irqsave(&conn->timer_lock, flags);
> +       conn->timers_stopped = true;
> +       spin_unlock_irqrestore(&conn->timer_lock, flags);
>
>         kfree_skb(conn->rx_skb);
>
> @@ -1925,6 +1937,11 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>         spin_unlock(&hcon->proto_lock);
>
>         mutex_unlock(&conn->lock);
> +
> +       /* Channel deletion canceled pending timers. Drop conn->lock before
> +        * waiting for running callbacks so they can acquire it and return.
> +        */
> +       drain_workqueue(conn->timer_workqueue);
>         l2cap_conn_put(conn);
>  }
>
> @@ -1932,6 +1949,7 @@ static void l2cap_conn_free(struct kref *ref)
>  {
>         struct l2cap_conn *conn = container_of(ref, struct l2cap_conn, ref);
>
> +       destroy_workqueue(conn->timer_workqueue);
>         hci_conn_put(conn->hcon);
>         kfree(conn);
>  }
> @@ -7416,6 +7434,14 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
>                 return NULL;
>         }
>
> +       conn->timer_workqueue = alloc_ordered_workqueue("l2cap", WQ_MEM_RECLAIM);
> +       if (!conn->timer_workqueue) {
> +               kfree(conn);
> +               hci_chan_del(hchan);
> +               return NULL;
> +       }
> +       spin_lock_init(&conn->timer_lock);
> +
>         kref_init(&conn->ref);
>         conn->hchan = hchan;
>
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz