net/bluetooth/hci_event.c | 34 +++++++++++++++++++--------------- net/bluetooth/iso.c | 12 +++++++++--- 2 files changed, 28 insertions(+), 18 deletions(-)
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>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
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
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
© 2016 - 2025 Red Hat, Inc.