* [PATCH v4] Bluetooth: serialize accept_q access
@ 2026-05-06 11:43 Ren Wei
2026-05-06 13:56 ` [v4] " bluez.test.bot
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Ren Wei @ 2026-05-06 11:43 UTC (permalink / raw)
To: linux-bluetooth, netdev
Cc: marcel, luiz.dentz, davem, edumazet, kuba, pabeni, horms, jannh,
yuantan098, yifanwucs, tomapufckgml, bird, wangjiexun2025, n05ec
From: Jiexun Wang <wangjiexun2025@gmail.com>
bt_sock_poll() walks the accept queue without synchronization, while
child teardown can unlink the same socket and drop its last reference.
The unsynchronized accept queue walk has existed since the initial
Bluetooth import.
Protect accept_q with a dedicated lock for queue updates and polling.
Also rework bt_accept_dequeue() to take temporary child references under
the queue lock before dropping it and locking the child socket.
Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: Jann Horn <jannh@google.com>
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
Changes in v4:
- no functional changes
- clarify that the race dates back to the initial Bluetooth import
- update trailers
I noticed Jann also proposed a fix at
https://patchwork.kernel.org/project/bluetooth/patch/20260504-bluetooth-accept-uaf-fix-v1-1-1ca63c0efadd@google.com/,
so we're adding his Reported-by tag here. Please let us know if this
isn't appropriate.
- v3 Link: https://lore.kernel.org/all/20260404162324.2789862-1-n05ec@lzu.edu.cn/
Changes in v3:
- move sk_acceptq_added()/sk_acceptq_removed() inside accept_q_lock
critical sections to serialize sk_ack_backlog updates with accept_q
operations
- v2 Link: https://lore.kernel.org/all/06a6b4549acba207847ce532dedbf1c95ab22d13.1774925231.git.wangjiexun2025@gmail.com/
Changes in v2:
- add Tested-by: Ren Wei <enjou1224z@gmail.com>
- resend to the public Bluetooth/netdev lists
include/net/bluetooth/bluetooth.h | 1 +
net/bluetooth/af_bluetooth.c | 87 +++++++++++++++++++++++--------
2 files changed, 66 insertions(+), 22 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 69eed69f7f26..3faea66b1979 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -398,6 +398,7 @@ void baswap(bdaddr_t *dst, const bdaddr_t *src);
struct bt_sock {
struct sock sk;
struct list_head accept_q;
+ spinlock_t accept_q_lock; /* protects accept_q */
struct sock *parent;
unsigned long flags;
void (*skb_msg_name)(struct sk_buff *, void *, int *);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 2b94e2077203..fa14b9a915eb 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -154,6 +154,7 @@ struct sock *bt_sock_alloc(struct net *net, struct socket *sock,
sock_init_data(sock, sk);
INIT_LIST_HEAD(&bt_sk(sk)->accept_q);
+ spin_lock_init(&bt_sk(sk)->accept_q_lock);
sock_reset_flag(sk, SOCK_ZAPPED);
@@ -214,6 +215,7 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
{
const struct cred *old_cred;
struct pid *old_pid;
+ struct bt_sock *par = bt_sk(parent);
BT_DBG("parent %p, sk %p", parent, sk);
@@ -224,9 +226,13 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
else
lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
- list_add_tail(&bt_sk(sk)->accept_q, &bt_sk(parent)->accept_q);
bt_sk(sk)->parent = parent;
+ spin_lock_bh(&par->accept_q_lock);
+ list_add_tail(&bt_sk(sk)->accept_q, &par->accept_q);
+ sk_acceptq_added(parent);
+ spin_unlock_bh(&par->accept_q_lock);
+
/* Copy credentials from parent since for incoming connections the
* socket is allocated by the kernel.
*/
@@ -244,8 +250,6 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
bh_unlock_sock(sk);
else
release_sock(sk);
-
- sk_acceptq_added(parent);
}
EXPORT_SYMBOL(bt_accept_enqueue);
@@ -254,45 +258,72 @@ EXPORT_SYMBOL(bt_accept_enqueue);
*/
void bt_accept_unlink(struct sock *sk)
{
+ struct sock *parent = bt_sk(sk)->parent;
+
BT_DBG("sk %p state %d", sk, sk->sk_state);
+ spin_lock_bh(&bt_sk(parent)->accept_q_lock);
list_del_init(&bt_sk(sk)->accept_q);
- sk_acceptq_removed(bt_sk(sk)->parent);
+ sk_acceptq_removed(parent);
+ spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
bt_sk(sk)->parent = NULL;
sock_put(sk);
}
EXPORT_SYMBOL(bt_accept_unlink);
+static struct sock *bt_accept_get(struct sock *parent, struct sock *sk)
+{
+ struct bt_sock *bt = bt_sk(parent);
+ struct sock *next = NULL;
+
+ /* accept_q is modified from child teardown paths too, so take a
+ * temporary reference before dropping the queue lock.
+ */
+ spin_lock_bh(&bt->accept_q_lock);
+
+ if (sk) {
+ if (bt_sk(sk)->parent != parent)
+ goto out;
+
+ if (!list_is_last(&bt_sk(sk)->accept_q, &bt->accept_q)) {
+ next = &list_next_entry(bt_sk(sk), accept_q)->sk;
+ sock_hold(next);
+ }
+ } else if (!list_empty(&bt->accept_q)) {
+ next = &list_first_entry(&bt->accept_q,
+ struct bt_sock, accept_q)->sk;
+ sock_hold(next);
+ }
+
+out:
+ spin_unlock_bh(&bt->accept_q_lock);
+ return next;
+}
+
struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
{
- struct bt_sock *s, *n;
- struct sock *sk;
+ struct sock *sk, *next;
BT_DBG("parent %p", parent);
restart:
- list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
- sk = (struct sock *)s;
-
+ for (sk = bt_accept_get(parent, NULL); sk; sk = next) {
/* Prevent early freeing of sk due to unlink and sock_kill */
- sock_hold(sk);
lock_sock(sk);
/* Check sk has not already been unlinked via
* bt_accept_unlink() due to serialisation caused by sk locking
*/
- if (!bt_sk(sk)->parent) {
+ if (bt_sk(sk)->parent != parent) {
BT_DBG("sk %p, already unlinked", sk);
release_sock(sk);
sock_put(sk);
- /* Restart the loop as sk is no longer in the list
- * and also avoid a potential infinite loop because
- * list_for_each_entry_safe() is not thread safe.
- */
goto restart;
}
+ next = bt_accept_get(parent, sk);
+
/* sk is safely in the parent list so reduce reference count */
sock_put(sk);
@@ -310,6 +341,8 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
sock_graft(sk, newsock);
release_sock(sk);
+ if (next)
+ sock_put(next);
return sk;
}
@@ -518,18 +551,28 @@ EXPORT_SYMBOL(bt_sock_stream_recvmsg);
static inline __poll_t bt_accept_poll(struct sock *parent)
{
- struct bt_sock *s, *n;
+ struct bt_sock *bt = bt_sk(parent);
+ struct bt_sock *s;
struct sock *sk;
+ __poll_t mask = 0;
+
+ spin_lock_bh(&bt->accept_q_lock);
+ list_for_each_entry(s, &bt->accept_q, accept_q) {
+ int state;
- list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
sk = (struct sock *)s;
- if (sk->sk_state == BT_CONNECTED ||
- (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags) &&
- sk->sk_state == BT_CONNECT2))
- return EPOLLIN | EPOLLRDNORM;
+ state = READ_ONCE(sk->sk_state);
+
+ if (state == BT_CONNECTED ||
+ (test_bit(BT_SK_DEFER_SETUP, &bt->flags) &&
+ state == BT_CONNECT2)) {
+ mask = EPOLLIN | EPOLLRDNORM;
+ break;
+ }
}
+ spin_unlock_bh(&bt->accept_q_lock);
- return 0;
+ return mask;
}
__poll_t bt_sock_poll(struct file *file, struct socket *sock,
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [v4] Bluetooth: serialize accept_q access
2026-05-06 11:43 [PATCH v4] Bluetooth: serialize accept_q access Ren Wei
@ 2026-05-06 13:56 ` bluez.test.bot
2026-05-06 17:04 ` [PATCH v4] " Jann Horn
2026-05-08 19:00 ` patchwork-bot+bluetooth
2 siblings, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2026-05-06 13:56 UTC (permalink / raw)
To: linux-bluetooth, n05ec
[-- Attachment #1: Type: text/plain, Size: 15960 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=1090423
---Test result---
Test Summary:
CheckPatch FAIL 1.02 seconds
GitLint FAIL 0.36 seconds
SubjectPrefix PASS 2.06 seconds
BuildKernel PASS 27.34 seconds
CheckAllWarning PASS 30.36 seconds
CheckSparse PASS 30.05 seconds
BuildKernel32 PASS 26.21 seconds
TestRunnerSetup PASS 579.19 seconds
TestRunner_l2cap-tester FAIL 19.18 seconds
TestRunner_iso-tester PASS 311.73 seconds
TestRunner_bnep-tester FAIL 18.56 seconds
TestRunner_mgmt-tester FAIL 23.55 seconds
TestRunner_rfcomm-tester PASS 41.44 seconds
TestRunner_sco-tester PASS 80.71 seconds
TestRunner_ioctl-tester FAIL 50.29 seconds
TestRunner_mesh-tester PASS 39.76 seconds
TestRunner_smp-tester PASS 18.82 seconds
TestRunner_userchan-tester PASS 20.32 seconds
TestRunner_6lowpan-tester PASS 35.31 seconds
IncrementalBuild PASS 25.46 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v4] Bluetooth: serialize accept_q access
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#91:
Reported-by: Jann Horn <jannh@google.com>
Reported-by: Yuan Tan <yuantan098@gmail.com>
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#92:
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#93:
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#94:
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#95:
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
total: 0 errors, 5 warnings, 0 checks, 170 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/patch/14557166.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v4] Bluetooth: serialize accept_q access
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
28: B1 Line exceeds max length (119>80): " https://patchwork.kernel.org/project/bluetooth/patch/20260504-bluetooth-accept-uaf-fix-v1-1-1ca63c0efadd@google.com/,"
31: B1 Line exceeds max length (81>80): "- v3 Link: https://lore.kernel.org/all/20260404162324.2789862-1-n05ec@lzu.edu.cn/"
37: B1 Line exceeds max length (120>80): "- v2 Link: https://lore.kernel.org/all/06a6b4549acba207847ce532dedbf1c95ab22d13.1774925231.git.wangjiexun2025@gmail.com/"
##############################
Test: TestRunner_l2cap-tester - FAIL
Desc: Run l2cap-tester with test-runner
Output:
Crash detected:
==34== by 0x13325F: tester_run (tester.c:1085)
==34== by 0x1142AD: main (l2cap-tester.c:3295)
==34== Address 0x50 is not stack'd, malloc'd or (recently) free'd
==34==
==34==
==34== Process terminating with default action of signal 11 (SIGSEGV)
==34== Access not within mapped region at address 0x50
==34== at 0x12BE24: bthost_set_cmd_complete_cb (bthost.c:3487)
==34== by 0x11596D: setup_powered_client_callback (l2cap-tester.c:1317)
==34== by 0x12E5B0: request_complete (mgmt.c:320)
==34== by 0x12F045: can_read_data (mgmt.c:408)
==34== by 0x131AB8: watch_callback (io-glib.c:173)
==34== by 0x48A304D: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x48A33FF: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x48A36F2: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x133868: mainloop_run (mainloop-glib.c:65)
==34== by 0x133C9F: mainloop_run_with_signal (mainloop-notify.c:196)
==34== by 0x13325F: tester_run (tester.c:1085)
==34== by 0x1142AD: main (l2cap-tester.c:3295)
==34== If you believe this happened as a result of a stack
==34== overflow in your program's main thread (unlikely but
==34== possible), you can try to increase the size of the
==34== main thread stack using the --main-stacksize= flag.
==34== The main thread stack size used in this run was 8388608.
==34==
Valgrind errors:
==34== by 0x11596D: setup_powered_client_callback (l2cap-tester.c:1317)
==34== by 0x12E5B0: request_complete (mgmt.c:320)
==34== by 0x12F045: can_read_data (mgmt.c:408)
==34== by 0x131AB8: watch_callback (io-glib.c:173)
==34== by 0x48A304D: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x48A33FF: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x48A36F2: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x133868: mainloop_run (mainloop-glib.c:65)
==34== by 0x133C9F: mainloop_run_with_signal (mainloop-notify.c:196)
==34== by 0x13325F: tester_run (tester.c:1085)
==34== by 0x1142AD: main (l2cap-tester.c:3295)
==34== If you believe this happened as a result of a stack
==34== overflow in your program's main thread (unlikely but
==34== possible), you can try to increase the size of the
==34== main thread stack using the --main-stacksize= flag.
==34== The main thread stack size used in this run was 8388608.
==34==
==34== HEAP SUMMARY:
==34== in use at exit: 66,134 bytes in 464 blocks
==34== total heap usage: 632 allocs, 168 frees, 80,436 bytes allocated
==34==
==34== LEAK SUMMARY:
==34== definitely lost: 0 bytes in 0 blocks
==34== indirectly lost: 0 bytes in 0 blocks
==34== possibly lost: 0 bytes in 0 blocks
==34== still reachable: 66,134 bytes in 464 blocks
==34== suppressed: 0 bytes in 0 blocks
==34== Rerun with --leak-check=full to see details of leaked memory
==34==
==34== For lists of detected and suppressed errors, rerun with: -s
==34== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
Crash detected:
==34== suppressed: 0 bytes in 0 blocks
==34== Rerun with --leak-check=full to see details of leaked memory
==34==
==34== For lists of detected and suppressed errors, rerun with: -s
==34== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
Segmentation fault
Process 33 exited with status 139
reboot: Restarting system
reboot: machine restart
No test result found
##############################
Test: TestRunner_bnep-tester - FAIL
Desc: Run bnep-tester with test-runner
Output:
Crash detected:
==33== by 0x12C99F: tester_run (tester.c:1085)
==33== by 0x111CB3: main (bnep-tester.c:298)
==33== Address 0x50 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 0x50
==33== at 0x125C44: bthost_set_cmd_complete_cb (bthost.c:3487)
==33== by 0x111F42: setup_powered_client_callback (bnep-tester.c:244)
==33== by 0x127F00: request_complete (mgmt.c:320)
==33== by 0x1288B5: can_read_data (mgmt.c:408)
==33== by 0x12B328: 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 0x12CFA8: mainloop_run (mainloop-glib.c:65)
==33== by 0x12D3DF: mainloop_run_with_signal (mainloop-notify.c:196)
==33== by 0x12C99F: tester_run (tester.c:1085)
==33== by 0x111CB3: main (bnep-tester.c:298)
==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 0x111F42: setup_powered_client_callback (bnep-tester.c:244)
==33== by 0x127F00: request_complete (mgmt.c:320)
==33== by 0x1288B5: can_read_data (mgmt.c:408)
==33== by 0x12B328: 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 0x12CFA8: mainloop_run (mainloop-glib.c:65)
==33== by 0x12D3DF: mainloop_run_with_signal (mainloop-notify.c:196)
==33== by 0x12C99F: tester_run (tester.c:1085)
==33== by 0x111CB3: main (bnep-tester.c:298)
==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: 25,333 bytes in 82 blocks
==33== total heap usage: 237 allocs, 155 frees, 39,167 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: 25,333 bytes in 82 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: 2 errors from 2 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: 2 errors from 2 contexts (suppressed: 0 from 0)
Segmentation fault
Process 32 exited with status 139
reboot: Restarting system
reboot: machine restart
No test result found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Crash detected:
==34== by 0x14F1DF: tester_run (tester.c:1085)
==34== by 0x12B999: main (mgmt-tester.c:15134)
==34== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==34==
==34==
==34== Process terminating with default action of signal 11 (SIGSEGV)
==34== Access not within mapped region at address 0x0
==34== at 0x145134: bthost_notify_ready (bthost.c:1190)
==34== by 0x12FA10: read_info_callback (mgmt-tester.c:223)
==34== by 0x14A420: request_complete (mgmt.c:320)
==34== by 0x14AFD5: can_read_data (mgmt.c:408)
==34== by 0x14DA48: watch_callback (io-glib.c:173)
==34== by 0x48A304D: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x48A33FF: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x48A36F2: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x14F7E8: mainloop_run (mainloop-glib.c:65)
==34== by 0x14FC1F: mainloop_run_with_signal (mainloop-notify.c:196)
==34== by 0x14F1DF: tester_run (tester.c:1085)
==34== by 0x12B999: main (mgmt-tester.c:15134)
==34== If you believe this happened as a result of a stack
==34== overflow in your program's main thread (unlikely but
==34== possible), you can try to increase the size of the
==34== main thread stack using the --main-stacksize= flag.
==34== The main thread stack size used in this run was 8388608.
==34==
Valgrind errors:
==34== by 0x12FA10: read_info_callback (mgmt-tester.c:223)
==34== by 0x14A420: request_complete (mgmt.c:320)
==34== by 0x14AFD5: can_read_data (mgmt.c:408)
==34== by 0x14DA48: watch_callback (io-glib.c:173)
==34== by 0x48A304D: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x48A33FF: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x48A36F2: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==34== by 0x14F7E8: mainloop_run (mainloop-glib.c:65)
==34== by 0x14FC1F: mainloop_run_with_signal (mainloop-notify.c:196)
==34== by 0x14F1DF: tester_run (tester.c:1085)
==34== by 0x12B999: main (mgmt-tester.c:15134)
==34== If you believe this happened as a result of a stack
==34== overflow in your program's main thread (unlikely but
==34== possible), you can try to increase the size of the
==34== main thread stack using the --main-stacksize= flag.
==34== The main thread stack size used in this run was 8388608.
==34==
==34== HEAP SUMMARY:
==34== in use at exit: 182,331 bytes in 2,112 blocks
==34== total heap usage: 2,379 allocs, 267 frees, 204,063 bytes allocated
==34==
==34== LEAK SUMMARY:
==34== definitely lost: 0 bytes in 0 blocks
==34== indirectly lost: 0 bytes in 0 blocks
==34== possibly lost: 0 bytes in 0 blocks
==34== still reachable: 182,331 bytes in 2,112 blocks
==34== suppressed: 0 bytes in 0 blocks
==34== Rerun with --leak-check=full to see details of leaked memory
==34==
==34== For lists of detected and suppressed errors, rerun with: -s
==34== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
Crash detected:
==34== suppressed: 0 bytes in 0 blocks
==34== Rerun with --leak-check=full to see details of leaked memory
==34==
==34== For lists of detected and suppressed errors, rerun with: -s
==34== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
Segmentation fault
Process 33 exited with status 139
reboot: Restarting system
reboot: machine restart
No test result found
##############################
Test: TestRunner_ioctl-tester - FAIL
Desc: Run ioctl-tester with test-runner
Output:
Total: 28, Passed: 0 (0.0%), Failed: 11, Not Run: 17
Failed Test Cases
Device List Timed out -31.835 seconds
Device Info Timed out -6.933 seconds
Reset Stat Timed out -6.940 seconds
Set Link Mode - ACCEPT Timed out -6.946 seconds
Set Pkt Type - DM Timed out -15.222 seconds
Set Pkt Type - DH Timed out -15.230 seconds
Set Pkt Type - HV Timed out -15.236 seconds
Set Pkt Type - 2-DH Timed out -15.245 seconds
Set Pkt Type - 2-DH Timed out -15.253 seconds
Set Pkt Type - ALL Timed out -15.260 seconds
Set ACL MTU - 1 Timed out -15.267 seconds
https://github.com/bluez/bluetooth-next/pull/148
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] Bluetooth: serialize accept_q access
2026-05-06 11:43 [PATCH v4] Bluetooth: serialize accept_q access Ren Wei
2026-05-06 13:56 ` [v4] " bluez.test.bot
@ 2026-05-06 17:04 ` Jann Horn
2026-05-08 19:00 ` patchwork-bot+bluetooth
2 siblings, 0 replies; 4+ messages in thread
From: Jann Horn @ 2026-05-06 17:04 UTC (permalink / raw)
To: Ren Wei, luiz.dentz
Cc: linux-bluetooth, netdev, marcel, davem, edumazet, kuba, pabeni,
horms, yuantan098, yifanwucs, tomapufckgml, bird, wangjiexun2025
[-- Attachment #1: Type: text/plain, Size: 10085 bytes --]
On Wed, May 6, 2026 at 1:43 PM Ren Wei <n05ec@lzu.edu.cn> wrote:
> bt_sock_poll() walks the accept queue without synchronization, while
> child teardown can unlink the same socket and drop its last reference.
> The unsynchronized accept queue walk has existed since the initial
> Bluetooth import.
>
> Protect accept_q with a dedicated lock for queue updates and polling.
> Also rework bt_accept_dequeue() to take temporary child references under
> the queue lock before dropping it and locking the child socket.
>
> Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Reported-by: Jann Horn <jannh@google.com>
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
The patch looks good to me. I have some comments below, but they're
not important - from my perspective, this patch is ready to land in
the tree.
Reviewed-by: Jann Horn <jannh@google.com>
> ---
> Changes in v4:
> - no functional changes
> - clarify that the race dates back to the initial Bluetooth import
> - update trailers
> I noticed Jann also proposed a fix at
> https://patchwork.kernel.org/project/bluetooth/patch/20260504-bluetooth-accept-uaf-fix-v1-1-1ca63c0efadd@google.com/,
> so we're adding his Reported-by tag here. Please let us know if this
> isn't appropriate.
Thanks for letting me know that my patch was redundant, and for
listing me in Reported-by.
This addresses the race I described.
(You could add the line
"Closes: https://lore.kernel.org/r/20260504-bluetooth-accept-uaf-fix-v1-1-1ca63c0efadd@google.com"
after the "Reported-by: Jann Horn <jannh@google.com>".)
[...]
> @@ -254,45 +258,72 @@ EXPORT_SYMBOL(bt_accept_enqueue);
> */
> void bt_accept_unlink(struct sock *sk)
> {
> + struct sock *parent = bt_sk(sk)->parent;
> +
> BT_DBG("sk %p state %d", sk, sk->sk_state);
>
> + spin_lock_bh(&bt_sk(parent)->accept_q_lock);
> list_del_init(&bt_sk(sk)->accept_q);
> - sk_acceptq_removed(bt_sk(sk)->parent);
> + sk_acceptq_removed(parent);
> + spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
> bt_sk(sk)->parent = NULL;
> sock_put(sk);
> }
> EXPORT_SYMBOL(bt_accept_unlink);
>
> +static struct sock *bt_accept_get(struct sock *parent, struct sock *sk)
> +{
> + struct bt_sock *bt = bt_sk(parent);
> + struct sock *next = NULL;
> +
> + /* accept_q is modified from child teardown paths too, so take a
> + * temporary reference before dropping the queue lock.
> + */
> + spin_lock_bh(&bt->accept_q_lock);
> +
> + if (sk) {
> + if (bt_sk(sk)->parent != parent)
> + goto out;
This check seems redundant? The caller already bailed out if
"bt_sk(sk)->parent != parent", and lock_sock(sk) ensures that
bt_sk(sk)->parent can't change concurrently because bt_accept_unlink()
is also protected by lock_sock() or lock_sock_nested(), as the comment
above bt_accept_unlink() documents.
> +
> + if (!list_is_last(&bt_sk(sk)->accept_q, &bt->accept_q)) {
> + next = &list_next_entry(bt_sk(sk), accept_q)->sk;
> + sock_hold(next);
> + }
> + } else if (!list_empty(&bt->accept_q)) {
> + next = &list_first_entry(&bt->accept_q,
> + struct bt_sock, accept_q)->sk;
> + sock_hold(next);
> + }
> +
> +out:
> + spin_unlock_bh(&bt->accept_q_lock);
> + return next;
> +}
Hmm. This looks a bit complicated to me, and I find it hard to reason
about how accept_q walks are restarted after temporarily dropping the
lock; I think it would be nice if you could instead walk the
->accept_q while holding the accept_q_lock until you identify a socket
with the right ->sk_state, then drop the accept_q_lock and lock the
sock. Something like this diff on top of your patch (completely
untested); I have attached a properly formatted version of this diff
that you can apply with "git apply":
```
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 9d68dd86023c..26e7c7198522 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -271,50 +271,36 @@ void bt_accept_unlink(struct sock *sk)
}
EXPORT_SYMBOL(bt_accept_unlink);
-static struct sock *bt_accept_get(struct sock *parent, struct sock *sk)
-{
- struct bt_sock *bt = bt_sk(parent);
- struct sock *next = NULL;
-
- /* accept_q is modified from child teardown paths too, so take a
- * temporary reference before dropping the queue lock.
- */
- spin_lock_bh(&bt->accept_q_lock);
-
- if (sk) {
- if (bt_sk(sk)->parent != parent)
- goto out;
-
- if (!list_is_last(&bt_sk(sk)->accept_q, &bt->accept_q)) {
- next = &list_next_entry(bt_sk(sk), accept_q)->sk;
- sock_hold(next);
- }
- } else if (!list_empty(&bt->accept_q)) {
- next = &list_first_entry(&bt->accept_q,
- struct bt_sock, accept_q)->sk;
- sock_hold(next);
- }
-
-out:
- spin_unlock_bh(&bt->accept_q_lock);
- return next;
-}
-
struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
{
- struct sock *sk, *next;
+ struct bt_sock *s;
+ struct sock *sk;
BT_DBG("parent %p", parent);
restart:
- for (sk = bt_accept_get(parent, NULL); sk; sk = next) {
+ spin_lock_bh(&bt_sk(parent)->accept_q_lock);
+ list_for_each_entry(s, &bt_sk(parent)->accept_q, accept_q) {
+ unsigned char state;
+
+ sk = &s->sk;
+
+ /* lockless version of the checks below */
+ state = data_race(READ_ONCE(sk->sk_state));
+ if (state != BT_CLOSED && state != BT_CONNECTED && newsock &&
+ !test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags))
+ continue;
+
/* Prevent early freeing of sk due to unlink and sock_kill */
+ sock_hold(sk);
+ spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
lock_sock(sk);
+ /* socket is now locked, redo checks reliably */
/* Check sk has not already been unlinked via
* bt_accept_unlink() due to serialisation caused by sk locking
*/
- if (bt_sk(sk)->parent != parent) {
+ if (s->parent != parent) {
BT_DBG("sk %p, already unlinked", sk);
release_sock(sk);
sock_put(sk);
@@ -322,8 +308,6 @@ struct sock *bt_accept_dequeue(struct sock
*parent, struct socket *newsock)
goto restart;
}
- next = bt_accept_get(parent, sk);
-
/* sk is safely in the parent list so reduce reference count */
sock_put(sk);
@@ -331,7 +315,7 @@ struct sock *bt_accept_dequeue(struct sock
*parent, struct socket *newsock)
if (sk->sk_state == BT_CLOSED) {
bt_accept_unlink(sk);
release_sock(sk);
- continue;
+ goto restart;
}
if (sk->sk_state == BT_CONNECTED || !newsock ||
@@ -341,12 +325,11 @@ struct sock *bt_accept_dequeue(struct sock
*parent, struct socket *newsock)
sock_graft(sk, newsock);
release_sock(sk);
- if (next)
- sock_put(next);
return sk;
}
release_sock(sk);
+ goto restart;
}
return NULL;
```
I think this makes the code simpler, and it reduces the line count;
however, I think your approach is okay too, so it would also be fine
to keep your approach if you prefer.
[...]
> @@ -518,18 +551,28 @@ EXPORT_SYMBOL(bt_sock_stream_recvmsg);
>
> static inline __poll_t bt_accept_poll(struct sock *parent)
> {
> - struct bt_sock *s, *n;
> + struct bt_sock *bt = bt_sk(parent);
> + struct bt_sock *s;
> struct sock *sk;
> + __poll_t mask = 0;
> +
> + spin_lock_bh(&bt->accept_q_lock);
> + list_for_each_entry(s, &bt->accept_q, accept_q) {
> + int state;
>
> - list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
> sk = (struct sock *)s;
> - if (sk->sk_state == BT_CONNECTED ||
> - (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags) &&
> - sk->sk_state == BT_CONNECT2))
> - return EPOLLIN | EPOLLRDNORM;
> + state = READ_ONCE(sk->sk_state);
nitpick: This READ_ONCE() is not synchronized with a corresponding
WRITE_ONCE(); that's not really clean, and it might be appropriate to
mark this with data_race() if this is intentionally racy with
potentially-torn stores. But that's a minor detail.
> +
> + if (state == BT_CONNECTED ||
> + (test_bit(BT_SK_DEFER_SETUP, &bt->flags) &&
> + state == BT_CONNECT2)) {
> + mask = EPOLLIN | EPOLLRDNORM;
> + break;
> + }
> }
> + spin_unlock_bh(&bt->accept_q_lock);
>
> - return 0;
> + return mask;
> }
>
> __poll_t bt_sock_poll(struct file *file, struct socket *sock,
> --
> 2.34.1
>
[-- Attachment #2: locked-walk.diff --]
[-- Type: application/x-patch, Size: 2914 bytes --]
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v4] Bluetooth: serialize accept_q access
2026-05-06 11:43 [PATCH v4] Bluetooth: serialize accept_q access Ren Wei
2026-05-06 13:56 ` [v4] " bluez.test.bot
2026-05-06 17:04 ` [PATCH v4] " Jann Horn
@ 2026-05-08 19:00 ` patchwork-bot+bluetooth
2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+bluetooth @ 2026-05-08 19:00 UTC (permalink / raw)
To: Ren Wei
Cc: linux-bluetooth, netdev, marcel, luiz.dentz, davem, edumazet,
kuba, pabeni, horms, jannh, yuantan098, yifanwucs, tomapufckgml,
bird, wangjiexun2025
Hello:
This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
On Wed, 6 May 2026 19:43:30 +0800 you wrote:
> From: Jiexun Wang <wangjiexun2025@gmail.com>
>
> bt_sock_poll() walks the accept queue without synchronization, while
> child teardown can unlink the same socket and drop its last reference.
> The unsynchronized accept queue walk has existed since the initial
> Bluetooth import.
>
> [...]
Here is the summary with links:
- [v4] Bluetooth: serialize accept_q access
https://git.kernel.org/bluetooth/bluetooth-next/c/303bd23ee2e9
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-05-08 19:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-06 11:43 [PATCH v4] Bluetooth: serialize accept_q access Ren Wei
2026-05-06 13:56 ` [v4] " bluez.test.bot
2026-05-06 17:04 ` [PATCH v4] " Jann Horn
2026-05-08 19:00 ` patchwork-bot+bluetooth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox