[PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS

[PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS

[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>

[PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS

[Yang Li via B4 Relay]

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>

RE: [v2] Bluetooth: fix socket matching ambiguity between BIS and CIS

[bluez.test.bot]

[-- 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

Re: [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS

[Pauli Virtanen]

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

Re: [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS

[Luiz Augusto von Dentz]

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

Re: [PATCH v2] Bluetooth: fix socket matching ambiguity between BIS and CIS

[Yang Li]

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