Date: Mon, 13 Feb 2012 00:30:14 GMT From: dfilter@FreeBSD.ORG (dfilter service) To: freebsd-wireless@FreeBSD.org Subject: Re: kern/165060: commit references a PR Message-ID: <201202130030.q1D0UEKi052326@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/165060; it has been noted by GNATS. From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/165060: commit references a PR Date: Mon, 13 Feb 2012 00:28:55 +0000 (UTC) Author: adrian Date: Mon Feb 13 00:28:41 2012 New Revision: 231571 URL: http://svn.freebsd.org/changeset/base/231571 Log: Attempt to address some potential vap->iv_bss race conditions. There are unfortunately a number of situations where vap->iv_bss is changed or freed by some code in net80211. Because multiple threads can concurrently be doing work (and the vap->iv_bss access isn't at all done behind any kind of lock), it's quite possible that: * a change will occur in one thread - eg, by a call through ieee80211_sta_join1(); * a state change occurs in another thread - eg an RX is scheduled in the ath tasklet and it calls ieee80211_input_mimo_all(), which does dereference vap->iv_bss; * these two executing concurrently, causing things to explode. Another instance is ath_beacon_alloc() which takes an ieee80211_node *. It's called with the vap->iv_bss node from ath_newstate(). If the node has changed in the meantime (say it's been freed elsewhere) the reference that it grabbed _before_ refcounting it may be stale. I would _prefer_ that these sorts of things were serialised somewhere but that may be a bit much to ask. Instead, the best we can (currently) hope is that the underlying bss node is still (somewhat) valid. There is a related PR (kern/164382) described by the first case above. That should be fixed by properly serialising the RX path and reset path so an RX can't occur at the same time as the vap free/shutdown path. This is inspired by some related fixes in r212127. PR: kern/165060 Modified: head/sys/dev/ath/if_ath.c Modified: head/sys/dev/ath/if_ath.c ============================================================================== --- head/sys/dev/ath/if_ath.c Sun Feb 12 23:48:39 2012 (r231570) +++ head/sys/dev/ath/if_ath.c Mon Feb 13 00:28:41 2012 (r231571) @@ -1669,6 +1669,7 @@ ath_bmiss_vap(struct ieee80211vap *vap) struct ath_softc *sc = ifp->if_softc; u_int64_t lastrx = sc->sc_lastrx; u_int64_t tsf = ath_hal_gettsf64(sc->sc_ah); + /* XXX should take a locked ref to iv_bss */ u_int bmisstimeout = vap->iv_bmissthreshold * vap->iv_bss->ni_intval * 1024; @@ -3245,7 +3246,7 @@ ath_beacon_config(struct ath_softc *sc, if (vap == NULL) vap = TAILQ_FIRST(&ic->ic_vaps); /* XXX */ - ni = vap->iv_bss; + ni = ieee80211_ref_node(vap->iv_bss); /* extract tstamp from last beacon and convert to TU */ nexttbtt = TSF_TO_TU(LE_READ_4(ni->ni_tstamp.data + 4), @@ -3415,6 +3416,7 @@ ath_beacon_config(struct ath_softc *sc, ath_beacon_start_adhoc(sc, vap); } sc->sc_syncbeacon = 0; + ieee80211_free_node(ni); #undef FUDGE #undef TSF_TO_TU } @@ -3853,6 +3855,7 @@ ath_recv_mgmt(struct ieee80211_node *ni, switch (subtype) { case IEEE80211_FC0_SUBTYPE_BEACON: /* update rssi statistics for use by the hal */ + /* XXX unlocked check against vap->iv_bss? */ ATH_RSSI_LPF(sc->sc_halstats.ns_avgbrssi, rssi); if (sc->sc_syncbeacon && ni == vap->iv_bss && vap->iv_state == IEEE80211_S_RUN) { @@ -5721,7 +5724,7 @@ ath_newstate(struct ieee80211vap *vap, e taskqueue_unblock(sc->sc_tq); } - ni = vap->iv_bss; + ni = ieee80211_ref_node(vap->iv_bss); rfilt = ath_calcrxfilter(sc); stamode = (vap->iv_opmode == IEEE80211_M_STA || vap->iv_opmode == IEEE80211_M_AHDEMO || @@ -5752,7 +5755,8 @@ ath_newstate(struct ieee80211vap *vap, e if (nstate == IEEE80211_S_RUN) { /* NB: collect bss node again, it may have changed */ - ni = vap->iv_bss; + ieee80211_free_node(ni); + ni = ieee80211_ref_node(vap->iv_bss); DPRINTF(sc, ATH_DEBUG_STATE, "%s(RUN): iv_flags 0x%08x bintvl %d bssid %s " @@ -5875,6 +5879,7 @@ ath_newstate(struct ieee80211vap *vap, e #endif } bad: + ieee80211_free_node(ni); return error; } @@ -5893,6 +5898,7 @@ ath_setup_stationkey(struct ieee80211_no struct ath_softc *sc = vap->iv_ic->ic_ifp->if_softc; ieee80211_keyix keyix, rxkeyix; + /* XXX should take a locked ref to vap->iv_bss */ if (!ath_key_alloc(vap, &ni->ni_ucastkey, &keyix, &rxkeyix)) { /* * Key cache is full; we'll fall back to doing @@ -6448,6 +6454,7 @@ ath_tdma_config(struct ath_softc *sc, st return; } } + /* XXX should take a locked ref to iv_bss */ tp = vap->iv_bss->ni_txparms; /* * Calculate the guard time for each slot. This is the @@ -6697,6 +6704,7 @@ ath_tdma_beacon_send(struct ath_softc *s * Record local TSF for our last send for use * in arbitrating slot collisions. */ + /* XXX should take a locked ref to iv_bss */ vap->iv_bss->ni_tstamp.tsf = ath_hal_gettsf64(ah); } } _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201202130030.q1D0UEKi052326>