Hi,
[ EXTERNAL EMAIL ]
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?
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@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...
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
