net/bluetooth/hci_sync.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
From: Archie Pusaka <apusaka@chromium.org>
When in the middle of adding accept list, the userspace can still
remove devices, therefore causing crash if the removed device is
the one being processed.
Use a safe loop mechanism to guard against deletion while iterating
the pending items.
Below is a sample btsnoop log when user enters wrong passkey when
pairing a LE keyboard and the corresponding stacktrace.
@ MGMT Event: Command Complete (0x0001) plen 10
Add Device (0x0033) plen 7
Status: Success (0x00)
LE Address: CA:CA:BD:78:37:F9 (Static)
< HCI Command: LE Add Device To Accept List (0x08|0x0011) plen 7
Address type: Random (0x01)
Address: CA:CA:BD:78:37:F9 (Static)
@ MGMT Event: Device Removed (0x001b) plen 7
LE Address: CA:CA:BD:78:37:F9 (Static)
> HCI Event: Command Complete (0x0e) plen 4
LE Add Device To Accept List (0x08|0x0011) ncmd 1
Status: Success (0x00)
[ 167.409813] Call trace:
[ 167.409983] hci_le_add_accept_list_sync+0x64/0x26c
[ 167.410150] hci_update_passive_scan_sync+0x5f0/0x6dc
[ 167.410318] add_device_sync+0x18/0x24
[ 167.410486] hci_cmd_sync_work+0xe8/0x150
[ 167.410509] process_one_work+0x140/0x4d0
[ 167.410526] worker_thread+0x134/0x2e4
[ 167.410544] kthread+0x148/0x160
[ 167.410562] ret_from_fork+0x10/0x30
Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Zhengping Jiang <jiangzp@google.com>
Reviewed-by: Michael Sun <michaelfsun@google.com>
---
net/bluetooth/hci_sync.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 3067d94e7a8e..8e843d34f7de 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1863,7 +1863,7 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev,
*/
static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
{
- struct hci_conn_params *params;
+ struct hci_conn_params *params, *tmp;
struct bdaddr_list *b, *t;
u8 num_entries = 0;
bool pend_conn, pend_report;
@@ -1930,7 +1930,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
* just abort and return filer policy value to not use the
* accept list.
*/
- list_for_each_entry(params, &hdev->pend_le_conns, action) {
+ list_for_each_entry_safe(params, tmp, &hdev->pend_le_conns, action) {
err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
if (err)
goto done;
@@ -1940,7 +1940,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
* the list of pending reports and also add these to the
* accept list if there is still space. Abort if space runs out.
*/
- list_for_each_entry(params, &hdev->pend_le_reports, action) {
+ list_for_each_entry_safe(params, tmp, &hdev->pend_le_reports, action) {
err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
if (err)
goto done;
--
2.37.1.359.gd136c6c3e2-googHi Archie,
On Fri, Jul 22, 2022 at 3:23 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> When in the middle of adding accept list, the userspace can still
> remove devices, therefore causing crash if the removed device is
> the one being processed.
>
> Use a safe loop mechanism to guard against deletion while iterating
> the pending items.
>
> Below is a sample btsnoop log when user enters wrong passkey when
> pairing a LE keyboard and the corresponding stacktrace.
> @ MGMT Event: Command Complete (0x0001) plen 10
> Add Device (0x0033) plen 7
> Status: Success (0x00)
> LE Address: CA:CA:BD:78:37:F9 (Static)
> < HCI Command: LE Add Device To Accept List (0x08|0x0011) plen 7
> Address type: Random (0x01)
> Address: CA:CA:BD:78:37:F9 (Static)
> @ MGMT Event: Device Removed (0x001b) plen 7
> LE Address: CA:CA:BD:78:37:F9 (Static)
> > HCI Event: Command Complete (0x0e) plen 4
> LE Add Device To Accept List (0x08|0x0011) ncmd 1
> Status: Success (0x00)
>
> [ 167.409813] Call trace:
> [ 167.409983] hci_le_add_accept_list_sync+0x64/0x26c
> [ 167.410150] hci_update_passive_scan_sync+0x5f0/0x6dc
> [ 167.410318] add_device_sync+0x18/0x24
> [ 167.410486] hci_cmd_sync_work+0xe8/0x150
> [ 167.410509] process_one_work+0x140/0x4d0
> [ 167.410526] worker_thread+0x134/0x2e4
> [ 167.410544] kthread+0x148/0x160
> [ 167.410562] ret_from_fork+0x10/0x30
>
> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> Reviewed-by: Zhengping Jiang <jiangzp@google.com>
> Reviewed-by: Michael Sun <michaelfsun@google.com>
>
> ---
>
> net/bluetooth/hci_sync.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 3067d94e7a8e..8e843d34f7de 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1863,7 +1863,7 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev,
> */
> static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
> {
> - struct hci_conn_params *params;
> + struct hci_conn_params *params, *tmp;
> struct bdaddr_list *b, *t;
> u8 num_entries = 0;
> bool pend_conn, pend_report;
> @@ -1930,7 +1930,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
> * just abort and return filer policy value to not use the
> * accept list.
> */
> - list_for_each_entry(params, &hdev->pend_le_conns, action) {
> + list_for_each_entry_safe(params, tmp, &hdev->pend_le_conns, action) {
> err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
> if (err)
> goto done;
> @@ -1940,7 +1940,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
> * the list of pending reports and also add these to the
> * accept list if there is still space. Abort if space runs out.
> */
> - list_for_each_entry(params, &hdev->pend_le_reports, action) {
> + list_for_each_entry_safe(params, tmp, &hdev->pend_le_reports, action) {
> err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
> if (err)
> goto done;
Hmm if this happens it means other threads are actually interfering
with cmd_sync queue which is something that is probably a bug since
the whole point of cmd_sync is to serialize the commands making it
easier to do more complex state updates (such accept+resolve list
updates), we could perhaps still apply this change as a workaround but
ultimately I think it would be better to add a mgmt-tester reproducing
the issue and have a proper fix of the code updating the list from a
different thread.
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Luiz Augusto von Dentz
Hi Eric and Luiz, > "the userspace can still remove devices" is a bit vague. I mean removing devices via MGMT command. > It seems that the issue at hand is that hci_le_add_accept_list_sync() can > move the current item from pend_le_conns / pend_le_reports lists ? The issue is, hci_le_add_accept_list_sync() is iterating the lists when the content is being removed elsewhere. > Hopefully these lists can not be changed by other threads while > hci_update_accept_list_sync() is running ? Probably. Looks like Luiz also thinks the same way. > Please add a Fixes: tag Unfortunately I don't know when this is introduced. > Hmm if this happens it means other threads are actually interfering > with cmd_sync queue which is something that is probably a bug since > the whole point of cmd_sync is to serialize the commands making it > easier to do more complex state updates (such accept+resolve list > updates) Thanks, I haven't fully grasped the intention of having hci_sync and how to properly use it. > we could perhaps still apply this change as a workaround but > ultimately I think it would be better to add a mgmt-tester reproducing > the issue and have a proper fix of the code updating the list from a > different thread. Agree. Having said that, I don't think currently I have the time to invest in writing a test and a proper fix, so my apologies on this. Best, Archie
On Fri, Jul 22, 2022 at 12:23 PM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> When in the middle of adding accept list, the userspace can still
> remove devices, therefore causing crash if the removed device is
> the one being processed.
>
> Use a safe loop mechanism to guard against deletion while iterating
> the pending items.
"the userspace can still remove devices" is a bit vague.
It seems that the issue at hand is that hci_le_add_accept_list_sync() can
move the current item from pend_le_conns / pend_le_reports lists ?
Hopefully these lists can not be changed by other threads while
hci_update_accept_list_sync() is running ?
>
> Below is a sample btsnoop log when user enters wrong passkey when
> pairing a LE keyboard and the corresponding stacktrace.
> @ MGMT Event: Command Complete (0x0001) plen 10
> Add Device (0x0033) plen 7
> Status: Success (0x00)
> LE Address: CA:CA:BD:78:37:F9 (Static)
> < HCI Command: LE Add Device To Accept List (0x08|0x0011) plen 7
> Address type: Random (0x01)
> Address: CA:CA:BD:78:37:F9 (Static)
> @ MGMT Event: Device Removed (0x001b) plen 7
> LE Address: CA:CA:BD:78:37:F9 (Static)
> > HCI Event: Command Complete (0x0e) plen 4
> LE Add Device To Accept List (0x08|0x0011) ncmd 1
> Status: Success (0x00)
>
> [ 167.409813] Call trace:
> [ 167.409983] hci_le_add_accept_list_sync+0x64/0x26c
> [ 167.410150] hci_update_passive_scan_sync+0x5f0/0x6dc
> [ 167.410318] add_device_sync+0x18/0x24
> [ 167.410486] hci_cmd_sync_work+0xe8/0x150
> [ 167.410509] process_one_work+0x140/0x4d0
> [ 167.410526] worker_thread+0x134/0x2e4
> [ 167.410544] kthread+0x148/0x160
> [ 167.410562] ret_from_fork+0x10/0x30
>
> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Please add a Fixes: tag
> Reviewed-by: Zhengping Jiang <jiangzp@google.com>
> Reviewed-by: Michael Sun <michaelfsun@google.com>
>
> ---
>
> net/bluetooth/hci_sync.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 3067d94e7a8e..8e843d34f7de 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1863,7 +1863,7 @@ struct sk_buff *hci_read_local_oob_data_sync(struct hci_dev *hdev,
> */
> static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
> {
> - struct hci_conn_params *params;
> + struct hci_conn_params *params, *tmp;
> struct bdaddr_list *b, *t;
> u8 num_entries = 0;
> bool pend_conn, pend_report;
> @@ -1930,7 +1930,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
> * just abort and return filer policy value to not use the
> * accept list.
> */
> - list_for_each_entry(params, &hdev->pend_le_conns, action) {
> + list_for_each_entry_safe(params, tmp, &hdev->pend_le_conns, action) {
> err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
> if (err)
> goto done;
> @@ -1940,7 +1940,7 @@ static u8 hci_update_accept_list_sync(struct hci_dev *hdev)
> * the list of pending reports and also add these to the
> * accept list if there is still space. Abort if space runs out.
> */
> - list_for_each_entry(params, &hdev->pend_le_reports, action) {
> + list_for_each_entry_safe(params, tmp, &hdev->pend_le_reports, action) {
> err = hci_le_add_accept_list_sync(hdev, params, &num_entries);
> if (err)
> goto done;
> --
> 2.37.1.359.gd136c6c3e2-goog
>
© 2016 - 2026 Red Hat, Inc.