On Sun, Aug 25, 2019 at 03:18:46PM +0200, Stefan Sperling wrote:
> 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
>
i ran this on iwn and athn, both in client mode. saw no issues.
jmc
> 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 *);
> >
> >
> >
>
