Hi, On Mon, Jun 30, 2025 at 2:15 AM Yang Li <yang.li@xxxxxxxxxxx> wrote: > > Hi, > > [ EXTERNAL EMAIL ] > > > > Hi, > > > > On Fri, Jun 27, 2025 at 7:31 AM Yang Li <yang.li@xxxxxxxxxxx> wrote: > >> Hi Luiz, > >>> [ EXTERNAL EMAIL ] > >>> > >>> Hi Yang, > >>> > >>> On Thu, Jun 26, 2025 at 1:54 AM Yang Li <yang.li@xxxxxxxxxxx> wrote: > >>>> Hi Pauli, > >>>>> [ EXTERNAL EMAIL ] > >>>>> > >>>>> Hi, > >>>>> > >>>>> ke, 2025-06-25 kello 16:42 +0800, Yang Li via B4 Relay kirjoitti: > >>>>>> From: Yang Li <yang.li@xxxxxxxxxxx> > >>>>>> > >>>>>> When the BIS source stops, the controller sends an LE BIG Sync Lost > >>>>>> event (subevent 0x1E). Currently, this event is not handled, causing > >>>>>> the BIS stream to remain active in BlueZ and preventing recovery. > >>>>>> > >>>>>> Signed-off-by: Yang Li <yang.li@xxxxxxxxxxx> > >>>>>> --- > >>>>>> Changes in v2: > >>>>>> - Matching the BIG handle is required when looking up a BIG connection. > >>>>>> - Use ev->reason to determine the cause of disconnection. > >>>>>> - Call hci_conn_del after hci_disconnect_cfm to remove the connection entry > >>>>>> - Delete the big connection > >>>>>> - Link to v1: https://lore.kernel.org/r/20250624-handle_big_sync_lost_event-v1-1-c32ce37dd6a5@xxxxxxxxxxx > >>>>>> --- > >>>>>> include/net/bluetooth/hci.h | 6 ++++++ > >>>>>> net/bluetooth/hci_event.c | 31 +++++++++++++++++++++++++++++++ > >>>>>> 2 files changed, 37 insertions(+) > >>>>>> > >>>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > >>>>>> index 82cbd54443ac..48389a64accb 100644 > >>>>>> --- a/include/net/bluetooth/hci.h > >>>>>> +++ b/include/net/bluetooth/hci.h > >>>>>> @@ -2849,6 +2849,12 @@ struct hci_evt_le_big_sync_estabilished { > >>>>>> __le16 bis[]; > >>>>>> } __packed; > >>>>>> > >>>>>> +#define HCI_EVT_LE_BIG_SYNC_LOST 0x1e > >>>>>> +struct hci_evt_le_big_sync_lost { > >>>>>> + __u8 handle; > >>>>>> + __u8 reason; > >>>>>> +} __packed; > >>>>>> + > >>>>>> #define HCI_EVT_LE_BIG_INFO_ADV_REPORT 0x22 > >>>>>> struct hci_evt_le_big_info_adv_report { > >>>>>> __le16 sync_handle; > >>>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > >>>>>> index 66052d6aaa1d..d0b9c8dca891 100644 > >>>>>> --- a/net/bluetooth/hci_event.c > >>>>>> +++ b/net/bluetooth/hci_event.c > >>>>>> @@ -7026,6 +7026,32 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data, > >>>>>> hci_dev_unlock(hdev); > >>>>>> } > >>>>>> > >>>>>> +static void hci_le_big_sync_lost_evt(struct hci_dev *hdev, void *data, > >>>>>> + struct sk_buff *skb) > >>>>>> +{ > >>>>>> + struct hci_evt_le_big_sync_lost *ev = data; > >>>>>> + struct hci_conn *bis, *conn; > >>>>>> + > >>>>>> + bt_dev_dbg(hdev, "big handle 0x%2.2x", ev->handle); > >>>>>> + > >>>>>> + hci_dev_lock(hdev); > >>>>>> + > >>>>>> + list_for_each_entry(bis, &hdev->conn_hash.list, list) { > >>>>> This should check bis->type == BIS_LINK too. > >>>> Will do. > >>>>>> + if (test_and_clear_bit(HCI_CONN_BIG_SYNC, &bis->flags) && > >>>>>> + (bis->iso_qos.bcast.big == ev->handle)) { > >>>>>> + hci_disconn_cfm(bis, ev->reason); > >>>>>> + hci_conn_del(bis); > >>>>>> + > >>>>>> + /* Delete the big connection */ > >>>>>> + conn = hci_conn_hash_lookup_pa_sync_handle(hdev, bis->sync_handle); > >>>>>> + if (conn) > >>>>>> + hci_conn_del(conn); > >>>>> Problems: > >>>>> > >>>>> - use after free > >>>>> > >>>>> - hci_conn_del() cannot be used inside list_for_each_entry() > >>>>> of the connection list > >>>>> > >>>>> - also list_for_each_entry_safe() allows deleting only the iteration > >>>>> cursor, so some restructuring above is needed > >>>> Following your suggestion, I updated the hci_le_big_sync_lost_evt function. > >>>> > >>>> +static void hci_le_big_sync_lost_evt(struct hci_dev *hdev, void *data, > >>>> + struct sk_buff *skb) > >>>> +{ > >>>> + struct hci_evt_le_big_sync_lost *ev = data; > >>>> + struct hci_conn *bis, *conn, *n; > >>>> + > >>>> + bt_dev_dbg(hdev, "big handle 0x%2.2x", ev->handle); > >>>> + > >>>> + hci_dev_lock(hdev); > >>>> + > >>>> + /* Delete the pa sync connection */ > >>>> + bis = hci_conn_hash_lookup_pa_sync_big_handle(hdev, ev->handle); > >>>> + if (bis) { > >>>> + conn = hci_conn_hash_lookup_pa_sync_handle(hdev, > >>>> bis->sync_handle); > >>>> + if (conn) > >>>> + hci_conn_del(conn); > >>>> + } > >>>> + > >>>> + /* Delete each bis connection */ > >>>> + list_for_each_entry_safe(bis, n, &hdev->conn_hash.list, list) { > >>>> + if (bis->type == BIS_LINK && > >>>> + bis->iso_qos.bcast.big == ev->handle && > >>>> + test_and_clear_bit(HCI_CONN_BIG_SYNC, &bis->flags)) { > >>>> + hci_disconn_cfm(bis, ev->reason); > >>>> + hci_conn_del(bis); > >>>> + } > >>>> + } > >>> Id follow the logic in hci_le_create_big_complete_evt, so you do something like: > >>> > >>> while ((conn = hci_conn_hash_lookup_big_state(hdev, ev->handle, > >>> BT_CONNECTED)))... > >>> > >>> That way we don't operate on the list cursor, that said we may need to > >>> add the role as parameter to hci_conn_hash_lookup_big_state, because > >>> the BIG id domain is role specific so we can have clashes if there are > >>> Broadcast Sources using the same BIG id the above would return them as > >>> well and even if we check for the role inside the while loop will keep > >>> returning it forever. > >> I updated the patch according to your suggestion; however, during testing, it resulted in a system panic. > > What is the backtrace? > > > >> hci_conn_hash_lookup_big_state(struct hci_dev *hdev, __u8 handle, __u16 state) > >> > >> list_for_each_entry_rcu(c, &h->list, list) { > >> if (c->type != BIS_LINK || bacmp(&c->dst, BDADDR_ANY) || > >> + c->role != HCI_ROLE_SLAVE || > >> c->state != state) > >> continue; > > It needs to be passed as an argument not just change the role > > internally otherwise it will break the existing users of it. > > After testing, I found that the dst addr of the two BIS connections > under BIG sync is the address of the BIS source, so I added separate > checks for MASTER and SLAVE roles. > > [ 268.202466][1 T1962 d.] lookup big: 00000000736585c7, addr > 21:97:07:b1:9f:66, type 131, handle 0x0100, state 1, role 1 > [ 268.203806][1 T1962 d.] lookup big: 0000000041894659, addr > 21:97:07:b1:9f:66, type 131, handle 0x0101, state 1, role 1 > > I updated as below, > > -hci_conn_hash_lookup_big_state(struct hci_dev *hdev, __u8 handle, > __u16 state) > +hci_conn_hash_lookup_big_state(struct hci_dev *hdev, __u8 handle, > + __u16 state, __u8 role) > { > struct hci_conn_hash *h = &hdev->conn_hash; > struct hci_conn *c; > @@ -1335,10 +1336,18 @@ hci_conn_hash_lookup_big_state(struct hci_dev > *hdev, __u8 handle, __u16 state) > rcu_read_lock(); > > list_for_each_entry_rcu(c, &h->list, list) { > - if (c->type != BIS_LINK || bacmp(&c->dst, BDADDR_ANY) || > - c->role != HCI_ROLE_SLAVE || > - c->state != state) > - continue; > + if (role == HCI_ROLE_MASTER) { > + if (c->type != BIS_LINK || bacmp(&c->dst, > BDADDR_ANY) || > + c->state != state || c->role != role) > + continue; > + } else { > + if (c->type != BIS_LINK || > + c->state != state || > + c->role != role) > + continue; > + } > > > > >> +static void hci_le_big_sync_lost_evt(struct hci_dev *hdev, void *data, > >> + struct sk_buff *skb) > >> +{ > >> + struct hci_evt_le_big_sync_lost *ev = data; > >> + struct hci_conn *bis, *conn; > >> + > >> + bt_dev_dbg(hdev, "big handle 0x%2.2x", ev->handle); > >> + > >> + hci_dev_lock(hdev); > >> + > >> + /* Delete the pa sync connection */ > >> + bis = hci_conn_hash_lookup_pa_sync_big_handle(hdev, ev->handle); > >> + if (bis) { > >> + conn = hci_conn_hash_lookup_pa_sync_handle(hdev, bis->sync_handle); > >> + if (conn) > >> + hci_conn_del(conn); > >> + } > >> + > >> + /* Delete each bis connection */ > >> + while ((bis = hci_conn_hash_lookup_big_state(hdev, ev->handle, > >> + BT_CONNECTED))) { > >> + clear_bit(HCI_CONN_BIG_SYNC, &bis->flags); > >> + hci_disconn_cfm(bis, ev->reason); > >> + hci_conn_del(bis); > >> + } > >> + > >> + hci_dev_unlock(hdev); > >> +} > >> > >> However, during testing, I encountered some issues: > >> > >> 1. The current BIS connections all have the state BT_OPEN (2). > > Hmm, that doesn't sound right, if the BIG Sync has been completed the > > BIS connection shall be moved to BT_CONNECTED. Looks like we are not > > marking it as connected: > > > > hci_le_big_sync_established_evt (Broadcast Sink): > > > > set_bit(HCI_CONN_BIG_SYNC, &bis->flags); > > hci_iso_setup_path(bis); > > > > hci_le_create_big_complete_evt (Broadcast Source): > > > > conn->state = BT_CONNECTED; > > set_bit(HCI_CONN_BIG_CREATED, &conn->flags); > > hci_debugfs_create_conn(conn); > > hci_conn_add_sysfs(conn); > > hci_iso_setup_path(conn); > > Yes, in addition, state = BT_CONNECTED also needs to be set in > hci_cc_le_setup_iso_path. > > I will update the patch again. > > @@ -3890,7 +3890,7 @@ static u8 hci_cc_le_setup_iso_path(struct hci_dev > *hdev, void *data, > hci_conn_del(conn); > goto unlock; > } > - > + conn->state = BT_CONNECTED; Ive submitted a fix addressing this already: https://patchwork.kernel.org/project/bluetooth/patch/20250627151902.421666-1-luiz.dentz@xxxxxxxxx/ > >> [ 131.813237][1 T1967 d.] list conn 00000000fd2e0fb2, handle 0x0010, > >> state 1 #LE link > >> [ 131.813439][1 T1967 d.] list conn 00000000553bfedc, handle 0x0f01, > >> state 2 #PA link > >> [ 131.814301][1 T1967 d.] list conn 0000000074213ccb, handle 0x0100, > >> state 2 #bis1 link > >> [ 131.815167][1 T1967 d.] list conn 00000000ee6adb18, handle 0x0101, > >> state 2 #bis2 link > >> > >> 2. hci_conn_hash_lookup_big_state() fails to find the corresponding BIS > >> connection even when the state is set to OPEN. > >> > >> Therefore, I’m considering reverting to the original patch, but adding a > >> role check as an additional condition. > >> What do you think? > >> > >> + /* Delete each bis connection */ > >> + list_for_each_entry_safe(bis, n, &hdev->conn_hash.list, list) { > >> + if (bis->type == BIS_LINK && > >> + bis->role == HCI_ROLE_SLAVE && > >> + bis->iso_qos.bcast.big == ev->handle && > >> + test_and_clear_bit(HCI_CONN_BIG_SYNC, &bis->flags)) { > >> + hci_disconn_cfm(bis, ev->reason); > >> + hci_conn_del(bis); > >> + } > >> + } > >> > >>>> + > >>>> + hci_dev_unlock(hdev); > >>>> +} > >>>> > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> + hci_dev_unlock(hdev); > >>>>>> +} > >>>>>> + > >>>>>> static void hci_le_big_info_adv_report_evt(struct hci_dev *hdev, void *data, > >>>>>> struct sk_buff *skb) > >>>>>> { > >>>>>> @@ -7149,6 +7175,11 @@ static const struct hci_le_ev { > >>>>>> hci_le_big_sync_established_evt, > >>>>>> sizeof(struct hci_evt_le_big_sync_estabilished), > >>>>>> HCI_MAX_EVENT_SIZE), > >>>>>> + /* [0x1e = HCI_EVT_LE_BIG_SYNC_LOST] */ > >>>>>> + HCI_LE_EV_VL(HCI_EVT_LE_BIG_SYNC_LOST, > >>>>>> + hci_le_big_sync_lost_evt, > >>>>>> + sizeof(struct hci_evt_le_big_sync_lost), > >>>>>> + HCI_MAX_EVENT_SIZE), > >>>>>> /* [0x22 = HCI_EVT_LE_BIG_INFO_ADV_REPORT] */ > >>>>>> HCI_LE_EV_VL(HCI_EVT_LE_BIG_INFO_ADV_REPORT, > >>>>>> hci_le_big_info_adv_report_evt, > >>>>>> > >>>>>> --- > >>>>>> base-commit: bd35cd12d915bc410c721ba28afcada16f0ebd16 > >>>>>> change-id: 20250612-handle_big_sync_lost_event-4c7dc64390a2 > >>>>>> > >>>>>> Best regards, > >>>>> -- > >>>>> Pauli Virtanen > >>> > >>> -- > >>> Luiz Augusto von Dentz > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz
