memory modified after free / taskq node_alloc/free{_cb}

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 {

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 {

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.
 			 */

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);
+}

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)

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;