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. > +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); > [ 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
