net80211: fix LOR/deadlock in ieee80211_ff_node_cleanup().

Add new lock for stageq (part of ieee80211_superg structure) and
ni_tx_superg (part of ieee80211_node structure);
drop com_lock protection where it is used to protect them.

While here, drop duplicate OPACKETS counter incrementation.

ni_tx_ampdu is not protected with it (however, it is also used without
locking in other places; probably, it requires some other solution
to be thread-safe).

Tested with RTL8188CUS (AP) and RTL8188EU (STA).

NOTE: Since this change breaks KBI, all wireless drivers need to be
recompiled.

Reviewed by:	adrian
Approved by:	re (gjb)
Differential Revision:	https://reviews.freebsd.org/D6958

5 files changed, 69 insertions, 56 deletions

diff --git a/sys/net80211/ieee80211_ddb.c b/sys/net80211/ieee80211_ddb.c
index 94fcf93..2fd5067 100644
--- a/sys/net80211/ieee80211_ddb.c
+++ b/sys/net80211/ieee80211_ddb.c
@@ -506,6 +506,8 @@ _db_show_com(const struct ieee80211com *ic, int showvaps, int showsta,
 	db_printf("\tsoftc %p", ic->ic_softc);
 	db_printf("\tname %s", ic->ic_name);
 	db_printf(" comlock %p", &ic->ic_comlock);
+	db_printf(" txlock %p", &ic->ic_txlock);
+	db_printf(" fflock %p", &ic->ic_fflock);
 	db_printf("\n");
 	db_printf("\theadroom %d", ic->ic_headroom);
 	db_printf(" phytype %d", ic->ic_phytype);
diff --git a/sys/net80211/ieee80211_freebsd.h b/sys/net80211/ieee80211_freebsd.h
index 2cb3b43..438ae73 100644
--- a/sys/net80211/ieee80211_freebsd.h
+++ b/sys/net80211/ieee80211_freebsd.h
@@ -83,6 +83,25 @@ typedef struct {
 	mtx_assert(IEEE80211_TX_LOCK_OBJ(_ic), MA_NOTOWNED)
 
 /*
+ * Stageq / ni_tx_superg lock
+ */
+typedef struct {
+	char		name[16];		/* e.g. "ath0_ff_lock" */
+	struct mtx	mtx;
+} ieee80211_ff_lock_t;
+#define IEEE80211_FF_LOCK_INIT(_ic, _name) do {				\
+	ieee80211_ff_lock_t *fl = &(_ic)->ic_fflock;			\
+	snprintf(fl->name, sizeof(fl->name), "%s_ff_lock", _name);	\
+	mtx_init(&fl->mtx, fl->name, NULL, MTX_DEF);			\
+} while (0)
+#define IEEE80211_FF_LOCK_OBJ(_ic)	(&(_ic)->ic_fflock.mtx)
+#define IEEE80211_FF_LOCK_DESTROY(_ic)	mtx_destroy(IEEE80211_FF_LOCK_OBJ(_ic))
+#define IEEE80211_FF_LOCK(_ic)		mtx_lock(IEEE80211_FF_LOCK_OBJ(_ic))
+#define IEEE80211_FF_UNLOCK(_ic)	mtx_unlock(IEEE80211_FF_LOCK_OBJ(_ic))
+#define IEEE80211_FF_LOCK_ASSERT(_ic) \
+	mtx_assert(IEEE80211_FF_LOCK_OBJ(_ic), MA_OWNED)
+
+/*
  * Node locking definitions.
  */
 typedef struct {
diff --git a/sys/net80211/ieee80211_superg.c b/sys/net80211/ieee80211_superg.c
index d5d1c18..eb95ea3 100644
--- a/sys/net80211/ieee80211_superg.c
+++ b/sys/net80211/ieee80211_superg.c
@@ -99,6 +99,8 @@ ieee80211_superg_attach(struct ieee80211com *ic)
 {
 	struct ieee80211_superg *sg;
 
+	IEEE80211_FF_LOCK_INIT(ic, ic->ic_name);
+
 	sg = (struct ieee80211_superg *) IEEE80211_MALLOC(
 	     sizeof(struct ieee80211_superg), M_80211_VAP,
 	     IEEE80211_M_NOWAIT | IEEE80211_M_ZERO);
@@ -120,6 +122,8 @@ ieee80211_superg_attach(struct ieee80211com *ic)
 void
 ieee80211_superg_detach(struct ieee80211com *ic)
 {
+	IEEE80211_FF_LOCK_DESTROY(ic);
+
 	if (ic->ic_superg != NULL) {
 		IEEE80211_FREE(ic->ic_superg, M_80211_VAP);
 		ic->ic_superg = NULL;
@@ -575,19 +579,14 @@ ff_transmit(struct ieee80211_node *ni, struct mbuf *m)
 {
 	struct ieee80211vap *vap = ni->ni_vap;
 	struct ieee80211com *ic = ni->ni_ic;
-	int error;
 
-	IEEE80211_TX_LOCK_ASSERT(vap->iv_ic);
+	IEEE80211_TX_LOCK_ASSERT(ic);
 
 	/* encap and xmit */
 	m = ieee80211_encap(vap, ni, m);
-	if (m != NULL) {
-		struct ifnet *ifp = vap->iv_ifp;
-
-		error = ieee80211_parent_xmitpkt(ic, m);
-		if (!error)
-			if_inc_counter(ifp, IFCOUNTER_OPACKETS, 1);
-	} else
+	if (m != NULL)
+		(void) ieee80211_parent_xmitpkt(ic, m);
+	else
 		ieee80211_free_node(ni);
 }
 
@@ -620,14 +619,6 @@ ff_flush(struct mbuf *head, struct mbuf *last)
 
 /*
  * Age frames on the staging queue.
- *
- * This is called without the comlock held, but it does all its work
- * behind the comlock.  Because of this, it's possible that the
- * staging queue will be serviced between the function which called
- * it and now; thus simply checking that the queue has work in it
- * may fail.
- *
- * See PR kern/174283 for more details.
  */
 void
 ieee80211_ff_age(struct ieee80211com *ic, struct ieee80211_stageq *sq,
@@ -636,11 +627,14 @@ ieee80211_ff_age(struct ieee80211com *ic, struct ieee80211_stageq *sq,
 	struct mbuf *m, *head;
 	struct ieee80211_node *ni;
 
-#if 0
+	IEEE80211_FF_LOCK(ic);
+	if (sq->depth == 0) {
+		IEEE80211_FF_UNLOCK(ic);
+		return;		/* nothing to do */
+	}
+
 	KASSERT(sq->head != NULL, ("stageq empty"));
-#endif
 
-	IEEE80211_LOCK(ic);
 	head = sq->head;
 	while ((m = sq->head) != NULL && M_AGE_GET(m) < quanta) {
 		int tid = WME_AC_TO_TID(M_WME_GETAC(m));
@@ -657,7 +651,7 @@ ieee80211_ff_age(struct ieee80211com *ic, struct ieee80211_stageq *sq,
 		sq->tail = NULL;
 	else
 		M_AGE_SUB(m, quanta);
-	IEEE80211_UNLOCK(ic);
+	IEEE80211_FF_UNLOCK(ic);
 
 	IEEE80211_TX_LOCK(ic);
 	ff_flush(head, m);
@@ -669,7 +663,7 @@ stageq_add(struct ieee80211com *ic, struct ieee80211_stageq *sq, struct mbuf *m)
 {
 	int age = ieee80211_ffagemax;
 
-	IEEE80211_LOCK_ASSERT(ic);
+	IEEE80211_FF_LOCK_ASSERT(ic);
 
 	if (sq->tail != NULL) {
 		sq->tail->m_nextpkt = m;
@@ -688,7 +682,7 @@ stageq_remove(struct ieee80211com *ic, struct ieee80211_stageq *sq, struct mbuf
 {
 	struct mbuf *m, *mprev;
 
-	IEEE80211_LOCK_ASSERT(ic);
+	IEEE80211_FF_LOCK_ASSERT(ic);
 
 	mprev = NULL;
 	for (m = sq->head; m != NULL; m = m->m_nextpkt) {
@@ -767,6 +761,11 @@ ieee80211_ff_check(struct ieee80211_node *ni, struct mbuf *m)
 
 	IEEE80211_TX_UNLOCK_ASSERT(ic);
 
+	IEEE80211_LOCK(ic);
+	limit = IEEE80211_TXOP_TO_US(
+	    ic->ic_wme.wme_chanParams.cap_wmeParams[pri].wmep_txopLimit);
+	IEEE80211_UNLOCK(ic);
+
 	/*
 	 * Check if the supplied frame can be aggregated.
 	 *
@@ -774,7 +773,7 @@ ieee80211_ff_check(struct ieee80211_node *ni, struct mbuf *m)
 	 *     Do 802.1x EAPOL frames proceed in the clear? Then they couldn't
 	 *     be aggregated with other types of frames when encryption is on?
 	 */
-	IEEE80211_LOCK(ic);
+	IEEE80211_FF_LOCK(ic);
 	tap = &ni->ni_tx_ampdu[WME_AC_TO_TID(pri)];
 	mstaged = ni->ni_tx_superg[WME_AC_TO_TID(pri)];
 	/* XXX NOTE: reusing packet counter state from A-MPDU */
@@ -792,7 +791,7 @@ ieee80211_ff_check(struct ieee80211_node *ni, struct mbuf *m)
 	if (vap->iv_opmode != IEEE80211_M_STA &&
 	    ETHER_IS_MULTICAST(mtod(m, struct ether_header *)->ether_dhost)) {
 		/* XXX flush staged frame? */
-		IEEE80211_UNLOCK(ic);
+		IEEE80211_FF_UNLOCK(ic);
 		return m;
 	}
 	/*
@@ -801,15 +800,13 @@ ieee80211_ff_check(struct ieee80211_node *ni, struct mbuf *m)
 	 */
 	if (mstaged == NULL &&
 	    ieee80211_txampdu_getpps(tap) < ieee80211_ffppsmin) {
-		IEEE80211_UNLOCK(ic);
+		IEEE80211_FF_UNLOCK(ic);
 		return m;
 	}
 	sq = &sg->ff_stageq[pri];
 	/*
 	 * Check the txop limit to insure the aggregate fits.
 	 */
-	limit = IEEE80211_TXOP_TO_US(
-		ic->ic_wme.wme_chanParams.cap_wmeParams[pri].wmep_txopLimit);
 	if (limit != 0 &&
 	    (txtime = ff_approx_txtime(ni, m, mstaged)) > limit) {
 		/*
@@ -824,7 +821,7 @@ ieee80211_ff_check(struct ieee80211_node *ni, struct mbuf *m)
 		ni->ni_tx_superg[WME_AC_TO_TID(pri)] = NULL;
 		if (mstaged != NULL)
 			stageq_remove(ic, sq, mstaged);
-		IEEE80211_UNLOCK(ic);
+		IEEE80211_FF_UNLOCK(ic);
 
 		if (mstaged != NULL) {
 			IEEE80211_TX_LOCK(ic);
@@ -846,7 +843,7 @@ ieee80211_ff_check(struct ieee80211_node *ni, struct mbuf *m)
 	if (mstaged != NULL) {
 		ni->ni_tx_superg[WME_AC_TO_TID(pri)] = NULL;
 		stageq_remove(ic, sq, mstaged);
-		IEEE80211_UNLOCK(ic);
+		IEEE80211_FF_UNLOCK(ic);
 
 		IEEE80211_NOTE(vap, IEEE80211_MSG_SUPERG, ni,
 		    "%s: aggregate fast-frame", __func__);
@@ -862,13 +859,13 @@ ieee80211_ff_check(struct ieee80211_node *ni, struct mbuf *m)
 		mstaged->m_nextpkt = m;
 		mstaged->m_flags |= M_FF; /* NB: mark for encap work */
 	} else {
-		KASSERT(ni->ni_tx_superg[WME_AC_TO_TID(pri)]== NULL,
+		KASSERT(ni->ni_tx_superg[WME_AC_TO_TID(pri)] == NULL,
 		    ("ni_tx_superg[]: %p",
 		    ni->ni_tx_superg[WME_AC_TO_TID(pri)]));
 		ni->ni_tx_superg[WME_AC_TO_TID(pri)] = m;
 
 		stageq_add(ic, sq, m);
-		IEEE80211_UNLOCK(ic);
+		IEEE80211_FF_UNLOCK(ic);
 
 		IEEE80211_NOTE(vap, IEEE80211_MSG_SUPERG, ni,
 		    "%s: stage frame, %u queued", __func__, sq->depth);
@@ -926,7 +923,7 @@ ieee80211_ff_node_cleanup(struct ieee80211_node *ni)
 	struct mbuf *m, *next_m, *head;
 	int tid;
 
-	IEEE80211_LOCK(ic);
+	IEEE80211_FF_LOCK(ic);
 	head = NULL;
 	for (tid = 0; tid < WME_NUM_TID; tid++) {
 		int ac = TID_TO_WME_AC(tid);
@@ -945,7 +942,7 @@ ieee80211_ff_node_cleanup(struct ieee80211_node *ni)
 			head = m;
 		}
 	}
-	IEEE80211_UNLOCK(ic);
+	IEEE80211_FF_UNLOCK(ic);
 
 	/*
 	 * Free mbufs, taking care to not dereference the mbuf after
diff --git a/sys/net80211/ieee80211_superg.h b/sys/net80211/ieee80211_superg.h
index ced3c9b..2f8628c 100644
--- a/sys/net80211/ieee80211_superg.h
+++ b/sys/net80211/ieee80211_superg.h
@@ -107,38 +107,32 @@ struct mbuf *ieee80211_ff_check(struct ieee80211_node *, struct mbuf *);
 void	ieee80211_ff_age(struct ieee80211com *, struct ieee80211_stageq *,
 	     int quanta);
 
-/*
- * See ieee80211_ff_age() for a description of the locking
- * expectation here.
- */
 static __inline void
-ieee80211_ff_flush(struct ieee80211com *ic, int ac)
+ieee80211_ff_age_all(struct ieee80211com *ic, int quanta)
 {
 	struct ieee80211_superg *sg = ic->ic_superg;
 
-	if (sg != NULL && sg->ff_stageq[ac].depth)
-		ieee80211_ff_age(ic, &sg->ff_stageq[ac], 0x7fffffff);
+	if (sg != NULL) {
+		ieee80211_ff_age(ic, &sg->ff_stageq[WME_AC_VO], quanta);
+		ieee80211_ff_age(ic, &sg->ff_stageq[WME_AC_VI], quanta);
+		ieee80211_ff_age(ic, &sg->ff_stageq[WME_AC_BE], quanta);
+		ieee80211_ff_age(ic, &sg->ff_stageq[WME_AC_BK], quanta);
+	}
 }
 
-/*
- * See ieee80211_ff_age() for a description of the locking
- * expectation here.
- */
 static __inline void
-ieee80211_ff_age_all(struct ieee80211com *ic, int quanta)
+ieee80211_ff_flush(struct ieee80211com *ic, int ac)
 {
 	struct ieee80211_superg *sg = ic->ic_superg;
 
-	if (sg != NULL) {
-		if (sg->ff_stageq[WME_AC_VO].depth)
-			ieee80211_ff_age(ic, &sg->ff_stageq[WME_AC_VO], quanta);
-		if (sg->ff_stageq[WME_AC_VI].depth)
-			ieee80211_ff_age(ic, &sg->ff_stageq[WME_AC_VI], quanta);
-		if (sg->ff_stageq[WME_AC_BE].depth)
-			ieee80211_ff_age(ic, &sg->ff_stageq[WME_AC_BE], quanta);
-		if (sg->ff_stageq[WME_AC_BK].depth)
-			ieee80211_ff_age(ic, &sg->ff_stageq[WME_AC_BK], quanta);
-	}
+	if (sg != NULL)
+		ieee80211_ff_age(ic, &sg->ff_stageq[ac], 0x7fffffff);
+}
+
+static __inline void
+ieee80211_ff_flush_all(struct ieee80211com *ic)
+{
+	ieee80211_ff_age_all(ic, 0x7fffffff);
 }
 
 struct mbuf *ieee80211_ff_encap(struct ieee80211vap *, struct mbuf *,
diff --git a/sys/net80211/ieee80211_var.h b/sys/net80211/ieee80211_var.h
index 3c33f0a..5ca3f43 100644
--- a/sys/net80211/ieee80211_var.h
+++ b/sys/net80211/ieee80211_var.h
@@ -121,6 +121,7 @@ struct ieee80211com {
 	const char		*ic_name;	/* usually device name */
 	ieee80211_com_lock_t	ic_comlock;	/* state update lock */
 	ieee80211_tx_lock_t	ic_txlock;	/* ic/vap TX lock */
+	ieee80211_ff_lock_t	ic_fflock;	/* stageq/ni_tx_superg lock */
 	LIST_ENTRY(ieee80211com)   ic_next;	/* on global list */
 	TAILQ_HEAD(, ieee80211vap) ic_vaps;	/* list of vap instances */
 	int			ic_headroom;	/* driver tx headroom needs */