-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Bluetooth: ISO: Fix issue with BIS tx_complete #93278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The assert from this PR may be superseded by #93279 |
df12b0e
to
41bc2d1
Compare
41bc2d1
to
109eb53
Compare
109eb53
to
f7e0400
Compare
@PavelVPV FYI, as this issue concerns the way Host uses system workqueue, potentially causing issues to other users of system work queue. |
@@ -1,9 +1,12 @@ | |||
/* | |||
* Copyright (c) 2024 Nordic Semiconductor ASA | |||
* Copyright (c) 2024-2025 Nordic Semiconductor ASA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we don't need to update copyright year.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't we ever managed to write and guidelines on this :D I usually just update them when I remember them, but I can omit it if you prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me. I remember getting such comment long time ago, but can't find it now.
Perhaps, all work items should be moved below |
f7e0400
to
c1bad57
Compare
I moved the assert to |
c1bad57
to
e7bf83c
Compare
BIS termination as broadcaster is handled different than ACL and CIS, and in rare chances the tx_complete for BIS may not have been completed in the system workqueue before iso_new was called for the same bt_conn struct (e.g. via bt_iso_cig_create), which would perform k_work_init(&conn->tx_complete_work, tx_complete_work); but where conn->tx_complete_work still existed in the system workqueue, which would cause the list of pending items on the system workqueue to be removed as the `next` pointer would be NULL. This also adds an assert in bt_conn_new to prevent this issue from appearing again. Signed-off-by: Emil Gydesen <[email protected]>
e7bf83c
to
2e043d6
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a race condition in Bluetooth ISO (Isochronous) connections where BIS (Broadcast Isochronous Stream) termination could cause work queue corruption. The issue occurred when tx_complete_work
was reinitialized while still pending in the system workqueue, potentially corrupting the queue's linked list structure.
Key changes:
- Add explicit tx_complete cleanup for BIS broadcaster termination
- Add assertion to detect work queue corruption during connection deallocation
- Add mock functions for k_work_busy_get to support testing
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
subsys/bluetooth/host/iso.c | Adds explicit bt_conn_tx_notify call for BIS broadcaster disconnection to flush pending tx_complete work |
subsys/bluetooth/host/conn.c | Adds assertion to detect pending tx_complete_work during connection deallocation |
tests/bluetooth/host/conn/mocks/kernel.h | Declares mock function for k_work_busy_get |
tests/bluetooth/host/conn/mocks/kernel.c | Defines mock function for k_work_busy_get |
IF_ENABLED(CONFIG_BT_CONN_TX, | ||
(__ASSERT(!(deallocated && k_work_is_pending(&conn->tx_complete_work)), | ||
"tx_complete_work is pending when conn is deallocated"))); | ||
conn = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting conn to NULL after the assertion will cause the assertion to access a NULL pointer. The conn = NULL assignment should remain after the assertion, but the assertion should be moved before this line or use a different approach to access the work structure.
Copilot uses AI. Check for mistakes.
BIS termination as broadcaster is handled different than ACL and CIS, and in rare chances the
tx_complete for BIS may not have been completed in the system workqueue before iso_new was called for the same bt_conn struct (e.g. via bt_iso_cig_create), which would perform
k_work_init(&conn->tx_complete_work, tx_complete_work);
but where conn->tx_complete_work still existed in
the system workqueue, which would cause the list
of pending items on the system workqueue to be removed as the
next
pointer would be NULL.This also adds an assert in bt_conn_new to prevent this issue from appearing again.