Page MenuHomeFreeBSD

net80211: workaround a beacon setup crash race w/ ieee80211_getcapinfo()
Needs RevisionPublic

Authored by adrian on Wed, Mar 26, 3:50 AM.
Referenced Files
Unknown Object (File)
Thu, Apr 17, 2:46 AM
Unknown Object (File)
Wed, Apr 16, 9:52 PM
Unknown Object (File)
Tue, Apr 15, 4:05 AM
Unknown Object (File)
Mon, Apr 14, 9:05 AM
Unknown Object (File)
Wed, Apr 9, 6:43 AM
Unknown Object (File)
Wed, Apr 9, 6:43 AM
Unknown Object (File)
Sun, Apr 6, 8:44 AM
Unknown Object (File)
Sun, Apr 6, 6:30 AM

Details

Reviewers
bz
Group Reviewers
wireless
Summary

There's a race during beacon setup and VAP setup/interface up that
is easily triggered by configuring AP interfaces in rc.conf and letting
the rc system bring things up.

ic->ic_curchan has a channel (which is a legacy hold over anyway),
but ni->ni_chan is set to IEEE80211_CHAN_ANYC. So, the BSS stuff
is (surprise!) not quite setup by the time the newstate path is
run (newstate_cb -> ath_newstate into RUN state -> ath_beacon_alloc()
-> ieee80211_beacon_alloc() -> ieee80211_beacon_construct() ->
ieee80211_getcapinfo()) and things explode.

This is definitely the wrong place to solve this problem though,
and when it's actually solved, a KASSERT belongs here instead.

In the meantime, this will just set up an incomplete capinfo field
until the next beacon update, at which point there'll (hopefully!) be
a valid BSS ni->ni_chan.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 63132
Build 60016: arc lint + arc unit

Event Timeline

thj added inline comments.
sys/net80211/ieee80211_output.c
2678

Should there also be a check for IEEE80211_CHAN_ANY here? I don't know if it makes sense of empty capinfo to have SHSLOT set

bz requested changes to this revision.Sat, Apr 12, 12:49 AM
bz added a subscriber: bz.

Hmm See also, e.g., wpi_run():

/* XXX kernel panic workaround */
if (c == IEEE80211_CHAN_ANYC) {
        device_printf(sc->sc_dev, "%s: incomplete configuration\n",
            __func__);
        return EINVAL;
}

If you want to add the ANYC checks there they both must come with a comment as-to why that is.

I agree on the race problem -- plenty of them in STA land too.

This revision now requires changes to proceed.Sat, Apr 12, 12:49 AM

I believe we have a different underlying problem and this should not go in as it manifests elsewhere as well:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=286063

In D49514#1136551, @bz wrote:

I believe we have a different underlying problem and this should not go in as it manifests elsewhere as well:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=286063

Hmmmmmmmmmmmmm.

Yeah, I think there's some fun race(s) going on here between VAP setup and the initial newstate change.

Time to go add some tracing.. :-) Let's sit on this commit for a while longer.

sys/net80211/ieee80211_output.c
2678

Hm, good point, lemme go see what the deal is with setting / clearing F_SHSLOT in various operating modes.