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

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

[Cen Zhang]

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: Bluetooth: L2CAP: avoid using hci_conn after dropping hold

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 4632 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1090570

---Test result---

Test Summary:
CheckPatch                    PASS      0.76 seconds
GitLint                       PASS      0.35 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      24.95 seconds
CheckAllWarning               PASS      28.20 seconds
CheckSparse                   PASS      26.80 seconds
BuildKernel32                 PASS      24.75 seconds
TestRunnerSetup               PASS      530.45 seconds
TestRunner_l2cap-tester       FAIL      20.24 seconds
IncrementalBuild              PASS      25.67 seconds

Details
##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
Crash detected:
==33==    by 0x13350F: tester_run (tester.c:1085)
==33==    by 0x1142AD: main (l2cap-tester.c:3295)
==33==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==33== 
==33== 
==33== Process terminating with default action of signal 11 (SIGSEGV)
==33==  Access not within mapped region at address 0x0
==33==    at 0x483FF54: __strcmp_sse2 (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==33==    by 0x115C46: read_info_callback (l2cap-tester.c:156)
==33==    by 0x12E860: request_complete (mgmt.c:320)
==33==    by 0x12F2F5: can_read_data (mgmt.c:408)
==33==    by 0x131D68: watch_callback (io-glib.c:173)
==33==    by 0x48A304D: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==33==    by 0x48A33FF: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==33==    by 0x48A36F2: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==33==    by 0x133B18: mainloop_run (mainloop-glib.c:65)
==33==    by 0x133F4F: mainloop_run_with_signal (mainloop-notify.c:196)
==33==    by 0x13350F: tester_run (tester.c:1085)
==33==    by 0x1142AD: main (l2cap-tester.c:3295)
==33==  If you believe this happened as a result of a stack
==33==  overflow in your program's main thread (unlikely but
==33==  possible), you can try to increase the size of the
==33==  main thread stack using the --main-stacksize= flag.
==33==  The main thread stack size used in this run was 8388608.
==33== 
Valgrind errors:
==33==    by 0x115C46: read_info_callback (l2cap-tester.c:156)
==33==    by 0x12E860: request_complete (mgmt.c:320)
==33==    by 0x12F2F5: can_read_data (mgmt.c:408)
==33==    by 0x131D68: watch_callback (io-glib.c:173)
==33==    by 0x48A304D: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==33==    by 0x48A33FF: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==33==    by 0x48A36F2: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==33==    by 0x133B18: mainloop_run (mainloop-glib.c:65)
==33==    by 0x133F4F: mainloop_run_with_signal (mainloop-notify.c:196)
==33==    by 0x13350F: tester_run (tester.c:1085)
==33==    by 0x1142AD: main (l2cap-tester.c:3295)
==33==  If you believe this happened as a result of a stack
==33==  overflow in your program's main thread (unlikely but
==33==  possible), you can try to increase the size of the
==33==  main thread stack using the --main-stacksize= flag.
==33==  The main thread stack size used in this run was 8388608.
==33== 
==33== HEAP SUMMARY:
==33==     in use at exit: 66,009 bytes in 462 blocks
==33==   total heap usage: 592 allocs, 130 frees, 79,004 bytes allocated
==33== 
==33== LEAK SUMMARY:
==33==    definitely lost: 0 bytes in 0 blocks
==33==    indirectly lost: 0 bytes in 0 blocks
==33==      possibly lost: 0 bytes in 0 blocks
==33==    still reachable: 66,009 bytes in 462 blocks
==33==         suppressed: 0 bytes in 0 blocks
==33== Rerun with --leak-check=full to see details of leaked memory
==33== 
==33== For lists of detected and suppressed errors, rerun with: -s
==33== ERROR SUMMARY: 51 errors from 3 contexts (suppressed: 0 from 0)
Crash detected:
==33==         suppressed: 0 bytes in 0 blocks
==33== Rerun with --leak-check=full to see details of leaked memory
==33== 
==33== For lists of detected and suppressed errors, rerun with: -s
==33== ERROR SUMMARY: 51 errors from 3 contexts (suppressed: 0 from 0)
Segmentation fault
Process 32 exited with status 139
reboot: Restarting system
reboot: machine restart
No test result found


https://github.com/bluez/bluetooth-next/pull/149

---
Regards,
Linux Bluetooth

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

[Luiz Augusto von Dentz]

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