Bluetooth: L2CAP: avoid using hci_conn after dropping hold

[PATCH] Bluetooth: L2CAP: avoid using hci_conn after dropping hold

Posted by Cen Zhang 1 month, 1 week ago

l2cap_chan_connect() drops the temporary HCI connection hold after
__l2cap_chan_add() attaches the L2CAP channel and takes its own hold.
The function then checks hcon->state to see whether the channel can be
started immediately because the underlying HCI link is already connected.

Keep that state sample before hci_conn_drop(hcon), and only use the
cached result afterwards.  This avoids dereferencing hcon after the
temporary hold has been released.  Use READ_ONCE() for the sample because
HCI connection state can be advanced concurrently by the command-sync
worker while L2CAP is setting up the channel.

The sampled state is only an optimization for the already-connected case:
a stale non-connected value leaves the L2CAP channel pending for the
normal HCI connect confirmation path.

Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 net/bluetooth/l2cap_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 95c65fece39bd..40e84c1623a9c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7078,6 +7078,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 	struct l2cap_conn *conn;
 	struct hci_conn *hcon;
 	struct hci_dev *hdev;
+	bool link_connected;
 	int err;
 
 	BT_DBG("%pMR -> %pMR (type %u) psm 0x%4.4x mode 0x%2.2x", &chan->src,
@@ -7222,6 +7223,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 	chan->src_type = bdaddr_src_type(hcon);
 
 	__l2cap_chan_add(conn, chan);
+	link_connected = READ_ONCE(hcon->state) == BT_CONNECTED;
 
 	/* l2cap_chan_add takes its own ref so we can drop this one */
 	hci_conn_drop(hcon);
@@ -7236,7 +7238,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 	chan->sport = 0;
 	write_unlock(&chan_list_lock);
 
-	if (hcon->state == BT_CONNECTED) {
+	if (link_connected) {
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
 			__clear_chan_timer(chan);
 			if (l2cap_chan_check_security(chan, true))

Re: [PATCH] Bluetooth: L2CAP: avoid using hci_conn after dropping hold

Posted by Luiz Augusto von Dentz 1 month ago

Hi Cen,

On Wed, May 6, 2026 at 11:53 AM Cen Zhang <zzzccc427@gmail.com> wrote:
>
> l2cap_chan_connect() drops the temporary HCI connection hold after
> __l2cap_chan_add() attaches the L2CAP channel and takes its own hold.
> The function then checks hcon->state to see whether the channel can be
> started immediately because the underlying HCI link is already connected.
>
> Keep that state sample before hci_conn_drop(hcon), and only use the
> cached result afterwards.  This avoids dereferencing hcon after the
> temporary hold has been released.  Use READ_ONCE() for the sample because
> HCI connection state can be advanced concurrently by the command-sync
> worker while L2CAP is setting up the channel.
>
> The sampled state is only an optimization for the already-connected case:
> a stale non-connected value leaves the L2CAP channel pending for the
> normal HCI connect confirmation path.
>
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
>  net/bluetooth/l2cap_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 95c65fece39bd..40e84c1623a9c 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -7078,6 +7078,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
>         struct l2cap_conn *conn;
>         struct hci_conn *hcon;
>         struct hci_dev *hdev;
> +       bool link_connected;
>         int err;
>
>         BT_DBG("%pMR -> %pMR (type %u) psm 0x%4.4x mode 0x%2.2x", &chan->src,
> @@ -7222,6 +7223,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
>         chan->src_type = bdaddr_src_type(hcon);
>
>         __l2cap_chan_add(conn, chan);
> +       link_connected = READ_ONCE(hcon->state) == BT_CONNECTED;
>
>         /* l2cap_chan_add takes its own ref so we can drop this one */
>         hci_conn_drop(hcon);
> @@ -7236,7 +7238,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
>         chan->sport = 0;
>         write_unlock(&chan_list_lock);
>
> -       if (hcon->state == BT_CONNECTED) {
> +       if (link_connected) {
>                 if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
>                         __clear_chan_timer(chan);
>                         if (l2cap_chan_check_security(chan, true))

It doesn't seem to be very useful to read the hcon->state and cache it
while other function still going to access it, anyway we refcount the
hcon and hci_dev_lock is being held accourding to sashiko:

https://sashiko.dev/#/patchset/20260506155313.1412894-1-zzzccc427%40gmail.com

-- 
Luiz Augusto von Dentz