From 544bb668add673bc8ab66877773cfa86067df1ea Mon Sep 17 00:00:00 2001 From: Rahul Tank Date: Wed, 16 Sep 2026 10:50:44 +0530 Subject: [PATCH] fix(nimble): stop GATTC temp-list crash on coex STAILQ_REMOVE on emptied temp_proc_list faulted during service discovery when BLE host preempted 802.15.4. Insert the proc on the active list before ATT TX and defer free while the public API still holds the pointer. --- nimble/host/src/ble_gattc.c | 204 ++++++++++++++++++++++-------------- 1 file changed, 126 insertions(+), 78 deletions(-) diff --git a/nimble/host/src/ble_gattc.c b/nimble/host/src/ble_gattc.c index dad08887f..ee20c7921 100644 --- a/nimble/host/src/ble_gattc.c +++ b/nimble/host/src/ble_gattc.c @@ -112,6 +112,14 @@ /** Procedure stalled due to resource exhaustion. */ #define BLE_GATTC_PROC_F_STALLED 0x01 +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) +/** Procedure is still referenced by its public API caller. */ +#define BLE_GATTC_PROC_F_API_LOCKED 0x02 + +/** Procedure free was deferred until its public API caller returns. */ +#define BLE_GATTC_PROC_F_FREE_PENDING 0x04 +#endif + /** Represents an in-progress GATT procedure. */ struct ble_gattc_proc { STAILQ_ENTRY(ble_gattc_proc) next; @@ -244,9 +252,6 @@ struct ble_gattc_proc { }; -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) -static struct ble_gattc_proc_list temp_proc_list; -#endif STAILQ_HEAD(ble_gattc_proc_list, ble_gattc_proc); /** @@ -872,6 +877,16 @@ ble_gattc_proc_free(struct ble_gattc_proc *proc) int i; if (proc != NULL) { +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) + ble_hs_lock_nested(); + if (proc->flags & BLE_GATTC_PROC_F_API_LOCKED) { + proc->flags |= BLE_GATTC_PROC_F_FREE_PENDING; + ble_hs_unlock_nested(); + return; + } + ble_hs_unlock_nested(); +#endif + ble_gattc_dbg_assert_proc_not_inserted(proc); switch (proc->op) { @@ -931,6 +946,35 @@ ble_gattc_proc_insert(struct ble_gattc_proc *proc) ble_hs_unlock(); } +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) +static int +ble_gattc_proc_remove_locked(struct ble_gattc_proc *proc) +{ + struct ble_gattc_proc *cur; + struct ble_gattc_proc *prev; + + BLE_HS_DBG_ASSERT(ble_hs_locked_by_cur_task()); + + prev = NULL; + cur = STAILQ_FIRST(&ble_gattc_procs); + while (cur != NULL) { + if (cur == proc) { + if (prev == NULL) { + STAILQ_REMOVE_HEAD(&ble_gattc_procs, next); + } else { + STAILQ_REMOVE_AFTER(&ble_gattc_procs, prev, next); + } + return 1; + } + + prev = cur; + cur = STAILQ_NEXT(cur, next); + } + + return 0; +} +#endif + static void ble_gattc_proc_set_exp_timer(struct ble_gattc_proc *proc) { @@ -938,6 +982,48 @@ ble_gattc_proc_set_exp_timer(struct ble_gattc_proc *proc) ble_npl_time_ms_to_ticks32(BLE_GATTC_UNRESPONSIVE_TIMEOUT_MS); } +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) +static void +ble_gattc_proc_preempt_insert(struct ble_gattc_proc *proc) +{ + if (!(proc->flags & BLE_GATTC_PROC_F_STALLED)) { + ble_gattc_proc_set_exp_timer(proc); + } + + ble_hs_lock(); + proc->flags |= BLE_GATTC_PROC_F_API_LOCKED; + STAILQ_INSERT_TAIL(&ble_gattc_procs, proc, next); + ble_hs_unlock(); + + ble_hs_timer_resched(); +} + +static void +ble_gattc_proc_preempt_complete(struct ble_gattc_proc *proc, int status) +{ + int free_proc; + + free_proc = 0; + + ble_hs_lock(); + if (status != 0) { + free_proc = ble_gattc_proc_remove_locked(proc); + } + + if (proc->flags & BLE_GATTC_PROC_F_FREE_PENDING) { + proc->flags &= ~BLE_GATTC_PROC_F_FREE_PENDING; + free_proc = 1; + } + + proc->flags &= ~BLE_GATTC_PROC_F_API_LOCKED; + ble_hs_unlock(); + + if (free_proc) { + ble_gattc_proc_free(proc); + } +} +#endif + #if MYNEWT_VAL(BLE_GATTC) static void ble_gattc_proc_set_resume_timer(struct ble_gattc_proc *proc) @@ -1182,13 +1268,11 @@ ble_gattc_extract(ble_gattc_match_fn *cb, void *arg, int max_procs, struct ble_gattc_proc *proc; struct ble_gattc_proc *prev; struct ble_gattc_proc *next; -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - struct ble_gattc_proc *cur; - uint8_t flag = 0; -#endif int num_extracted; - /* Only the parent task is allowed to remove entries from the list. */ + /* Parent task performs normal extraction. The preemption-protect TX + * failure path may also remove an active proc while holding ble_hs_lock. + */ BLE_HS_DBG_ASSERT(ble_hs_is_parent_task()); STAILQ_INIT(dst_list); @@ -1198,27 +1282,6 @@ ble_gattc_extract(ble_gattc_match_fn *cb, void *arg, int max_procs, prev = NULL; -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - proc = STAILQ_FIRST(&temp_proc_list); - while (proc != NULL) { - next = STAILQ_NEXT(proc, next); - STAILQ_FOREACH(cur, &ble_gattc_procs, next) { - if (proc == cur) { - flag = 1; - break; - } - } - if (!flag) { - /* Detected a preemption case */ - STAILQ_INSERT_TAIL(&ble_gattc_procs, proc, next); - } - flag = 0; - proc = next; - } - /* Clear the temp proc list */ - STAILQ_INIT(&temp_proc_list); -#endif - proc = STAILQ_FIRST(&ble_gattc_procs); while (proc != NULL) { next = STAILQ_NEXT(proc, next); @@ -1812,7 +1875,9 @@ ble_gattc_disc_all_svcs_tx(struct ble_gattc_proc *proc) ble_uuid16_t uuid = BLE_UUID16_INIT(BLE_ATT_UUID_PRIMARY_SERVICE); int rc; +#if !MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) ble_gattc_dbg_assert_proc_not_inserted(proc); +#endif rc = ble_att_clt_tx_read_group_type(proc->conn_handle, proc->cid, proc->disc_all_svcs.prev_handle + 1, @@ -1977,9 +2042,7 @@ ble_gattc_disc_all_svcs(uint16_t conn_handle, ble_gatt_disc_svc_fn *cb, proc->disc_all_svcs.cb_arg = cb_arg; #if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - ble_hs_lock(); - STAILQ_INSERT_TAIL(&temp_proc_list, proc, next); - ble_hs_unlock(); + ble_gattc_proc_preempt_insert(proc); #endif ble_gattc_log_proc_init("discover all services\n"); @@ -1989,21 +2052,17 @@ ble_gattc_disc_all_svcs(uint16_t conn_handle, ble_gatt_disc_svc_fn *cb, } done: - /* One ble_gattc_proc cannot be part of multiple linked lists. - * Hence it needs to be removed before going into ble_gattc_process_status. - */ -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - if (proc != NULL) { - ble_hs_lock(); - STAILQ_REMOVE(&temp_proc_list, proc, ble_gattc_proc, next); - ble_hs_unlock(); - } -#endif - if (rc != 0) { STATS_INC(ble_gattc_stats, disc_all_svcs_fail); } +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) + if (proc != NULL) { + ble_gattc_proc_preempt_complete(proc, rc); + return rc; + } +#endif + ble_gattc_process_status(proc, rc); return rc; } @@ -2645,7 +2704,9 @@ ble_gattc_disc_all_chrs_tx(struct ble_gattc_proc *proc) ble_uuid16_t uuid = BLE_UUID16_INIT(BLE_ATT_UUID_CHARACTERISTIC); int rc; +#if !MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) ble_gattc_dbg_assert_proc_not_inserted(proc); +#endif rc = ble_att_clt_tx_read_type(proc->conn_handle, proc->cid, proc->disc_all_chrs.prev_handle + 1, @@ -2809,12 +2870,10 @@ ble_gattc_disc_all_chrs(uint16_t conn_handle, uint16_t start_handle, proc->disc_all_chrs.cb = cb; proc->disc_all_chrs.cb_arg = cb_arg; -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - ble_hs_lock(); - STAILQ_INSERT_TAIL(&temp_proc_list, proc, next); - ble_hs_unlock(); -#endif ble_gattc_log_disc_all_chrs(proc); +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) + ble_gattc_proc_preempt_insert(proc); +#endif rc = ble_gattc_disc_all_chrs_tx(proc); if (rc != 0) { @@ -2822,21 +2881,17 @@ ble_gattc_disc_all_chrs(uint16_t conn_handle, uint16_t start_handle, } done: - /* One ble_gattc_proc cannot be part of multiple linked lists. - * Hence it needs to be removed before going into ble_gattc_process_status. - */ -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - if (proc != NULL) { - ble_hs_lock(); - STAILQ_REMOVE(&temp_proc_list, proc, ble_gattc_proc, next); - ble_hs_unlock(); - } -#endif - if (rc != 0) { STATS_INC(ble_gattc_stats, disc_all_chrs_fail); } +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) + if (proc != NULL) { + ble_gattc_proc_preempt_complete(proc, rc); + return rc; + } +#endif + ble_gattc_process_status(proc, rc); return rc; } @@ -3149,7 +3204,9 @@ ble_gattc_disc_all_dscs_tx(struct ble_gattc_proc *proc) { int rc; +#if !MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) ble_gattc_dbg_assert_proc_not_inserted(proc); +#endif rc = ble_att_clt_tx_find_info(proc->conn_handle, proc->cid, proc->disc_all_dscs.prev_handle + 1, @@ -3307,12 +3364,10 @@ ble_gattc_disc_all_dscs(uint16_t conn_handle, uint16_t start_handle, proc->disc_all_dscs.cb = cb; proc->disc_all_dscs.cb_arg = cb_arg; -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - ble_hs_lock(); - STAILQ_INSERT_TAIL(&temp_proc_list, proc, next); - ble_hs_unlock(); -#endif ble_gattc_log_disc_all_dscs(proc); +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) + ble_gattc_proc_preempt_insert(proc); +#endif rc = ble_gattc_disc_all_dscs_tx(proc); if (rc != 0) { @@ -3320,21 +3375,17 @@ ble_gattc_disc_all_dscs(uint16_t conn_handle, uint16_t start_handle, } done: - /* One ble_gattc_proc cannot be part of multiple linked lists. - * Hence it needs to be removed before going into ble_gattc_process_status. - */ -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - if (proc != NULL) { - ble_hs_lock(); - STAILQ_REMOVE(&temp_proc_list,proc,ble_gattc_proc, next); - ble_hs_unlock(); - } -#endif - if (rc != 0) { STATS_INC(ble_gattc_stats, disc_all_dscs_fail); } +#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) + if (proc != NULL) { + ble_gattc_proc_preempt_complete(proc, rc); + return rc; + } +#endif + ble_gattc_process_status(proc, rc); return rc; } @@ -6829,9 +6880,6 @@ ble_gattc_init(void) } #endif -#if MYNEWT_VAL(BLE_GATTC_PROC_PREEMPTION_PROTECT) - STAILQ_INIT(&temp_proc_list); -#endif STAILQ_INIT(&ble_gattc_procs); #if MYNEWT_VAL(BLE_GATTC_AUTO_PAIR) STAILQ_INIT(&ble_gattc_cached_procs);