Skip to content

Commit

Permalink
[net80211] remove node scan lock / generation number + fix few LORs
Browse files Browse the repository at this point in the history 
Drop scan generation number and node table scan lock - the only place
where ni_scangen is checked is in ieee80211_timeout_stations() (and it
is used to prevent duplicate checking of the same node); node scan lock
protects only this variable + node table scan generation number.

This will fix (at least) next LOR (hostap mode):

lock order reversal:
1st 0xc175f84c urtwm0_scan_loc (urtwm0_scan_loc) @ /usr/src/sys/modules/wlan/../../net80211/ieee80211_node.c:2019
2nd 0xc175e018 urtwm0_com_lock (urtwm0_com_lock) @ /usr/src/sys/modules/wlan/../../net80211/ieee80211_node.c:2693
stack backtrace:
#0 0xa070d1c5 at witness_debugger+0x75
#1 0xa070d0f6 at witness_checkorder+0xd46
#2 0xa0694cce at __mtx_lock_flags+0x9e
#3 0xb03ad9ef at ieee80211_node_leave+0x12f
#4 0xb03afd13 at ieee80211_timeout_stations+0x483
#5 0xb03aa1c2 at ieee80211_node_timeout+0x42
#6 0xa06c6fa1 at softclock_call_cc+0x1e1
#7 0xa06c7518 at softclock+0xc8
#8 0xa06789ae at intr_event_execute_handlers+0x8e
#9 0xa0678fa0 at ithread_loop+0x90
#10 0xa0675fbe at fork_exit+0x7e
#11 0xa08af910 at fork_trampoline+0x8

In addition to the above:

* switch to ieee80211_iterate_nodes();
* do not assert that node table lock is held, while calling node_age();
  that's not really needed (there are no resources, which can be protected
  by this lock) + this fixes LOR/deadlock between ieee80211_timeout_stations()
  and ieee80211_set_tim() (easy to reproduce in HOSTAP mode while
  sending something to an STA with enabled power management).

Tested:

* (avos) urtwn0, hostap mode
* (adrian) AR9380, STA mode
* (adrian) AR9380, AR9331, AR9580, hostap mode

Notes:

* This changes the net80211 internals, so you have to recompile all of it
  and the wifi drivers.

Submitted by:	avos
Approved by:	re (delphij)
Differential Revision:	https://reviews.freebsd.org/D6833
  • Loading branch information
