Date: Wed, 29 Jun 2016 17:25:46 +0000 (UTC) From: Andriy Voskoboinyk <avos@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r302283 - head/sys/net80211 Message-ID: <201606291725.u5THPk0Y011747@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: avos Date: Wed Jun 29 17:25:46 2016 New Revision: 302283 URL: https://svnweb.freebsd.org/changeset/base/302283 Log: 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 Modified: head/sys/net80211/ieee80211_ddb.c head/sys/net80211/ieee80211_freebsd.h head/sys/net80211/ieee80211_superg.c head/sys/net80211/ieee80211_superg.h head/sys/net80211/ieee80211_var.h Modified: head/sys/net80211/ieee80211_ddb.c ============================================================================== --- head/sys/net80211/ieee80211_ddb.c Wed Jun 29 16:45:01 2016 (r302282) +++ head/sys/net80211/ieee80211_ddb.c Wed Jun 29 17:25:46 2016 (r302283) @@ -506,6 +506,8 @@ _db_show_com(const struct ieee80211com * 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); Modified: head/sys/net80211/ieee80211_freebsd.h ============================================================================== --- head/sys/net80211/ieee80211_freebsd.h Wed Jun 29 16:45:01 2016 (r302282) +++ head/sys/net80211/ieee80211_freebsd.h Wed Jun 29 17:25:46 2016 (r302283) @@ -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 { Modified: head/sys/net80211/ieee80211_superg.c ============================================================================== --- head/sys/net80211/ieee80211_superg.c Wed Jun 29 16:45:01 2016 (r302282) +++ head/sys/net80211/ieee80211_superg.c Wed Jun 29 17:25:46 2016 (r302283) @@ -99,6 +99,8 @@ ieee80211_superg_attach(struct ieee80211 { 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 ieee80211 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, s { 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 /* * 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 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 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, stru { 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, s { 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 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 * 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 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 */ 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->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 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 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 ieee802 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 ieee802 head = m; } } - IEEE80211_UNLOCK(ic); + IEEE80211_FF_UNLOCK(ic); /* * Free mbufs, taking care to not dereference the mbuf after Modified: head/sys/net80211/ieee80211_superg.h ============================================================================== --- head/sys/net80211/ieee80211_superg.h Wed Jun 29 16:45:01 2016 (r302282) +++ head/sys/net80211/ieee80211_superg.h Wed Jun 29 17:25:46 2016 (r302283) @@ -107,38 +107,32 @@ struct mbuf *ieee80211_ff_check(struct i 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 *, Modified: head/sys/net80211/ieee80211_var.h ============================================================================== --- head/sys/net80211/ieee80211_var.h Wed Jun 29 16:45:01 2016 (r302282) +++ head/sys/net80211/ieee80211_var.h Wed Jun 29 17:25:46 2016 (r302283) @@ -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 */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201606291725.u5THPk0Y011747>