| Message ID | 20220722182248.1.I20e96c839200bb75cd6af80384f16c8c01498f57@changeid (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Bluetooth: hci_sync: Use safe loop when adding accept list | expand |
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 >
Hi 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 >
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
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;