adrian authored and adrian committed Jun 19, 2016
1 parent fa16c81 commit 09a0af3
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 184 deletions.
7 changes: 2 additions & 5 deletions sys/net80211/ieee80211_ddb.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,8 @@ _db_show_sta(const struct ieee80211_node *ni)
db_printf("\tvap %p wdsvap %p ic %p table %p\n",
ni->ni_vap, ni->ni_wdsvap, ni->ni_ic, ni->ni_table);
db_printf("\tflags=%b\n", ni->ni_flags, IEEE80211_NODE_BITS);
db_printf("\tscangen %u authmode %u ath_flags 0x%x ath_defkeyix %u\n",
ni->ni_scangen, ni->ni_authmode,
ni->ni_ath_flags, ni->ni_ath_defkeyix);
db_printf("\tauthmode %u ath_flags 0x%x ath_defkeyix %u\n",
ni->ni_authmode, ni->ni_ath_flags, ni->ni_ath_defkeyix);
db_printf("\tassocid 0x%x txpower %u vlan %u\n",
ni->ni_associd, ni->ni_txpower, ni->ni_vlan);
db_printf("\tjointime %d (%lu secs) challenge %p\n",
Expand Down Expand Up @@ -688,8 +687,6 @@ _db_show_node_table(const char *tag, const struct ieee80211_node_table *nt)
db_printf("%s%s@%p:\n", tag, nt->nt_name, nt);
db_printf("%s nodelock %p", tag, &nt->nt_nodelock);
db_printf(" inact_init %d", nt->nt_inact_init);
db_printf(" scanlock %p", &nt->nt_scanlock);
db_printf(" scangen %u\n", nt->nt_scangen);
db_printf("%s keyixmax %d keyixmap %p\n",
tag, nt->nt_keyixmax, nt->nt_keyixmap);
for (i = 0; i < nt->nt_keyixmax; i++) {
Expand Down
22 changes: 0 additions & 22 deletions sys/net80211/ieee80211_freebsd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,28 +106,6 @@ typedef struct {
#define IEEE80211_NODE_LOCK_ASSERT(_nt) \
mtx_assert(IEEE80211_NODE_LOCK_OBJ(_nt), MA_OWNED)

/*
* Node table iteration locking definitions; this protects the
* scan generation # used to iterate over the station table
* while grabbing+releasing the node lock.
*/
typedef struct {
char name[16]; /* e.g. "ath0_scan_lock" */
struct mtx mtx;
} ieee80211_scan_lock_t;
#define IEEE80211_NODE_ITERATE_LOCK_INIT(_nt, _name) do { \
ieee80211_scan_lock_t *sl = &(_nt)->nt_scanlock; \
snprintf(sl->name, sizeof(sl->name), "%s_scan_lock", _name); \
mtx_init(&sl->mtx, sl->name, NULL, MTX_DEF); \
} while (0)
#define IEEE80211_NODE_ITERATE_LOCK_OBJ(_nt) (&(_nt)->nt_scanlock.mtx)
#define IEEE80211_NODE_ITERATE_LOCK_DESTROY(_nt) \
mtx_destroy(IEEE80211_NODE_ITERATE_LOCK_OBJ(_nt))
#define IEEE80211_NODE_ITERATE_LOCK(_nt) \
mtx_lock(IEEE80211_NODE_ITERATE_LOCK_OBJ(_nt))
#define IEEE80211_NODE_ITERATE_UNLOCK(_nt) \
mtx_unlock(IEEE80211_NODE_ITERATE_LOCK_OBJ(_nt))

/*
* Power-save queue definitions.
*/
Expand Down
269 changes: 115 additions & 154 deletions sys/net80211/ieee80211_node.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1099,8 +1099,6 @@ node_age(struct ieee80211_node *ni)
{
struct ieee80211vap *vap = ni->ni_vap;

IEEE80211_NODE_LOCK_ASSERT(&vap->iv_ic->ic_sta);

/*
* Age frames on the power save queue.
*/
Expand Down Expand Up @@ -1922,10 +1920,8 @@ ieee80211_node_table_init(struct ieee80211com *ic,

nt->nt_ic = ic;
IEEE80211_NODE_LOCK_INIT(nt, ic->ic_name);
IEEE80211_NODE_ITERATE_LOCK_INIT(nt, ic->ic_name);
TAILQ_INIT(&nt->nt_node);
nt->nt_name = name;
nt->nt_scangen = 1;
nt->nt_inact_init = inact;
nt->nt_keyixmax = keyixmax;
if (nt->nt_keyixmax > 0) {
Expand Down Expand Up @@ -1994,159 +1990,137 @@ ieee80211_node_table_cleanup(struct ieee80211_node_table *nt)
IEEE80211_FREE(nt->nt_keyixmap, M_80211_NODE);
nt->nt_keyixmap = NULL;
}
IEEE80211_NODE_ITERATE_LOCK_DESTROY(nt);
IEEE80211_NODE_LOCK_DESTROY(nt);
}

/*
* Timeout inactive stations and do related housekeeping.
* Note that we cannot hold the node lock while sending a
* frame as this would lead to a LOR. Instead we use a
* generation number to mark nodes that we've scanned and
* drop the lock and restart a scan if we have to time out
* a node. Since we are single-threaded by virtue of
* controlling the inactivity timer we can be sure this will
* process each node only once.
*/
static void
ieee80211_timeout_stations(struct ieee80211com *ic)
timeout_stations(void *arg __unused, struct ieee80211_node *ni)
{
struct ieee80211_node_table *nt = &ic->ic_sta;
struct ieee80211vap *vap;
struct ieee80211_node *ni;
int gen = 0;
struct ieee80211com *ic = ni->ni_ic;
struct ieee80211vap *vap = ni->ni_vap;

IEEE80211_NODE_ITERATE_LOCK(nt);
gen = ++nt->nt_scangen;
restart:
IEEE80211_NODE_LOCK(nt);
TAILQ_FOREACH(ni, &nt->nt_node, ni_list) {
if (ni->ni_scangen == gen) /* previously handled */
continue;
ni->ni_scangen = gen;
/*
* Ignore entries for which have yet to receive an
* authentication frame. These are transient and
* will be reclaimed when the last reference to them
* goes away (when frame xmits complete).
*/
vap = ni->ni_vap;
/*
* Only process stations when in RUN state. This
* insures, for example, that we don't timeout an
* inactive station during CAC. Note that CSA state
* is actually handled in ieee80211_node_timeout as
* it applies to more than timeout processing.
*/
if (vap->iv_state != IEEE80211_S_RUN)
continue;
/* XXX can vap be NULL? */
if ((vap->iv_opmode == IEEE80211_M_HOSTAP ||
vap->iv_opmode == IEEE80211_M_STA) &&
(ni->ni_flags & IEEE80211_NODE_AREF) == 0)
continue;
/*
* Only process stations when in RUN state. This
* insures, for example, that we don't timeout an
* inactive station during CAC. Note that CSA state
* is actually handled in ieee80211_node_timeout as
* it applies to more than timeout processing.
*/
if (vap->iv_state != IEEE80211_S_RUN)
return;
/*
* Ignore entries for which have yet to receive an
* authentication frame. These are transient and
* will be reclaimed when the last reference to them
* goes away (when frame xmits complete).
*/
if ((vap->iv_opmode == IEEE80211_M_HOSTAP ||
vap->iv_opmode == IEEE80211_M_STA) &&
(ni->ni_flags & IEEE80211_NODE_AREF) == 0)
return;
/*
* Free fragment if not needed anymore
* (last fragment older than 1s).
* XXX doesn't belong here, move to node_age
*/
if (ni->ni_rxfrag[0] != NULL &&
ticks > ni->ni_rxfragstamp + hz) {
m_freem(ni->ni_rxfrag[0]);
ni->ni_rxfrag[0] = NULL;
}
if (ni->ni_inact > 0) {
ni->ni_inact--;
IEEE80211_NOTE(vap, IEEE80211_MSG_INACT, ni,
"%s: inact %u inact_reload %u nrates %u",
__func__, ni->ni_inact, ni->ni_inact_reload,
ni->ni_rates.rs_nrates);
}
/*
* Special case ourself; we may be idle for extended periods
* of time and regardless reclaiming our state is wrong.
* XXX run ic_node_age
*/
/* XXX before inact decrement? */
if (ni == vap->iv_bss)
return;
if (ni->ni_associd != 0 ||
(vap->iv_opmode == IEEE80211_M_IBSS ||
vap->iv_opmode == IEEE80211_M_AHDEMO)) {
/*
* Free fragment if not needed anymore
* (last fragment older than 1s).
* XXX doesn't belong here, move to node_age
* Age/drain resources held by the station.
*/
if (ni->ni_rxfrag[0] != NULL &&
ticks > ni->ni_rxfragstamp + hz) {
m_freem(ni->ni_rxfrag[0]);
ni->ni_rxfrag[0] = NULL;
}
if (ni->ni_inact > 0) {
ni->ni_inact--;
IEEE80211_NOTE(vap, IEEE80211_MSG_INACT, ni,
"%s: inact %u inact_reload %u nrates %u",
__func__, ni->ni_inact, ni->ni_inact_reload,
ni->ni_rates.rs_nrates);
}
ic->ic_node_age(ni);
/*
* Special case ourself; we may be idle for extended periods
* of time and regardless reclaiming our state is wrong.
* XXX run ic_node_age
* Probe the station before time it out. We
* send a null data frame which may not be
* universally supported by drivers (need it
* for ps-poll support so it should be...).
*
* XXX don't probe the station unless we've
* received a frame from them (and have
* some idea of the rates they are capable
* of); this will get fixed more properly
* soon with better handling of the rate set.
*/
if (ni == vap->iv_bss)
continue;
if (ni->ni_associd != 0 ||
(vap->iv_opmode == IEEE80211_M_IBSS ||
vap->iv_opmode == IEEE80211_M_AHDEMO)) {
/*
* Age/drain resources held by the station.
*/
ic->ic_node_age(ni);
/*
* Probe the station before time it out. We
* send a null data frame which may not be
* universally supported by drivers (need it
* for ps-poll support so it should be...).
*
* XXX don't probe the station unless we've
* received a frame from them (and have
* some idea of the rates they are capable
* of); this will get fixed more properly
* soon with better handling of the rate set.
*/
if ((vap->iv_flags_ext & IEEE80211_FEXT_INACT) &&
(0 < ni->ni_inact &&
ni->ni_inact <= vap->iv_inact_probe) &&
ni->ni_rates.rs_nrates != 0) {
IEEE80211_NOTE(vap,
IEEE80211_MSG_INACT | IEEE80211_MSG_NODE,
ni, "%s",
"probe station due to inactivity");
/*
* Grab a reference before unlocking the table
* so the node cannot be reclaimed before we
* send the frame. ieee80211_send_nulldata
* understands we've done this and reclaims the
* ref for us as needed.
*/
ieee80211_ref_node(ni);
IEEE80211_NODE_UNLOCK(nt);
ieee80211_send_nulldata(ni);
/* XXX stat? */
goto restart;
}
}
if ((vap->iv_flags_ext & IEEE80211_FEXT_INACT) &&
ni->ni_inact <= 0) {
(0 < ni->ni_inact &&
ni->ni_inact <= vap->iv_inact_probe) &&
ni->ni_rates.rs_nrates != 0) {
IEEE80211_NOTE(vap,
IEEE80211_MSG_INACT | IEEE80211_MSG_NODE, ni,
"station timed out due to inactivity "
"(refcnt %u)", ieee80211_node_refcnt(ni));
IEEE80211_MSG_INACT | IEEE80211_MSG_NODE,
ni, "%s",
"probe station due to inactivity");
/*
* Send a deauthenticate frame and drop the station.
* This is somewhat complicated due to reference counts
* and locking. At this point a station will typically
* have a reference count of 1. ieee80211_node_leave
* will do a "free" of the node which will drop the
* reference count. But in the meantime a reference
* wil be held by the deauth frame. The actual reclaim
* of the node will happen either after the tx is
* completed or by ieee80211_node_leave.
*
* Separately we must drop the node lock before sending
* in case the driver takes a lock, as this can result
* in a LOR between the node lock and the driver lock.
* Grab a reference so the node cannot
* be reclaimed before we send the frame.
* ieee80211_send_nulldata understands
* we've done this and reclaims the
* ref for us as needed.
*/
/* XXX fix this (not required anymore). */
ieee80211_ref_node(ni);
IEEE80211_NODE_UNLOCK(nt);
if (ni->ni_associd != 0) {
IEEE80211_SEND_MGMT(ni,
IEEE80211_FC0_SUBTYPE_DEAUTH,
IEEE80211_REASON_AUTH_EXPIRE);
}
ieee80211_node_leave(ni);
ieee80211_free_node(ni);
vap->iv_stats.is_node_timeout++;
goto restart;
/* XXX useless */
ieee80211_send_nulldata(ni);
/* XXX stat? */
return;
}
}
IEEE80211_NODE_UNLOCK(nt);
if ((vap->iv_flags_ext & IEEE80211_FEXT_INACT) &&
ni->ni_inact <= 0) {
IEEE80211_NOTE(vap,
IEEE80211_MSG_INACT | IEEE80211_MSG_NODE, ni,
"station timed out due to inactivity "
"(refcnt %u)", ieee80211_node_refcnt(ni));
/*
* Send a deauthenticate frame and drop the station.
* This is somewhat complicated due to reference counts
* and locking. At this point a station will typically
* have a reference count of 2. ieee80211_node_leave
* will do a "free" of the node which will drop the
* reference count. But in the meantime a reference
* wil be held by the deauth frame. The actual reclaim
* of the node will happen either after the tx is
* completed or by ieee80211_node_leave.
*/
if (ni->ni_associd != 0) {
IEEE80211_SEND_MGMT(ni,
IEEE80211_FC0_SUBTYPE_DEAUTH,
IEEE80211_REASON_AUTH_EXPIRE);
}
ieee80211_node_leave(ni);
vap->iv_stats.is_node_timeout++;
}
}

/*
* Timeout inactive stations and do related housekeeping.
*/
static void
ieee80211_timeout_stations(struct ieee80211com *ic)
{
struct ieee80211_node_table *nt = &ic->ic_sta;

IEEE80211_NODE_ITERATE_UNLOCK(nt);
ieee80211_iterate_nodes(nt, timeout_stations, NULL);
}

/*
Expand Down Expand Up @@ -2247,23 +2221,12 @@ int
ieee80211_iterate_nt(struct ieee80211_node_table *nt,
struct ieee80211_node **ni_arr, uint16_t max_aid)
{
u_int gen;
int i, j, ret;
struct ieee80211_node *ni;

IEEE80211_NODE_ITERATE_LOCK(nt);
IEEE80211_NODE_LOCK(nt);

gen = ++nt->nt_scangen;
i = ret = 0;

/*
* We simply assume here that since the node
* scan generation doesn't change (as
* we are holding both the node table and
* node table iteration locks), we can simply
* assign it to the node here.
*/
TAILQ_FOREACH(ni, &nt->nt_node, ni_list) {
if (i >= max_aid) {
ret = E2BIG;
Expand All @@ -2272,7 +2235,6 @@ ieee80211_iterate_nt(struct ieee80211_node_table *nt,
break;
}
ni_arr[i] = ieee80211_ref_node(ni);
ni_arr[i]->ni_scangen = gen;
i++;
}

Expand All @@ -2287,7 +2249,6 @@ ieee80211_iterate_nt(struct ieee80211_node_table *nt,
* ieee80211_free_node().
*/
IEEE80211_NODE_UNLOCK(nt);
IEEE80211_NODE_ITERATE_UNLOCK(nt);

/*
* If ret is non-zero, we hit some kind of error.
Expand Down Expand Up @@ -2362,8 +2323,8 @@ ieee80211_dump_node(struct ieee80211_node_table *nt, struct ieee80211_node *ni)
{
printf("0x%p: mac %s refcnt %d\n", ni,
ether_sprintf(ni->ni_macaddr), ieee80211_node_refcnt(ni));
printf("\tscangen %u authmode %u flags 0x%x\n",
ni->ni_scangen, ni->ni_authmode, ni->ni_flags);
printf("\tauthmode %u flags 0x%x\n",
ni->ni_authmode, ni->ni_flags);
printf("\tassocid 0x%x txpower %u vlan %u\n",
ni->ni_associd, ni->ni_txpower, ni->ni_vlan);
printf("\ttxseq %u rxseq %u fragno %u rxfragstamp %u\n",
Expand Down
Loading

0 comments on commit 09a0af3

Please sign in to comment.