This has been very stable for me on an iwm(4) client device.
Has anyone tested this in other contexts?
Could somebody please test this in hostap mode?
Thanks,
Stefan
On Sat, Aug 17, 2019 at 12:01:24AM +0200, Stefan Sperling wrote:
> When a scan begins we currently toss away everything we have
> learned about access points in the previous scan iteration.
> This behaviour gets in the way of some things.
>
> For instance, I am working on another diff to show reasons for association
> failures in ifconfig output ("wrong channel", "wrong WPA key", "wrong BSSID").
> Because the scan loop keeps deleting existing nodes it gets in the way of
> such features. Whenever ifconfig wants to read information about an AP we
> have failed to associate to the AP's node has been freed already, and has
> perhaps been re-allocated upon reception of a new beacon.
>
> There are several ways in which nodes will still get freed with this diff:
>
> 1) This diff adds a new way of timing out inactive nodes which don't
> send a beacon within 10 scan iterations. This should get rid of stale
> APs if we're scanning for some time in a changing environment (should
> cover laptops walking around in buildings looking for APs to connect to).
>
> 2) If we fail to associate a few times, the corresponding node is removed.
> This mechanism already exists in -current code and is not changed here.
> See how ni_fails is handled in ieee80211_node_choose_bss().
>
> 3) If net80211 transitions back to INIT state (e.g. because of a
> user-initiated configuration change) all nodes are removed.
>
> 4) When a background scan starts all nodes will be removed.
> This could be revisited later. I have left it as-is for now.
> Background scan only occurs in RUN state and would be unwise to mix
> such changes into this diff, which aims to fix SCAN state.
>
>
> This isn't a new idea.
> I can recall conversations about this with various people.
>
> My question is whether this diff breaks anything for anyone.
>
>
> diff refs/heads/master refs/heads/keepnodes
> blob - 461f33f2561841520e61dd3de64a857413d1b224
> blob + 4634efcc61bcc44bea8a44abb4590cd6df303384
> --- sys/dev/ic/bwfm.c
> +++ sys/dev/ic/bwfm.c
> @@ -2610,7 +2610,7 @@ bwfm_newstate(struct ieee80211com *ic, enum ieee80211_
> return 0;
> }
> ieee80211_set_link_state(ic, LINK_STATE_DOWN);
> - ieee80211_free_allnodes(ic, 1);
> + ieee80211_node_cleanup(ic, ic->ic_bss);
> ic->ic_state = nstate;
> splx(s);
> return 0;
> blob - 0eb9dc07a0a75583f80579cc2d4c285dd1dc36b2
> blob + 0caa61779fed20f6e96a337a7ec0278efaaa72b5
> --- sys/dev/ic/pgt.c
> +++ sys/dev/ic/pgt.c
> @@ -170,7 +170,7 @@ void node_mark_active_ap(void *, struct
> ieee80211_nod
> void node_mark_active_adhoc(void *, struct ieee80211_node *);
> void pgt_watchdog(struct ifnet *);
> int pgt_init(struct ifnet *);
> -void pgt_update_hw_from_sw(struct pgt_softc *, int, int);
> +void pgt_update_hw_from_sw(struct pgt_softc *, int);
> void pgt_hostap_handle_mlme(struct pgt_softc *, uint32_t,
> struct pgt_obj_mlme *);
> void pgt_update_sw_from_hw(struct pgt_softc *,
> @@ -544,8 +544,7 @@ trying_again:
> sc->sc_flags &= ~flag;
> if (ic->ic_if.if_flags & IFF_RUNNING)
> pgt_update_hw_from_sw(sc,
> - ic->ic_state != IEEE80211_S_INIT,
> - ic->ic_opmode != IEEE80211_M_MONITOR);
> + ic->ic_state != IEEE80211_S_INIT);
> }
>
> ic->ic_if.if_flags &= ~IFF_RUNNING;
> @@ -2015,7 +2014,7 @@ pgt_media_change(struct ifnet *ifp)
>
> error = ieee80211_media_change(ifp);
> if (error == ENETRESET) {
> - pgt_update_hw_from_sw(sc, 0, 0);
> + pgt_update_hw_from_sw(sc, 0);
> error = 0;
> }
>
> @@ -2367,7 +2366,7 @@ pgt_ioctl(struct ifnet *ifp, u_long cmd, caddr_t req)
> }
>
> if (error == ENETRESET) {
> - pgt_update_hw_from_sw(sc, 0, 0);
> + pgt_update_hw_from_sw(sc, 0);
> error = 0;
> }
> splx(s);
> @@ -2501,8 +2500,7 @@ pgt_init(struct ifnet *ifp)
>
> if (!(sc->sc_flags & (SC_DYING | SC_UNINITIALIZED)))
> pgt_update_hw_from_sw(sc,
> - ic->ic_state != IEEE80211_S_INIT,
> - ic->ic_opmode != IEEE80211_M_MONITOR);
> + ic->ic_state != IEEE80211_S_INIT);
>
> ifp->if_flags |= IFF_RUNNING;
> ifq_clr_oactive(&ifp->if_snd);
> @@ -2522,7 +2520,7 @@ pgt_init(struct ifnet *ifp)
> * back to the BSS had before.
> */
> void
> -pgt_update_hw_from_sw(struct pgt_softc *sc, int keepassoc, int keepnodes)
> +pgt_update_hw_from_sw(struct pgt_softc *sc, int keepassoc)
> {
> struct ieee80211com *ic = &sc->sc_ic;
> struct arpcom *ac = &ic->ic_ac;
> @@ -2769,8 +2767,6 @@ badopmode:
> splx(s);
>
> if (success) {
> - if (shouldbeup && keepnodes)
> - sc->sc_flags |= SC_NOFREE_ALLNODES;
> if (shouldbeup)
> ieee80211_new_state(ic, IEEE80211_S_SCAN, -1);
> else
> @@ -2942,11 +2938,7 @@ pgt_newstate(struct ieee80211com *ic, enum ieee80211_s
> case IEEE80211_S_SCAN:
> ic->ic_if.if_timer = 1;
> ic->ic_mgt_timer = 0;
> - if (sc->sc_flags & SC_NOFREE_ALLNODES)
> - sc->sc_flags &= ~SC_NOFREE_ALLNODES;
> - else
> - ieee80211_free_allnodes(ic, 1);
> -
> + ieee80211_node_cleanup(ic, ic->ic_bss);
> ieee80211_set_link_state(ic, LINK_STATE_DOWN);
> #ifndef IEEE80211_STA_ONLY
> /* Just use any old channel; we override it anyway. */
> @@ -3262,7 +3254,7 @@ pgt_activate(struct device *self, int act)
> case DVACT_SUSPEND:
> if (ifp->if_flags & IFF_RUNNING) {
> pgt_stop(sc, SC_NEEDS_RESET);
> - pgt_update_hw_from_sw(sc, 0, 0);
> + pgt_update_hw_from_sw(sc, 0);
> }
> if (sc->sc_power != NULL)
> (*sc->sc_power)(sc, act);
> @@ -3283,10 +3275,10 @@ pgt_wakeup(struct pgt_softc *sc)
> (*sc->sc_power)(sc, DVACT_RESUME);
>
> pgt_stop(sc, SC_NEEDS_RESET);
> - pgt_update_hw_from_sw(sc, 0, 0);
> + pgt_update_hw_from_sw(sc, 0);
>
> if (ifp->if_flags & IFF_UP) {
> pgt_init(ifp);
> - pgt_update_hw_from_sw(sc, 0, 0);
> + pgt_update_hw_from_sw(sc, 0);
> }
> }
> blob - 038e6a63dfff113b525bb1e9a30c935996535569
> blob + 83b42976022d4ed42a9165720139abe5c1508324
> --- sys/dev/pci/if_iwm.c
> +++ sys/dev/pci/if_iwm.c
> @@ -5701,7 +5701,7 @@ iwm_scan(struct iwm_softc *sc)
> ieee80211_state_name[IEEE80211_S_SCAN]);
> if ((sc->sc_flags & IWM_FLAG_BGSCAN) == 0) {
> ieee80211_set_link_state(ic, LINK_STATE_DOWN);
> - ieee80211_free_allnodes(ic, 1);
> + ieee80211_node_cleanup(ic, ic->ic_bss);
> }
> ic->ic_state = IEEE80211_S_SCAN;
> iwm_led_blink_start(sc);
> blob - be1f18695ea4d1c2dedcd7e1630b97f3f72a7cd6
> blob + ef5f81699e90f2d4c4202b25880c9120907e5ec3
> --- sys/dev/pci/if_iwn.c
> +++ sys/dev/pci/if_iwn.c
> @@ -1825,7 +1825,7 @@ iwn_newstate(struct ieee80211com *ic, enum ieee80211_s
> ieee80211_state_name[nstate]);
> if ((sc->sc_flags & IWN_FLAG_BGSCAN) == 0) {
> ieee80211_set_link_state(ic, LINK_STATE_DOWN);
> - ieee80211_free_allnodes(ic, 1);
> + ieee80211_node_cleanup(ic, ic->ic_bss);
> }
> ic->ic_state = nstate;
> return 0;
> blob - b67bcfd77b19bceb5ee8b7ceffc1bfe7aab92598
> blob + 3075074c587ff95f69675e252c4b58f3aaf63e37
> --- sys/dev/pci/if_wpi.c
> +++ sys/dev/pci/if_wpi.c
> @@ -1058,7 +1058,7 @@ wpi_newstate(struct ieee80211com *ic, enum ieee80211_s
> ieee80211_state_name[ic->ic_state],
> ieee80211_state_name[nstate]);
> ieee80211_set_link_state(ic, LINK_STATE_DOWN);
> - ieee80211_free_allnodes(ic, 1);
> + ieee80211_node_cleanup(ic, ic->ic_bss);
> ic->ic_state = nstate;
> return 0;
>
> blob - a2476c29743019bca8b9183c32be0971f992b178
> blob + e8064dda864afdda3814234882fd125c430799ce
> --- sys/dev/usb/if_atu.c
> +++ sys/dev/usb/if_atu.c
> @@ -1210,7 +1210,7 @@ atu_newstate(struct ieee80211com *ic, enum ieee80211_s
> case IEEE80211_S_SCAN:
> memcpy(ic->ic_chan_scan, ic->ic_chan_active,
> sizeof(ic->ic_chan_active));
> - ieee80211_free_allnodes(ic, 1);
> + ieee80211_node_cleanup(ic, ic->ic_bss);
>
> /* tell the event thread that we want a scan */
> sc->sc_cmd = ATU_C_SCAN;
> blob - ec3e0f61e963ea403c9d8b88e675fbe42d5aeaeb
> blob + 905396f216158f9497fc28be19737e61595adce3
> --- sys/net80211/ieee80211_node.c
> +++ sys/net80211/ieee80211_node.c
> @@ -74,7 +74,6 @@ void ieee80211_setup_node(struct ieee80211com *, struc
> const u_int8_t *);
> void ieee80211_free_node(struct ieee80211com *, struct ieee80211_node *);
> struct ieee80211_node *ieee80211_alloc_node_helper(struct ieee80211com *);
> -void ieee80211_node_cleanup(struct ieee80211com *, struct ieee80211_node *);
> void ieee80211_node_switch_bss(struct ieee80211com *, struct ieee80211_node
> *);
> void ieee80211_node_addba_request(struct ieee80211_node *, int);
> void ieee80211_node_addba_request_ac_be_to(void *);
> @@ -92,6 +91,7 @@ void ieee80211_node_leave_11g(struct ieee80211com *, s
> void ieee80211_inact_timeout(void *);
> void ieee80211_node_cache_timeout(void *);
> #endif
> +void ieee80211_clean_inactive_nodes(struct ieee80211com *, int);
>
> #ifndef IEEE80211_STA_ONLY
> void
> @@ -780,6 +780,19 @@ ieee80211_reset_scan(struct ifnet *ifp)
> }
>
> /*
> + * Increase a node's inactitivy counter.
> + * This counter get reset to zero if a frame is received.
> + * This function is intended for station mode only.
> + * See ieee80211_node_cache_timeout() for hostap mode.
> + */
> +void
> +ieee80211_node_raise_inact(void *arg, struct ieee80211_node *ni)
> +{
> + if (ni->ni_refcnt == 0 && ni->ni_inact < IEEE80211_INACT_SCAN)
> + ni->ni_inact++;
> +}
> +
> +/*
> * Begin an active scan.
> */
> void
> @@ -807,14 +820,12 @@ ieee80211_begin_scan(struct ifnet *ifp)
> (ic->ic_flags & IEEE80211_F_ASCAN) ?
> "active" : "passive");
>
> - /*
> - * Flush any previously seen AP's. Note that the latter
> - * assumes we don't act as both an AP and a station,
> - * otherwise we'll potentially flush state of stations
> - * associated with us.
> - */
> - ieee80211_free_allnodes(ic, 1);
>
> + if (ic->ic_opmode == IEEE80211_M_STA) {
> + ieee80211_node_cleanup(ic, ic->ic_bss);
> + ieee80211_iterate_nodes(ic, ieee80211_node_raise_inact, NULL);
> + }
> +
> /*
> * Reset the current mode. Setting the current mode will also
> * reset scan state.
> @@ -1297,6 +1308,9 @@ ieee80211_end_scan(struct ifnet *ifp)
> if (ic->ic_scan_count)
> ic->ic_flags &= ~IEEE80211_F_ASCAN;
>
> + if (ic->ic_opmode == IEEE80211_M_STA)
> + ieee80211_clean_inactive_nodes(ic, IEEE80211_INACT_SCAN);
> +
> ni = RBT_MIN(ieee80211_tree, &ic->ic_tree);
>
> #ifndef IEEE80211_STA_ONLY
> @@ -2130,6 +2144,29 @@ ieee80211_clean_nodes(struct ieee80211com *ic, int cac
> "possible nodes leak\n", ifp->if_xname, nnodes,
> ic->ic_nnodes);
> #endif
> + splx(s);
> +}
> +
> +void
> +ieee80211_clean_inactive_nodes(struct ieee80211com *ic, int inact_max)
> +{
> + struct ieee80211_node *ni, *next_ni;
> + u_int gen = ic->ic_scangen++; /* NB: ok 'cuz single-threaded*/
> + int s;
> +
> + s = splnet();
> + for (ni = RBT_MIN(ieee80211_tree, &ic->ic_tree);
> + ni != NULL; ni = next_ni) {
> + next_ni = RBT_NEXT(ieee80211_tree, ni);
> + if (ni->ni_scangen == gen) /* previously handled */
> + continue;
> + ni->ni_scangen = gen;
> + if (ni->ni_refcnt > 0 || ni->ni_inact < inact_max)
> + continue;
> + ieee80211_free_node(ic, ni);
> + ic->ic_stats.is_node_timeout++;
> + }
> +
> splx(s);
> }
>
> blob - af6ad346c608189e4689050262a477a7a2e75cd5
> blob + 84c5ecabaf666eb20d0935b0d96cdd3af88084c4
> --- sys/net80211/ieee80211_node.h
> +++ sys/net80211/ieee80211_node.h
> @@ -41,6 +41,7 @@
> #define IEEE80211_INACT_MAX (300/IEEE80211_INACT_WAIT)
> #define IEEE80211_CACHE_SIZE 100
> #define IEEE80211_CACHE_WAIT 30
> +#define IEEE80211_INACT_SCAN 10 /* for station mode */
>
> struct ieee80211_rateset {
> u_int8_t rs_nrates;
> @@ -491,6 +492,7 @@ struct ieee80211_node *
> const char *, u_int8_t);
> void ieee80211_release_node(struct ieee80211com *,
> struct ieee80211_node *);
> +void ieee80211_node_cleanup(struct ieee80211com *, struct ieee80211_node *);
> void ieee80211_free_allnodes(struct ieee80211com *, int);
> void ieee80211_iterate_nodes(struct ieee80211com *,
> ieee80211_iter_func *, void *);
>
>
>
