d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

net80211: fix LOR/deadlock in ieee80211_ff_node_cleanup().

Browse Source 
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
This commit is contained in:
Andriy Voskoboinyk 2016-06-29 17:25:46 +00:00
parent d95fa04249
commit cdc0cf21eb
Notes: svn2git 2020-12-20 02:59:44 +00:00 
svn path=/head/; revision=302283
5 changed files with 73 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
sys/net80211/ieee80211_ddb.c
View File

@ -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);

19
sys/net80211/ieee80211_freebsd.h
View File

@ -82,6 +82,25 @@ typedef struct {
#define IEEE80211_TX_UNLOCK_ASSERT(_ic) \
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.
*/

67
sys/net80211/ieee80211_superg.c
View File

@ -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
KASSERT(sq->head != NULL, ("stageq empty"));
#endif
IEEE80211_FF_LOCK(ic);
if (sq->depth == 0) {
IEEE80211_FF_UNLOCK(ic);
return; /* nothing to do */
}
KASSERT(sq->head != NULL, ("stageq empty"));
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

44
sys/net80211/ieee80211_superg.h
View File

@ -107,40 +107,34 @@ 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)
{
struct ieee80211_superg *sg = ic->ic_superg;
if (sg != NULL && sg->ff_stageq[ac].depth)
ieee80211_ff_age(ic, &sg->ff_stageq[ac], 0x7fffffff);
}
/*
* See ieee80211_ff_age() for a description of the locking
* expectation here.
*/
static __inline void
ieee80211_ff_age_all(struct ieee80211com *ic, int quanta)
{
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);
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);
}
}
static __inline void
ieee80211_ff_flush(struct ieee80211com *ic, int ac)
{
struct ieee80211_superg *sg = ic->ic_superg;
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 *,
int, struct ieee80211_key *);
struct mbuf * ieee80211_amsdu_encap(struct ieee80211vap *vap, struct mbuf *m1,

1
sys/net80211/ieee80211_var.h
View File

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