* [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS @ 2025-05-09 10:17 ` Yang Li via B4 Relay 0 siblings, 0 replies; 6+ messages in thread From: Yang Li @ 2025-05-09 10:17 UTC (permalink / raw) To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz Cc: linux-bluetooth, linux-kernel, Yang Li When the DUT acts as a sink device, and a BIS already exists, creating a CIS connection can cause the kernel to incorrectly reference the BIS socket. This occurs because the socket lookup only checks for state == BT_LISTEN, without distinguishing between BIS and CIS socket types. To fix this, match the destination address (dst addr) during ISO socket lookup to differentiate between BIS and CIS sockets properly. Link: https://github.com/bluez/bluez/issues/1224 Signed-off-by: Yang Li <yang.li@amlogic.com> --- Changes in v2: - Fix compilation errors - Improved the problem description for clarity. - Link to v1: https://lore.kernel.org/r/20250507-iso-v1-1-6f60d243e037@amlogic.com --- net/bluetooth/hci_event.c | 34 +++++++++++++++++++--------------- net/bluetooth/iso.c | 12 +++++++++--- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 66052d6aaa1d..6b26344ad69f 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -6413,6 +6413,8 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, conn->sync_handle = le16_to_cpu(ev->handle); conn->sid = HCI_SID_INVALID; + conn->dst = ev->bdaddr; + conn->dst_type = ev->bdaddr_type; mask |= hci_proto_connect_ind(hdev, &ev->bdaddr, BIS_LINK, &flags); @@ -6425,7 +6427,7 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, goto unlock; /* Add connection to indicate PA sync event */ - pa_sync = hci_conn_add_unset(hdev, BIS_LINK, BDADDR_ANY, + pa_sync = hci_conn_add_unset(hdev, BIS_LINK, &ev->bdaddr, HCI_ROLE_SLAVE); if (IS_ERR(pa_sync)) @@ -6456,13 +6458,6 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, hci_dev_lock(hdev); - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); - if (!(mask & HCI_LM_ACCEPT)) - goto unlock; - - if (!(flags & HCI_PROTO_DEFER)) - goto unlock; - pa_sync = hci_conn_hash_lookup_pa_sync_handle (hdev, le16_to_cpu(ev->sync_handle)); @@ -6470,6 +6465,13 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, if (!pa_sync) goto unlock; + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); + if (!(mask & HCI_LM_ACCEPT)) + goto unlock; + + if (!(flags & HCI_PROTO_DEFER)) + goto unlock; + if (ev->data_status == LE_PA_DATA_COMPLETE && !test_and_set_bit(HCI_CONN_PA_SYNC, &pa_sync->flags)) { /* Notify iso layer */ @@ -6993,6 +6995,8 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data, set_bit(HCI_CONN_PA_SYNC, &bis->flags); bis->sync_handle = conn->sync_handle; + bis->dst = conn->dst; + bis->dst_type = conn->dst_type; bis->iso_qos.bcast.big = ev->handle; memset(&interval, 0, sizeof(interval)); memcpy(&interval, ev->latency, sizeof(ev->latency)); @@ -7038,13 +7042,6 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, hci_dev_lock(hdev); - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); - if (!(mask & HCI_LM_ACCEPT)) - goto unlock; - - if (!(flags & HCI_PROTO_DEFER)) - goto unlock; - pa_sync = hci_conn_hash_lookup_pa_sync_handle (hdev, le16_to_cpu(ev->sync_handle)); @@ -7054,6 +7051,13 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, pa_sync->iso_qos.bcast.encryption = ev->encryption; + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); + if (!(mask & HCI_LM_ACCEPT)) + goto unlock; + + if (!(flags & HCI_PROTO_DEFER)) + goto unlock; + /* Notify iso layer */ hci_connect_cfm(pa_sync, 0); diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index 6e2c752aaa8f..1dc233f04dbe 100644 --- a/net/bluetooth/iso.c +++ b/net/bluetooth/iso.c @@ -641,11 +641,12 @@ static struct sock *iso_get_sock(bdaddr_t *src, bdaddr_t *dst, continue; /* Exact match. */ - if (!bacmp(&iso_pi(sk)->src, src)) { + if (!bacmp(&iso_pi(sk)->src, src) + && !bacmp(&iso_pi(sk)->dst, dst) + ){ sock_hold(sk); break; } - /* Closest match */ if (!bacmp(&iso_pi(sk)->src, BDADDR_ANY)) { if (sk1) @@ -1962,7 +1963,7 @@ static void iso_conn_ready(struct iso_conn *conn) } if (!parent) - parent = iso_get_sock(&hcon->src, BDADDR_ANY, + parent = iso_get_sock(&hcon->src, &hcon->dst, BT_LISTEN, NULL, NULL); if (!parent) @@ -2203,6 +2204,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) } else { sk = iso_get_sock(&hdev->bdaddr, BDADDR_ANY, BT_LISTEN, NULL, NULL); + if (!sk) + sk = iso_get_sock(&hdev->bdaddr, bdaddr, + BT_LISTEN, NULL, NULL); + else + iso_pi(sk)->dst = *bdaddr; } done: --- base-commit: f3daca9b490154fbb0459848cc2ed61e8367bddc change-id: 20250506-iso-6515893b5bb3 Best regards, -- Yang Li <yang.li@amlogic.com> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS @ 2025-05-09 10:17 ` Yang Li via B4 Relay 0 siblings, 0 replies; 6+ messages in thread From: Yang Li via B4 Relay @ 2025-05-09 10:17 UTC (permalink / raw) To: Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz Cc: linux-bluetooth, linux-kernel, Yang Li From: Yang Li <yang.li@amlogic.com> When the DUT acts as a sink device, and a BIS already exists, creating a CIS connection can cause the kernel to incorrectly reference the BIS socket. This occurs because the socket lookup only checks for state == BT_LISTEN, without distinguishing between BIS and CIS socket types. To fix this, match the destination address (dst addr) during ISO socket lookup to differentiate between BIS and CIS sockets properly. Link: https://github.com/bluez/bluez/issues/1224 Signed-off-by: Yang Li <yang.li@amlogic.com> --- Changes in v2: - Fix compilation errors - Improved the problem description for clarity. - Link to v1: https://lore.kernel.org/r/20250507-iso-v1-1-6f60d243e037@amlogic.com --- net/bluetooth/hci_event.c | 34 +++++++++++++++++++--------------- net/bluetooth/iso.c | 12 +++++++++--- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 66052d6aaa1d..6b26344ad69f 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -6413,6 +6413,8 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, conn->sync_handle = le16_to_cpu(ev->handle); conn->sid = HCI_SID_INVALID; + conn->dst = ev->bdaddr; + conn->dst_type = ev->bdaddr_type; mask |= hci_proto_connect_ind(hdev, &ev->bdaddr, BIS_LINK, &flags); @@ -6425,7 +6427,7 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, goto unlock; /* Add connection to indicate PA sync event */ - pa_sync = hci_conn_add_unset(hdev, BIS_LINK, BDADDR_ANY, + pa_sync = hci_conn_add_unset(hdev, BIS_LINK, &ev->bdaddr, HCI_ROLE_SLAVE); if (IS_ERR(pa_sync)) @@ -6456,13 +6458,6 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, hci_dev_lock(hdev); - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); - if (!(mask & HCI_LM_ACCEPT)) - goto unlock; - - if (!(flags & HCI_PROTO_DEFER)) - goto unlock; - pa_sync = hci_conn_hash_lookup_pa_sync_handle (hdev, le16_to_cpu(ev->sync_handle)); @@ -6470,6 +6465,13 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, if (!pa_sync) goto unlock; + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); + if (!(mask & HCI_LM_ACCEPT)) + goto unlock; + + if (!(flags & HCI_PROTO_DEFER)) + goto unlock; + if (ev->data_status == LE_PA_DATA_COMPLETE && !test_and_set_bit(HCI_CONN_PA_SYNC, &pa_sync->flags)) { /* Notify iso layer */ @@ -6993,6 +6995,8 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data, set_bit(HCI_CONN_PA_SYNC, &bis->flags); bis->sync_handle = conn->sync_handle; + bis->dst = conn->dst; + bis->dst_type = conn->dst_type; bis->iso_qos.bcast.big = ev->handle; memset(&interval, 0, sizeof(interval)); memcpy(&interval, ev->latency, sizeof(ev->latency)); @@ -7038,13 +7042,6 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, hci_dev_lock(hdev); - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); - if (!(mask & HCI_LM_ACCEPT)) - goto unlock; - - if (!(flags & HCI_PROTO_DEFER)) - goto unlock; - pa_sync = hci_conn_hash_lookup_pa_sync_handle (hdev, le16_to_cpu(ev->sync_handle)); @@ -7054,6 +7051,13 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, pa_sync->iso_qos.bcast.encryption = ev->encryption; + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); + if (!(mask & HCI_LM_ACCEPT)) + goto unlock; + + if (!(flags & HCI_PROTO_DEFER)) + goto unlock; + /* Notify iso layer */ hci_connect_cfm(pa_sync, 0); diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index 6e2c752aaa8f..1dc233f04dbe 100644 --- a/net/bluetooth/iso.c +++ b/net/bluetooth/iso.c @@ -641,11 +641,12 @@ static struct sock *iso_get_sock(bdaddr_t *src, bdaddr_t *dst, continue; /* Exact match. */ - if (!bacmp(&iso_pi(sk)->src, src)) { + if (!bacmp(&iso_pi(sk)->src, src) + && !bacmp(&iso_pi(sk)->dst, dst) + ){ sock_hold(sk); break; } - /* Closest match */ if (!bacmp(&iso_pi(sk)->src, BDADDR_ANY)) { if (sk1) @@ -1962,7 +1963,7 @@ static void iso_conn_ready(struct iso_conn *conn) } if (!parent) - parent = iso_get_sock(&hcon->src, BDADDR_ANY, + parent = iso_get_sock(&hcon->src, &hcon->dst, BT_LISTEN, NULL, NULL); if (!parent) @@ -2203,6 +2204,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) } else { sk = iso_get_sock(&hdev->bdaddr, BDADDR_ANY, BT_LISTEN, NULL, NULL); + if (!sk) + sk = iso_get_sock(&hdev->bdaddr, bdaddr, + BT_LISTEN, NULL, NULL); + else + iso_pi(sk)->dst = *bdaddr; } done: --- base-commit: f3daca9b490154fbb0459848cc2ed61e8367bddc change-id: 20250506-iso-6515893b5bb3 Best regards, -- Yang Li <yang.li@amlogic.com> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [v2] Bluetooth: fix socket matching ambiguity between BIS and CIS 2025-05-09 10:17 ` Yang Li via B4 Relay (?) @ 2025-05-09 11:02 ` bluez.test.bot -1 siblings, 0 replies; 6+ messages in thread From: bluez.test.bot @ 2025-05-09 11:02 UTC (permalink / raw) To: linux-bluetooth, yang.li [-- Attachment #1: Type: text/plain, Size: 2896 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=961268 ---Test result--- Test Summary: CheckPatch PENDING 0.37 seconds GitLint PENDING 0.25 seconds SubjectPrefix PASS 1.15 seconds BuildKernel PASS 24.23 seconds CheckAllWarning PASS 26.98 seconds CheckSparse WARNING 30.38 seconds BuildKernel32 PASS 23.75 seconds TestRunnerSetup PASS 455.29 seconds TestRunner_l2cap-tester PASS 22.12 seconds TestRunner_iso-tester FAIL 41.14 seconds TestRunner_bnep-tester PASS 4.99 seconds TestRunner_mgmt-tester FAIL 121.78 seconds TestRunner_rfcomm-tester PASS 7.71 seconds TestRunner_sco-tester PASS 14.99 seconds TestRunner_ioctl-tester PASS 8.14 seconds TestRunner_mesh-tester PASS 6.38 seconds TestRunner_smp-tester PASS 7.15 seconds TestRunner_userchan-tester PASS 4.90 seconds IncrementalBuild PENDING 0.51 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: CheckSparse - WARNING Desc: Run sparse tool with linux kernel Output: net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h): ############################## Test: TestRunner_iso-tester - FAIL Desc: Run iso-tester with test-runner Output: Total: 127, Passed: 122 (96.1%), Failed: 4, Not Run: 1 Failed Test Cases ISO Broadcaster Receiver - Success Timed out 2.393 seconds ISO Broadcaster Receiver SID 0xff - Success Timed out 1.994 seconds ISO Broadcaster Receiver2 - Success Timed out 2.010 seconds ISO Broadcaster Receiver Encrypted - Success Timed out 1.986 seconds ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 490, Passed: 482 (98.4%), Failed: 4, Not Run: 4 Failed Test Cases LL Privacy - Add Device 3 (AL is full) Failed 0.202 seconds LL Privacy - Set Flags 1 (Add to RL) Failed 0.149 seconds LL Privacy - Set Flags 2 (Enable RL) Failed 0.157 seconds LL Privacy - Start Discovery 2 (Disable RL) Failed 0.198 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS 2025-05-09 10:17 ` Yang Li via B4 Relay (?) (?) @ 2025-05-09 15:46 ` Pauli Virtanen 2025-05-09 16:13 ` Luiz Augusto von Dentz 2025-05-12 11:55 ` Yang Li -1 siblings, 2 replies; 6+ messages in thread From: Pauli Virtanen @ 2025-05-09 15:46 UTC (permalink / raw) To: yang.li, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz Cc: linux-bluetooth, linux-kernel Hi, pe, 2025-05-09 kello 18:17 +0800, Yang Li via B4 Relay kirjoitti: > From: Yang Li <yang.li@amlogic.com> > > When the DUT acts as a sink device, and a BIS already exists, > creating a CIS connection can cause the kernel to incorrectly > reference the BIS socket. This occurs because the socket > lookup only checks for state == BT_LISTEN, without > distinguishing between BIS and CIS socket types. > > To fix this, match the destination address (dst addr) during > ISO socket lookup to differentiate between BIS and CIS sockets > properly. Does it work if you have both CIS and BIS established between the same two machines? Now that CIS_LINK and BIS_LINK are separate hci_conn types, it could make sense to introduce `__u8 hcon_type;` in iso_pinfo, maybe set in iso_connect/listen so that also the socket types won't be confused. > > Link: https://github.com/bluez/bluez/issues/1224 > > Signed-off-by: Yang Li <yang.li@amlogic.com> > --- > Changes in v2: > - Fix compilation errors > - Improved the problem description for clarity. > - Link to v1: https://lore.kernel.org/r/20250507-iso-v1-1-6f60d243e037@amlogic.com > --- > net/bluetooth/hci_event.c | 34 +++++++++++++++++++--------------- > net/bluetooth/iso.c | 12 +++++++++--- > 2 files changed, 28 insertions(+), 18 deletions(-) > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 66052d6aaa1d..6b26344ad69f 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -6413,6 +6413,8 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, > > conn->sync_handle = le16_to_cpu(ev->handle); > conn->sid = HCI_SID_INVALID; > + conn->dst = ev->bdaddr; > + conn->dst_type = ev->bdaddr_type; > > mask |= hci_proto_connect_ind(hdev, &ev->bdaddr, BIS_LINK, > &flags); > @@ -6425,7 +6427,7 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, > goto unlock; > > /* Add connection to indicate PA sync event */ > - pa_sync = hci_conn_add_unset(hdev, BIS_LINK, BDADDR_ANY, > + pa_sync = hci_conn_add_unset(hdev, BIS_LINK, &ev->bdaddr, > HCI_ROLE_SLAVE); Do these make the update of conn->dst in iso_conn_ready() unnecessary? It should be documented somewhere what are the different types of BIS_LINK hci_conn that exist, and what are their invariants... > if (IS_ERR(pa_sync)) > @@ -6456,13 +6458,6 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, > > hci_dev_lock(hdev); > > - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); > - if (!(mask & HCI_LM_ACCEPT)) > - goto unlock; > - > - if (!(flags & HCI_PROTO_DEFER)) > - goto unlock; > - > pa_sync = hci_conn_hash_lookup_pa_sync_handle > (hdev, > le16_to_cpu(ev->sync_handle)); > @@ -6470,6 +6465,13 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, > if (!pa_sync) > goto unlock; > > + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); > + if (!(mask & HCI_LM_ACCEPT)) > + goto unlock; > + > + if (!(flags & HCI_PROTO_DEFER)) > + goto unlock; > + Commit message should explain what this reordering of *_ind after pa_sync lookup/update are for. > if (ev->data_status == LE_PA_DATA_COMPLETE && > !test_and_set_bit(HCI_CONN_PA_SYNC, &pa_sync->flags)) { > /* Notify iso layer */ > @@ -6993,6 +6995,8 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data, > set_bit(HCI_CONN_PA_SYNC, &bis->flags); > > bis->sync_handle = conn->sync_handle; > + bis->dst = conn->dst; > + bis->dst_type = conn->dst_type; > bis->iso_qos.bcast.big = ev->handle; > memset(&interval, 0, sizeof(interval)); > memcpy(&interval, ev->latency, sizeof(ev->latency)); > @@ -7038,13 +7042,6 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, > > hci_dev_lock(hdev); > > - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); > - if (!(mask & HCI_LM_ACCEPT)) > - goto unlock; > - > - if (!(flags & HCI_PROTO_DEFER)) > - goto unlock; > - > pa_sync = hci_conn_hash_lookup_pa_sync_handle > (hdev, > le16_to_cpu(ev->sync_handle)); > @@ -7054,6 +7051,13 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, > > pa_sync->iso_qos.bcast.encryption = ev->encryption; > > + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); > + if (!(mask & HCI_LM_ACCEPT)) > + goto unlock; > + > + if (!(flags & HCI_PROTO_DEFER)) > + goto unlock; > + > > /* Notify iso layer */ > hci_connect_cfm(pa_sync, 0); > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c > index 6e2c752aaa8f..1dc233f04dbe 100644 > --- a/net/bluetooth/iso.c > +++ b/net/bluetooth/iso.c > @@ -641,11 +641,12 @@ static struct sock *iso_get_sock(bdaddr_t *src, bdaddr_t *dst, > continue; > > /* Exact match. */ > - if (!bacmp(&iso_pi(sk)->src, src)) { > + if (!bacmp(&iso_pi(sk)->src, src) > + && !bacmp(&iso_pi(sk)->dst, dst) > + ){ Code style issues here. > sock_hold(sk); > break; > } > - > /* Closest match */ > if (!bacmp(&iso_pi(sk)->src, BDADDR_ANY)) { > if (sk1) > @@ -1962,7 +1963,7 @@ static void iso_conn_ready(struct iso_conn *conn) > } > > if (!parent) > - parent = iso_get_sock(&hcon->src, BDADDR_ANY, > + parent = iso_get_sock(&hcon->src, &hcon->dst, > BT_LISTEN, NULL, NULL); I think the code here would be more clear if it's refactored to handle hcon->type == CIS_LINK and hcon->type == BIS_LINK with explicitly separate code path. What happens here if we have a BIS listener socket for `dst`, and `dst` initiates a CIS connection? Won't the CIS connection get resolved to the BIS listener socket? IIUC CIS listeners always have dst == BDADDR_ANY. BIS listeners have dst != BDADDR_ANY. Perhaps there could also be `__u8 hcon_type` in iso_pinfo that gets set to CIS_LINK or BIS_LINK in iso_connect/listen. > > if (!parent) > @@ -2203,6 +2204,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) > } else { > sk = iso_get_sock(&hdev->bdaddr, BDADDR_ANY, > BT_LISTEN, NULL, NULL); > + if (!sk) > + sk = iso_get_sock(&hdev->bdaddr, bdaddr, > + BT_LISTEN, NULL, NULL); > + else > + iso_pi(sk)->dst = *bdaddr; This updates the listener socket dst address with that of the connecting device? I think what is set in bind() shouldn't be modified later on. Isn't this wrong for CIS, won't it block connecting another device? > } > > done: > > --- > base-commit: f3daca9b490154fbb0459848cc2ed61e8367bddc > change-id: 20250506-iso-6515893b5bb3 > > Best regards, -- Pauli Virtanen ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS 2025-05-09 15:46 ` [PATCH v2] " Pauli Virtanen @ 2025-05-09 16:13 ` Luiz Augusto von Dentz 2025-05-12 11:55 ` Yang Li 1 sibling, 0 replies; 6+ messages in thread From: Luiz Augusto von Dentz @ 2025-05-09 16:13 UTC (permalink / raw) To: Pauli Virtanen Cc: yang.li, Marcel Holtmann, Johan Hedberg, linux-bluetooth, linux-kernel Hi Pauli, On Fri, May 9, 2025 at 11:46 AM Pauli Virtanen <pav@iki.fi> wrote: > > Hi, > > pe, 2025-05-09 kello 18:17 +0800, Yang Li via B4 Relay kirjoitti: > > From: Yang Li <yang.li@amlogic.com> > > > > When the DUT acts as a sink device, and a BIS already exists, > > creating a CIS connection can cause the kernel to incorrectly > > reference the BIS socket. This occurs because the socket > > lookup only checks for state == BT_LISTEN, without > > distinguishing between BIS and CIS socket types. > > > > To fix this, match the destination address (dst addr) during > > ISO socket lookup to differentiate between BIS and CIS sockets > > properly. > > Does it work if you have both CIS and BIS established between the same > two machines? > > Now that CIS_LINK and BIS_LINK are separate hci_conn types, it could > make sense to introduce `__u8 hcon_type;` in iso_pinfo, maybe set in > iso_connect/listen so that also the socket types won't be confused. We do store the hci_conn as part of iso_conn which is already a member of iso_pinfo, so the information of the conn type is already accessible from the iso.c. > > > > Link: https://github.com/bluez/bluez/issues/1224 > > > > Signed-off-by: Yang Li <yang.li@amlogic.com> > > --- > > Changes in v2: > > - Fix compilation errors > > - Improved the problem description for clarity. > > - Link to v1: https://lore.kernel.org/r/20250507-iso-v1-1-6f60d243e037@amlogic.com > > --- > > net/bluetooth/hci_event.c | 34 +++++++++++++++++++--------------- > > net/bluetooth/iso.c | 12 +++++++++--- > > 2 files changed, 28 insertions(+), 18 deletions(-) > > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > > index 66052d6aaa1d..6b26344ad69f 100644 > > --- a/net/bluetooth/hci_event.c > > +++ b/net/bluetooth/hci_event.c > > @@ -6413,6 +6413,8 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, > > > > conn->sync_handle = le16_to_cpu(ev->handle); > > conn->sid = HCI_SID_INVALID; > > + conn->dst = ev->bdaddr; > > + conn->dst_type = ev->bdaddr_type; > > > > mask |= hci_proto_connect_ind(hdev, &ev->bdaddr, BIS_LINK, > > &flags); > > @@ -6425,7 +6427,7 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, > > goto unlock; > > > > /* Add connection to indicate PA sync event */ > > - pa_sync = hci_conn_add_unset(hdev, BIS_LINK, BDADDR_ANY, > > + pa_sync = hci_conn_add_unset(hdev, BIS_LINK, &ev->bdaddr, > > HCI_ROLE_SLAVE); > > Do these make the update of conn->dst in iso_conn_ready() unnecessary? > > It should be documented somewhere what are the different types of > BIS_LINK hci_conn that exist, and what are their invariants... > > > if (IS_ERR(pa_sync)) > > @@ -6456,13 +6458,6 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, > > > > hci_dev_lock(hdev); > > > > - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); > > - if (!(mask & HCI_LM_ACCEPT)) > > - goto unlock; > > - > > - if (!(flags & HCI_PROTO_DEFER)) > > - goto unlock; > > - > > pa_sync = hci_conn_hash_lookup_pa_sync_handle > > (hdev, > > le16_to_cpu(ev->sync_handle)); > > @@ -6470,6 +6465,13 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, > > if (!pa_sync) > > goto unlock; > > > > + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); > > + if (!(mask & HCI_LM_ACCEPT)) > > + goto unlock; > > + > > + if (!(flags & HCI_PROTO_DEFER)) > > + goto unlock; > > + > > Commit message should explain what this reordering of *_ind after > pa_sync lookup/update are for. > > > if (ev->data_status == LE_PA_DATA_COMPLETE && > > !test_and_set_bit(HCI_CONN_PA_SYNC, &pa_sync->flags)) { > > /* Notify iso layer */ > > @@ -6993,6 +6995,8 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data, > > set_bit(HCI_CONN_PA_SYNC, &bis->flags); > > > > bis->sync_handle = conn->sync_handle; > > + bis->dst = conn->dst; > > + bis->dst_type = conn->dst_type; > > bis->iso_qos.bcast.big = ev->handle; > > memset(&interval, 0, sizeof(interval)); > > memcpy(&interval, ev->latency, sizeof(ev->latency)); > > @@ -7038,13 +7042,6 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, > > > > hci_dev_lock(hdev); > > > > - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); > > - if (!(mask & HCI_LM_ACCEPT)) > > - goto unlock; > > - > > - if (!(flags & HCI_PROTO_DEFER)) > > - goto unlock; > > - > > pa_sync = hci_conn_hash_lookup_pa_sync_handle > > (hdev, > > le16_to_cpu(ev->sync_handle)); > > @@ -7054,6 +7051,13 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, > > > > pa_sync->iso_qos.bcast.encryption = ev->encryption; > > > > + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); > > + if (!(mask & HCI_LM_ACCEPT)) > > + goto unlock; > > + > > + if (!(flags & HCI_PROTO_DEFER)) > > + goto unlock; > > + > > > > /* Notify iso layer */ > > hci_connect_cfm(pa_sync, 0); > > > > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c > > index 6e2c752aaa8f..1dc233f04dbe 100644 > > --- a/net/bluetooth/iso.c > > +++ b/net/bluetooth/iso.c > > @@ -641,11 +641,12 @@ static struct sock *iso_get_sock(bdaddr_t *src, bdaddr_t *dst, > > continue; > > > > /* Exact match. */ > > - if (!bacmp(&iso_pi(sk)->src, src)) { > > + if (!bacmp(&iso_pi(sk)->src, src) > > + && !bacmp(&iso_pi(sk)->dst, dst) > > + ){ > > Code style issues here. > > > sock_hold(sk); > > break; > > } > > - > > /* Closest match */ > > if (!bacmp(&iso_pi(sk)->src, BDADDR_ANY)) { > > if (sk1) > > @@ -1962,7 +1963,7 @@ static void iso_conn_ready(struct iso_conn *conn) > > } > > > > if (!parent) > > - parent = iso_get_sock(&hcon->src, BDADDR_ANY, > > + parent = iso_get_sock(&hcon->src, &hcon->dst, > > BT_LISTEN, NULL, NULL); > > I think the code here would be more clear if it's refactored to handle > hcon->type == CIS_LINK and hcon->type == BIS_LINK with explicitly > separate code path. > > What happens here if we have a BIS listener socket for `dst`, and `dst` > initiates a CIS connection? Won't the CIS connection get resolved to > the BIS listener socket? > > IIUC CIS listeners always have dst == BDADDR_ANY. BIS listeners have > dst != BDADDR_ANY. > > Perhaps there could also be `__u8 hcon_type` in iso_pinfo that gets set > to CIS_LINK or BIS_LINK in iso_connect/listen. > > > > > if (!parent) > > @@ -2203,6 +2204,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) > > } else { > > sk = iso_get_sock(&hdev->bdaddr, BDADDR_ANY, > > BT_LISTEN, NULL, NULL); > > + if (!sk) > > + sk = iso_get_sock(&hdev->bdaddr, bdaddr, > > + BT_LISTEN, NULL, NULL); > > + else > > + iso_pi(sk)->dst = *bdaddr; > > This updates the listener socket dst address with that of the > connecting device? I think what is set in bind() shouldn't be modified > later on. > > Isn't this wrong for CIS, won't it block connecting another device? On top of these comments this lacks proper testing with iso-tester which seems to be failing with these changes, beside that we would probably benefit from having a test that emulates this topology, which appears to mix CIS and BIS together, which doesn't seem to be covered in any audio configuration (AC-#) from the spec, although it seems valid to support it if controllers are capable of doing it. > > } > > > > done: > > > > --- > > base-commit: f3daca9b490154fbb0459848cc2ed61e8367bddc > > change-id: 20250506-iso-6515893b5bb3 > > > > Best regards, > > -- > Pauli Virtanen -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS 2025-05-09 15:46 ` [PATCH v2] " Pauli Virtanen 2025-05-09 16:13 ` Luiz Augusto von Dentz @ 2025-05-12 11:55 ` Yang Li 1 sibling, 0 replies; 6+ messages in thread From: Yang Li @ 2025-05-12 11:55 UTC (permalink / raw) To: Pauli Virtanen, Marcel Holtmann, Johan Hedberg, Luiz Augusto von Dentz Cc: linux-bluetooth, linux-kernel Hi, > [ EXTERNAL EMAIL ] > > Hi, > > pe, 2025-05-09 kello 18:17 +0800, Yang Li via B4 Relay kirjoitti: >> From: Yang Li <yang.li@amlogic.com> >> >> When the DUT acts as a sink device, and a BIS already exists, >> creating a CIS connection can cause the kernel to incorrectly >> reference the BIS socket. This occurs because the socket >> lookup only checks for state == BT_LISTEN, without >> distinguishing between BIS and CIS socket types. >> >> To fix this, match the destination address (dst addr) during >> ISO socket lookup to differentiate between BIS and CIS sockets >> properly. > Does it work if you have both CIS and BIS established between the same > two machines? Yes, it works. The DUT functions as a BIS sink, synchronizing with a BIS source to receive BIS ISO data. Simultaneously, it establishes a CIS connection with a K70 smartphone to receive CIS ISO data as well. > > Now that CIS_LINK and BIS_LINK are separate hci_conn types, it could > make sense to introduce `__u8 hcon_type;` in iso_pinfo, maybe set in > iso_connect/listen so that also the socket types won't be confused. The |hcon_type| can only distinguish whether the connection type is BIS or CIS, but it cannot be used in the |iso_get_sock| function to differentiate between BIS and CIS sockets. > >> Link: https://github.com/bluez/bluez/issues/1224 >> >> Signed-off-by: Yang Li <yang.li@amlogic.com> >> --- >> Changes in v2: >> - Fix compilation errors >> - Improved the problem description for clarity. >> - Link to v1: https://lore.kernel.org/r/20250507-iso-v1-1-6f60d243e037@amlogic.com >> --- >> net/bluetooth/hci_event.c | 34 +++++++++++++++++++--------------- >> net/bluetooth/iso.c | 12 +++++++++--- >> 2 files changed, 28 insertions(+), 18 deletions(-) >> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >> index 66052d6aaa1d..6b26344ad69f 100644 >> --- a/net/bluetooth/hci_event.c >> +++ b/net/bluetooth/hci_event.c >> @@ -6413,6 +6413,8 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, >> >> conn->sync_handle = le16_to_cpu(ev->handle); >> conn->sid = HCI_SID_INVALID; >> + conn->dst = ev->bdaddr; >> + conn->dst_type = ev->bdaddr_type; >> >> mask |= hci_proto_connect_ind(hdev, &ev->bdaddr, BIS_LINK, >> &flags); >> @@ -6425,7 +6427,7 @@ static void hci_le_pa_sync_estabilished_evt(struct hci_dev *hdev, void *data, >> goto unlock; >> >> /* Add connection to indicate PA sync event */ >> - pa_sync = hci_conn_add_unset(hdev, BIS_LINK, BDADDR_ANY, >> + pa_sync = hci_conn_add_unset(hdev, BIS_LINK, &ev->bdaddr, >> HCI_ROLE_SLAVE); > Do these make the update of conn->dst in iso_conn_ready() unnecessary? > > It should be documented somewhere what are the different types of > BIS_LINK hci_conn that exist, and what are their invariants... The pa_sync structure needs to retain the dst addr so that it can be used later to match the correct BIS socket. > >> if (IS_ERR(pa_sync)) >> @@ -6456,13 +6458,6 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, >> >> hci_dev_lock(hdev); >> >> - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); >> - if (!(mask & HCI_LM_ACCEPT)) >> - goto unlock; >> - >> - if (!(flags & HCI_PROTO_DEFER)) >> - goto unlock; >> - >> pa_sync = hci_conn_hash_lookup_pa_sync_handle >> (hdev, >> le16_to_cpu(ev->sync_handle)); >> @@ -6470,6 +6465,13 @@ static void hci_le_per_adv_report_evt(struct hci_dev *hdev, void *data, >> if (!pa_sync) >> goto unlock; >> >> + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); >> + if (!(mask & HCI_LM_ACCEPT)) >> + goto unlock; >> + >> + if (!(flags & HCI_PROTO_DEFER)) >> + goto unlock; >> + > Commit message should explain what this reordering of *_ind after > pa_sync lookup/update are for. The iso_connect_ind() function needs to locate the appropriate BIS socket by matching the destination address stored in pa_sync->dst. This is just a position swap and does not introduce any logical changes. > >> if (ev->data_status == LE_PA_DATA_COMPLETE && >> !test_and_set_bit(HCI_CONN_PA_SYNC, &pa_sync->flags)) { >> /* Notify iso layer */ >> @@ -6993,6 +6995,8 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data, >> set_bit(HCI_CONN_PA_SYNC, &bis->flags); >> >> bis->sync_handle = conn->sync_handle; >> + bis->dst = conn->dst; >> + bis->dst_type = conn->dst_type; >> bis->iso_qos.bcast.big = ev->handle; >> memset(&interval, 0, sizeof(interval)); >> memcpy(&interval, ev->latency, sizeof(ev->latency)); >> @@ -7038,13 +7042,6 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, >> >> hci_dev_lock(hdev); >> >> - mask |= hci_proto_connect_ind(hdev, BDADDR_ANY, BIS_LINK, &flags); >> - if (!(mask & HCI_LM_ACCEPT)) >> - goto unlock; >> - >> - if (!(flags & HCI_PROTO_DEFER)) >> - goto unlock; >> - >> pa_sync = hci_conn_hash_lookup_pa_sync_handle >> (hdev, >> le16_to_cpu(ev->sync_handle)); >> @@ -7054,6 +7051,13 @@ static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, >> >> pa_sync->iso_qos.bcast.encryption = ev->encryption; >> >> + mask |= hci_proto_connect_ind(hdev, &pa_sync->dst, BIS_LINK, &flags); >> + if (!(mask & HCI_LM_ACCEPT)) >> + goto unlock; >> + >> + if (!(flags & HCI_PROTO_DEFER)) >> + goto unlock; >> + >> >> /* Notify iso layer */ >> hci_connect_cfm(pa_sync, 0); >> >> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c >> index 6e2c752aaa8f..1dc233f04dbe 100644 >> --- a/net/bluetooth/iso.c >> +++ b/net/bluetooth/iso.c >> @@ -641,11 +641,12 @@ static struct sock *iso_get_sock(bdaddr_t *src, bdaddr_t *dst, >> continue; >> >> /* Exact match. */ >> - if (!bacmp(&iso_pi(sk)->src, src)) { >> + if (!bacmp(&iso_pi(sk)->src, src) >> + && !bacmp(&iso_pi(sk)->dst, dst) >> + ){ > Code style issues here. Will do it. >> sock_hold(sk); >> break; >> } >> - >> /* Closest match */ >> if (!bacmp(&iso_pi(sk)->src, BDADDR_ANY)) { >> if (sk1) >> @@ -1962,7 +1963,7 @@ static void iso_conn_ready(struct iso_conn *conn) >> } >> >> if (!parent) >> - parent = iso_get_sock(&hcon->src, BDADDR_ANY, >> + parent = iso_get_sock(&hcon->src, &hcon->dst, >> BT_LISTEN, NULL, NULL); > I think the code here would be more clear if it's refactored to handle > hcon->type == CIS_LINK and hcon->type == BIS_LINK with explicitly > separate code path. > > What happens here if we have a BIS listener socket for `dst`, and `dst` > initiates a CIS connection? Won't the CIS connection get resolved to > the BIS listener socket? > > IIUC CIS listeners always have dst == BDADDR_ANY. BIS listeners have > dst != BDADDR_ANY. > > Perhaps there could also be `__u8 hcon_type` in iso_pinfo that gets set > to CIS_LINK or BIS_LINK in iso_connect/listen. I completely agree with your point that |hconn->type| should be specified for both |bis_listen| and |cis_listen|. However, I believe that the destination address (|dst addr|) should also be provided. Since a CIS connection is based on an LE connection, the |dst addr| must already be known at the time of listening. Similarly, for BIS listening, the |dst addr| can be determined when the BIG Info is received. Therefore, it is possible to know the |dst addr| at that point as well. Moreover, if multiple BIS streams are to be supported simultaneously, relying solely on |hconn->type| is not sufficient. So, if we can set the |dst addr| during the |iso_sock_listen()| call, there would be no need to assign it later in the |*_connect_ind| functions. >> if (!parent) >> @@ -2203,6 +2204,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags) >> } else { >> sk = iso_get_sock(&hdev->bdaddr, BDADDR_ANY, >> BT_LISTEN, NULL, NULL); >> + if (!sk) >> + sk = iso_get_sock(&hdev->bdaddr, bdaddr, >> + BT_LISTEN, NULL, NULL); >> + else >> + iso_pi(sk)->dst = *bdaddr; > This updates the listener socket dst address with that of the > connecting device? I think what is set in bind() shouldn't be modified > later on. > > Isn't this wrong for CIS, won't it block connecting another device? Yes, exactly. So far, I haven't found a better approach other than updating the CIS socket's |dst addr| within the |*_connect_ind| function. Thanks~ > >> } >> >> done: >> >> --- >> base-commit: f3daca9b490154fbb0459848cc2ed61e8367bddc >> change-id: 20250506-iso-6515893b5bb3 >> >> Best regards, > -- > Pauli Virtanen ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-05-12 11:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-09 10:17 [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS Yang Li 2025-05-09 10:17 ` Yang Li via B4 Relay 2025-05-09 11:02 ` [v2] " bluez.test.bot 2025-05-09 15:46 ` [PATCH v2] " Pauli Virtanen 2025-05-09 16:13 ` Luiz Augusto von Dentz 2025-05-12 11:55 ` Yang Li
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.