diff --git a/otus/freebsd/src/sys/dev/athp/if_athp_core.h b/otus/freebsd/src/sys/dev/athp/if_athp_core.h index dedd2ae3..4e5f5212 100644 --- a/otus/freebsd/src/sys/dev/athp/if_athp_core.h +++ b/otus/freebsd/src/sys/dev/athp/if_athp_core.h @@ -89,7 +89,17 @@ struct ath10k_sta { * This is currently done via the callback queue to avoid * sleeping whilst holding net80211 locks, so.. */ - int is_in_peer_table; + bool is_in_peer_table; + + /* + * There can be a race in that node_alloc() and node_free() + * are called before their respective call back functions. + * This can lead to memory modified after free. Store the + * taskq entry in node_alloc() and in node_free() check if + * we can cancel it (and if, do so). If we can cancel it, + * do not even bother to run the free callback function. + */ + struct athp_taskq_entry *alloc_taskq_e; /* the following are protected by ar->data_lock */ u32 changed; /* IEEE80211_RC_* */ @@ -105,6 +115,14 @@ struct ath10k_sta { #endif }; +#define ATH10K_STA_IS_IN_PEER_TABLE(_ar, _p, _t) \ + do { \ + ath10k_dbg(_ar, ATH10K_DBG_NODE, "%s:%d ni %p " \ + "is_in_peer_table %d -> %d\n", __func__, __LINE__, \ + (_p), (_p)->is_in_peer_table, (_t)); \ + (_p)->is_in_peer_table = (_t); \ + } while (0) + #define ATH10K_VDEV_SETUP_TIMEOUT_HZ (50) enum ath10k_beacon_state { diff --git a/otus/freebsd/src/sys/dev/athp/if_athp_debug.h b/otus/freebsd/src/sys/dev/athp/if_athp_debug.h index 276a2f7f..b5c06006 100644 --- a/otus/freebsd/src/sys/dev/athp/if_athp_debug.h +++ b/otus/freebsd/src/sys/dev/athp/if_athp_debug.h @@ -53,6 +53,7 @@ #define ATH10K_DBG_TASKQ 0x80000000 #define ATH10K_DBG_KEYCACHE 0x100000000ULL #define ATH10K_DBG_BEACON 0x200000000ULL +#define ATH10K_DBG_NODE 0x400000000ULL #define ATH10K_DBG_ANY 0xffffffff enum ath10k_pktlog_filter { diff --git a/otus/freebsd/src/sys/dev/athp/if_athp_main.c b/otus/freebsd/src/sys/dev/athp/if_athp_main.c index 755eaef4..5f8f3875 100644 --- a/otus/freebsd/src/sys/dev/athp/if_athp_main.c +++ b/otus/freebsd/src/sys/dev/athp/if_athp_main.c @@ -94,6 +94,15 @@ __FBSDID("$FreeBSD$"); MALLOC_DEFINE(M_ATHPDEV, "athpdev", "athp memory"); +struct athp_node_alloc_state { + struct ieee80211vap *vap; + struct ieee80211_node *ni; + uint32_t is_assoc; + uint32_t is_run; + uint32_t is_node_qos; + uint8_t peer_macaddr[ETH_ALEN]; +}; + /* * These are the net80211 facing implementation pieces. */ @@ -215,7 +224,7 @@ athp_raw_xmit(struct ieee80211_node *ni, struct mbuf *m0, } arsta = ATHP_NODE(ni); - if (arsta->is_in_peer_table == 0) { + if (!arsta->is_in_peer_table) { ath10k_warn(ar, "%s: node %6D not yet in peer table!\n", __func__, ni->ni_macaddr, ":"); athp_tx_exit(ar); @@ -421,7 +430,7 @@ athp_transmit(struct ieee80211com *ic, struct mbuf *m0) } arsta = ATHP_NODE(ni); - if (arsta->is_in_peer_table == 0) { + if (!arsta->is_in_peer_table) { ath10k_warn(ar, "%s: node %6D not yet in peer table!\n", __func__, ni->ni_macaddr, ":"); athp_tx_exit(ar); @@ -692,7 +701,7 @@ athp_vap_bss_update_queue(struct ath10k *ar, struct ieee80211vap *vap, ku->is_run = is_run; /* schedule */ - (void) athp_taskq_queue(ar, e, "athp_node_alloc_cb", + (void) athp_taskq_queue(ar, e, "athp_node_bss_update_cb", athp_node_bss_update_cb); return (0); @@ -784,7 +793,7 @@ athp_vap_newstate(struct ieee80211vap *vap, enum ieee80211_state nstate, int arg */ if (vap->iv_opmode == IEEE80211_M_STA) { ATHP_CONF_LOCK(ar); - ATHP_NODE(bss_ni)->is_in_peer_table = 1; + ATH10K_STA_IS_IN_PEER_TABLE(ar, ATHP_NODE(bss_ni), true); athp_bss_info_config(vap, bss_ni); ath10k_bss_update(ar, vap, bss_ni, 1, 1); ATHP_CONF_UNLOCK(ar); @@ -873,7 +882,7 @@ athp_vap_newstate(struct ieee80211vap *vap, enum ieee80211_state nstate, int arg /* This brings the interface down; delete the peer */ if (vif->is_stabss_setup == 1) { - ATHP_NODE(bss_ni)->is_in_peer_table = 0; + ATH10K_STA_IS_IN_PEER_TABLE(ar, ATHP_NODE(bss_ni), false); ath10k_bss_update(ar, vap, bss_ni, 0, 0); } ATHP_CONF_UNLOCK(ar); @@ -906,7 +915,7 @@ athp_vap_newstate(struct ieee80211vap *vap, enum ieee80211_state nstate, int arg break; } ath10k_bss_update(ar, vap, bss_ni, 1, 0); - ATHP_NODE(bss_ni)->is_in_peer_table = 1; + ATH10K_STA_IS_IN_PEER_TABLE(ar, ATHP_NODE(bss_ni), true); ATHP_CONF_UNLOCK(ar); } break; @@ -1692,7 +1701,13 @@ athp_node_alloc_cb(struct ath10k *ar, struct athp_taskq_entry *e, int flush) /* XXX TODO: set "node" xmit flag to 1 */ arsta = ATHP_NODE(ku->ni); - arsta->is_in_peer_table = 1; + ATH10K_STA_IS_IN_PEER_TABLE(ar, arsta, true); + + /* + * Clear the taskq state given we have finished. The "cancel" logic + * can do with stale pointers, so we would not have to do this. + */ + arsta->alloc_taskq_e = NULL; } static void @@ -1787,12 +1802,20 @@ athp_node_alloc(struct ieee80211vap *vap, */ ku->ni = (void *) an; + /* + * Save the taskq entry so we can cancel it. There + * are cases when node_alloc and node_free were run but + * neither *_cb() yet and running the alloc_cb after + * node_free will modify memory after free. + */ + an->alloc_taskq_e = e; + /* schedule */ (void) athp_taskq_queue(ar, e, "athp_node_alloc_cb", athp_node_alloc_cb); } } else { /* "our" node - we always have it for hostap mode */ - an->is_in_peer_table = 1; + ATH10K_STA_IS_IN_PEER_TABLE(ar, an, true); } return (&an->an_node); @@ -1894,7 +1917,7 @@ athp_node_free(struct ieee80211_node *ni) "%s: delete peer for MAC %6D\n", __func__, ni->ni_macaddr, ":"); - arsta->is_in_peer_table = 0; + ATH10K_STA_IS_IN_PEER_TABLE(ar, arsta, false); /* * Only do this for hostap mode. @@ -1911,6 +1934,16 @@ athp_node_free(struct ieee80211_node *ni) * queue is emptied here just to be sure. */ + /* + * Make sure the node_alloc cb has run or cancel it. + * Otherwise we are risking to modify memory after free. + * XXX-BZ double-check that nothing of + * athp_node_free_cb() needs to be done. + */ + if (arsta->alloc_taskq_e != NULL && + athp_taskq_cancel(ar, arsta->alloc_taskq_e)) + goto finish; + /* * Allocate a callback function state. */ diff --git a/otus/freebsd/src/sys/dev/athp/if_athp_taskq.c b/otus/freebsd/src/sys/dev/athp/if_athp_taskq.c index 01798963..a6a88c4b 100644 --- a/otus/freebsd/src/sys/dev/athp/if_athp_taskq.c +++ b/otus/freebsd/src/sys/dev/athp/if_athp_taskq.c @@ -100,55 +100,53 @@ MALLOC_DEFINE(M_ATHPDEV_TASKQ, "athp_taskq", "athp taskq"); * are submitted to it in order. */ -#define ATHP_TASKQ_LOCK(h) (mtx_lock(&(h)->m)) -#define ATHP_TASKQ_UNLOCK(h) (mtx_unlock(&(h)->m)) -#define ATHP_TASKQ_LOCK_ASSERT(h) (mtx_assert(&(h)->m, MA_OWNED)) -#define ATHP_TASKQ_UNLOCK_ASSERT(h) (mtx_assert(&(h)->m, MA_NOTOWNED)) +#define ATHP_TASKQ_LOCK(h) mtx_lock(&(h)->m) +#define ATHP_TASKQ_UNLOCK(h) mtx_unlock(&(h)->m) +#define ATHP_TASKQ_LOCK_ASSERT(h) mtx_assert(&(h)->m, MA_OWNED) +#define ATHP_TASKQ_UNLOCK_ASSERT(h) mtx_assert(&(h)->m, MA_NOTOWNED) static void athp_taskq_task(void *arg, int npending) { struct ath10k *ar = arg; - struct ieee80211com *ic = &ar->sc_ic; struct athp_taskq_entry *e; struct athp_taskq_head *h; - int n = 0; + int n; h = ar->sc_taskq_head; if (h == NULL) return; - ath10k_dbg(ar, ATH10K_DBG_TASKQ, "%s: called\n", __func__); + ath10k_dbg(ar, ATH10K_DBG_TASKQ, "%s: called pending %d\n", + __func__, npending); - /* - * Run through the queue up to 4 at a time, and - * run the callbacks. - */ + n = 0; ATHP_TASKQ_LOCK(h); + /* Walk the queue, 4 a time, and run the callbacks. */ while ((n < 4) && (e = TAILQ_FIRST(&h->list)) != NULL) { TAILQ_REMOVE(&h->list, e, node); e->on_queue = 0; - ATHP_TASKQ_UNLOCK(h); + TAILQ_INSERT_TAIL(&h->active_list, e, node); + n++; + } + ATHP_TASKQ_UNLOCK(h); + + while ((e = TAILQ_FIRST(&h->active_list)) != NULL) { ath10k_dbg(ar, ATH10K_DBG_TASKQ, "%s: calling cb %s %p (ptr %p)\n", - __func__, - e->cb_str, - e->cb, - e); + __func__, e->cb_str, e->cb, e); e->cb(ar, e, 1); + TAILQ_REMOVE(&h->active_list, e, node); athp_taskq_entry_free(ar, e); - n++; - ATHP_TASKQ_LOCK(h); } - /* Whilst locked, see if there's any more work to do */ + /* See if there's any more work to do. */ n = 0; - if (h->is_running && TAILQ_FIRST(&h->list) != NULL) { + ATHP_TASKQ_LOCK(h); + if (h->is_running && TAILQ_FIRST(&h->list) != NULL) n = 1; - } ATHP_TASKQ_UNLOCK(h); - if (n) - ieee80211_runtask(ic, &h->run_task); + ieee80211_runtask(&ar->sc_ic, &h->run_task); } int @@ -168,6 +166,7 @@ athp_taskq_init(struct ath10k *ar) TASK_INIT(&h->run_task, 0, athp_taskq_task, ar); TAILQ_INIT(&h->list); + TAILQ_INIT(&h->active_list); ar->sc_taskq_head = h; @@ -183,6 +182,8 @@ athp_taskq_free(struct ath10k *ar) if (h == NULL) return; + /* XXX FIXME Someone dequeue all the entries and free them. */ + ar->sc_taskq_head = NULL; mtx_destroy(&h->m); @@ -324,19 +325,15 @@ athp_taskq_queue(struct ath10k *ar, struct athp_taskq_entry *e, ath10k_dbg(ar, ATH10K_DBG_TASKQ, "%s: queuing cb %s %p (ptr %p)\n", - __func__, - str, - cb, - e); + __func__, str, cb, e); e->ar = ar; - e->on_queue = 1; e->cb = cb; e->cb_str = str; ATHP_TASKQ_LOCK(h); - e->on_queue = 1; TAILQ_INSERT_TAIL(&h->list, e, node); + e->on_queue = 1; if (h->is_running) do_run = 1; ATHP_TASKQ_UNLOCK(h); @@ -346,3 +343,41 @@ athp_taskq_queue(struct ath10k *ar, struct athp_taskq_entry *e, return (0); } + +bool +athp_taskq_cancel(struct ath10k *ar, struct athp_taskq_entry *e) +{ + struct athp_taskq_head *h; + struct athp_taskq_entry *le; + bool cancelled; + + if (e == NULL) + return (false); + + if (ar->sc_taskq_head== NULL) + return (false); + h = ar->sc_taskq_head; + + /* We do not check e->on_queue in case it was a stale pointer. */ + + cancelled = false; + ATHP_TASKQ_LOCK(h); + TAILQ_FOREACH(le, &h->list, node) { + if (le == e) { + TAILQ_REMOVE(&h->list, e, node); + e->on_queue = 0; + cancelled = true; + break; + } + } + ATHP_TASKQ_UNLOCK(h); + + /* Wait until the "active queue" is empty. */ + while (!cancelled && !TAILQ_EMPTY(&h->active_list)) + DELAY(10); + + if (cancelled) + athp_taskq_entry_free(ar, e); + + return (cancelled); +} diff --git a/otus/freebsd/src/sys/dev/athp/if_athp_taskq.h b/otus/freebsd/src/sys/dev/athp/if_athp_taskq.h index 91d00986..d1955067 100644 --- a/otus/freebsd/src/sys/dev/athp/if_athp_taskq.h +++ b/otus/freebsd/src/sys/dev/athp/if_athp_taskq.h @@ -18,6 +18,7 @@ struct athp_taskq_entry { struct athp_taskq_head { TAILQ_HEAD(, athp_taskq_entry) list; + TAILQ_HEAD(, athp_taskq_entry) active_list; int is_running; struct task run_task; struct mtx m; @@ -34,6 +35,7 @@ extern struct athp_taskq_entry * athp_taskq_entry_alloc(struct ath10k *, int); extern void athp_taskq_entry_free(struct ath10k *, struct athp_taskq_entry *); extern int athp_taskq_queue(struct ath10k *, struct athp_taskq_entry *, const char *str, athp_taskq_cmd_cb *cb); +extern bool athp_taskq_cancel(struct ath10k *, struct athp_taskq_entry *); static inline void * athp_taskq_entry_to_ptr(struct athp_taskq_entry *e) diff --git a/otus/freebsd/src/sys/dev/athp/if_athp_var.h b/otus/freebsd/src/sys/dev/athp/if_athp_var.h index ed148fd7..1e6b72dc 100644 --- a/otus/freebsd/src/sys/dev/athp/if_athp_var.h +++ b/otus/freebsd/src/sys/dev/athp/if_athp_var.h @@ -50,15 +50,6 @@ struct athp_key_update { uint32_t cipher; }; -struct athp_node_alloc_state { - struct ieee80211vap *vap; - struct ieee80211_node *ni; - uint32_t is_assoc; - uint32_t is_run; - uint32_t is_node_qos; - uint8_t peer_macaddr[ETH_ALEN]; -}; - struct athp_keyidx_update { struct ieee80211vap *vap; ieee80211_keyix keyidx;