PANIC - SWBMISS (9.0-CURRENT)

[Adrian Chadd]

Hm, how are the interfaces configured? You're doing wds, I wonder how
the heck that works. :)

There's a locking problem with how software beacon miss is handled:

* sta_beacon_miss doesn't grab any lock, so I think its possible that
the swbmiss callout is being run on another CPU whilst
sta_becaon_miss() stops the callout and changes the state to S_ASSOC
or S_SCAN
* sta_newstate() grabs the ic lock, then it changes the state
(vap->iv_state = nstate) before it cancels the callout. Again, the
problem here is that the swbmiss callout may be scheduled during this
and there's no locking in sta_beacon_miss.

What should the solution be?

* should sta_beacon_miss (and the tdma one too?) grab the ic lock? i
think so, but I'd have to audit all the functions that it calls to
ensure it can be called with the ic lock held. In fact, I did a 30
second audit and it can't just hold the ic lock - as the calls to
ieeee80211_new_state() need the ic lock to be not held as it grabs the
lock itself. So we can't hold the lock for the whole function.
* should ieee80211_swbmiss() be called with the ic lock held? I think
so. That way if something external changes state, it should first grab
the lock, change the state, then cancel the swbmiss timer.

What I think should happen is that the beacon miss handler should be
called with the ic lock held. That way a state change can't occur
whilst its processing. That's going to take a bit of auditing though.

So here, try this patch; it may make things worse, it may make things
slightly better. I'm not sure. It just tries to eliminate a couple of
the race conditions that I found and makes sure ieee80211_swbmiss is
called with the ic lock held. I haven't tried to fix the more general
problem though.



Adrian

[adrian at pcbsd-2547] /data/freebsd/mips/if_ath_tx/src/sys/net80211> svn
diff ieee80211_sta.c ieee80211_tdma.c ieee80211_proto.c
Index: ieee80211_sta.c
===================================================================
--- ieee80211_sta.c     (revision 225723)
+++ ieee80211_sta.c     (working copy)
@@ -145,7 +145,9 @@
                return;
        }

+       IEEE80211_LOCK(ic);
        callout_stop(&vap->iv_swbmiss);
+       IEEE80211_UNLOCK(ic);
        vap->iv_bmiss_count = 0;
        vap->iv_stats.is_beacon_miss++;
        if (vap->iv_roaming == IEEE80211_ROAMING_AUTO) {
Index: ieee80211_tdma.c
===================================================================
--- ieee80211_tdma.c    (revision 225610)
+++ ieee80211_tdma.c    (working copy)
@@ -295,7 +295,9 @@
                "beacon miss, mode %u state %s\n",
                vap->iv_opmode, ieee80211_state_name[vap->iv_state]);

+       IEEE80211_LOCK(ic);
        callout_stop(&vap->iv_swbmiss);
+       IEEE80211_UNLOCK(ic);

        if (ts->tdma_peer != NULL) {    /* XXX? can this be null? */
                ieee80211_notify_node_leave(vap->iv_bss);
Index: ieee80211_proto.c
===================================================================
--- ieee80211_proto.c   (revision 225610)
+++ ieee80211_proto.c   (working copy)
@@ -193,7 +193,8 @@
        vap->iv_rtsthreshold = IEEE80211_RTS_DEFAULT;
        vap->iv_fragthreshold = IEEE80211_FRAG_DEFAULT;
        vap->iv_bmiss_max = IEEE80211_BMISS_MAX;
-       callout_init(&vap->iv_swbmiss, CALLOUT_MPSAFE);
+       callout_init_mtx(&vap->iv_swbmiss, IEEE80211_LOCK_OBJ(ic),
+           CALLOUT_MPSAFE);
        callout_init(&vap->iv_mgtsend, CALLOUT_MPSAFE);
        TASK_INIT(&vap->iv_nstate_task, 0, ieee80211_newstate_cb, vap);
        TASK_INIT(&vap->iv_swbmiss_task, 0, beacon_swmiss, vap);
@@ -1448,6 +1449,8 @@
        struct ieee80211vap *vap = arg;
        struct ieee80211com *ic = vap->iv_ic;

+       IEEE80211_LOCK_ASSERT(ic);
+
        /* XXX sleep state? */
        KASSERT(vap->iv_state == IEEE80211_S_RUN,
            ("wrong state %d", vap->iv_state));