Hi Pauli, On Fri, May 9, 2025 at 11:46 AM Pauli Virtanen <pav@xxxxxx> wrote: > > Hi, > > pe, 2025-05-09 kello 18:17 +0800, Yang Li via B4 Relay kirjoitti: > > From: Yang Li <yang.li@xxxxxxxxxxx> > > > > 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@xxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxx > > --- > > 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
