From 4310ac0bdf0fc984f7c4ea0e3bc86ba0be9f4780 Mon Sep 17 00:00:00 2001 From: "Bjoern A. Zeeb" Date: Tue, 2 Jun 2020 10:50:54 +0000 Subject: [PATCH 1/2] There are cases when node_alloc() and node_free() are called but the node_alloc_cd() has not run yet. This can lead to node_free() freeing the *ni before the node_alloc_cb() runs which then modifies memory (in this case the "is_in_peer_table") after free (in addition to inserting state which is wrong). To address this do multiple things: - change the is_in_peer_table from int to bool as that is what it is and use a macro and ath10k_dbg() to track state changes. This was mostly for debugging. - While here move athp_node_alloc_state into if_athp_main.c as it is not used anywhere outside the function. Helps understanding the code. - Store the allocated taskq entry in node_alloc() in "alloc_taskq_e" so we can check it during node_free() and cancel the callback if needed. - Rework parts of the athp_taskq to allow us to reliably cancel the entry: ad d a second list where we put the entries we are executing. Walking that list can be done lock-less. Add a athp_taskq_cancel() function which will either take the entry of the taskq or wait that no entries are run before returning. While there, remove extra () around the locking macros, remove an early (extra) on_queue = 1 in athp_taskq_queue() and fold some print lines into less vertical space. Fixes Issue #28. Sponsored by: Rubicon Communications, LLC (d/b/a "Netgate") --- otus/freebsd/src/sys/dev/athp/if_athp_core.h | 20 +++- otus/freebsd/src/sys/dev/athp/if_athp_debug.h | 1 + otus/freebsd/src/sys/dev/athp/if_athp_main.c | 51 ++++++++-- otus/freebsd/src/sys/dev/athp/if_athp_taskq.c | 93 +++++++++++++------ otus/freebsd/src/sys/dev/athp/if_athp_taskq.h | 2 + otus/freebsd/src/sys/dev/athp/if_athp_var.h | 9 -- 6 files changed, 128 insertions(+), 48 deletions(-) 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..101f08e4 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; From 45d9fe1e5f00e485ed5c14565546a5e52d162fea Mon Sep 17 00:00:00 2001 From: "Bjoern A. Zeeb" Date: Thu, 4 Jun 2020 11:06:03 +0000 Subject: [PATCH 2/2] Add missing '"' to make this compile. Sponsored by: Rubicon Communications, LLC (d/b/a "Netgate") --- otus/freebsd/src/sys/dev/athp/if_athp_core.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 101f08e4..4e5f5212 100644 --- a/otus/freebsd/src/sys/dev/athp/if_athp_core.h +++ b/otus/freebsd/src/sys/dev/athp/if_athp_core.h @@ -118,7 +118,7 @@ struct ath10k_sta { #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__, \ + "is_in_peer_table %d -> %d\n", __func__, __LINE__, \ (_p), (_p)->is_in_peer_table, (_t)); \ (_p)->is_in_peer_table = (_t); \ } while (0)