On Mon, Jan 10, 2022 at 11:00:52AM +0100, Stefan Sperling wrote:
> Ping. I have had zero feedback on this so far. Anyone?
Makes sense, I remember that part of the code making problems before.
ok tobhe.
>
> On Tue, Jan 04, 2022 at 02:35:52PM +0100, Stefan Sperling wrote:
> > The function ieee80211_find_node_for_beacon() was added by reyk on 2005.
> > At the time, net80211 nodes were stored in a hash table keyed on hashes
> > the node's source MAC addresses.
> > The purpose of ieee80211_find_node_for_beacon() was to avoid storing
> > duplicate nodes with the same source MAC address in a given hash bucket.
> > Hash collisions on differing MAC addresses were valid, but collisions
> > on the same address had to be avoided.
> >
> > Later on, the node table data structure was changed from a hash table
> > to an RB tree which is still in use today. The RB tree can only store a
> > single node per MAC address. However, find_node_for_beacon() was kept
> > regardless, now documented to serve a different purpose.
> > Its new purpose is to tell apart different nodes which happen to use
> > the same MAC address and hence cannot both be stored in the RB tree.
> > The idea is to help the net80211 stack to pick the "better" one of two
> > such APs while it is scanning for access points. The comment also claims
> > this function would avoid "bloat" of the nodes tree, which is obviously
> > related to the function's original purpose.
> >
> > There are several problems with this:
> >
> > The code which decides which node is "better" can erroneously match
> > an AP against itself, in case the AP uses a hidden SSID.
> > This prevents state updates for such APs.
> > Just a bit further down, the code looks up the same node again
> > and performs all of the intended updates. Simply skipping the
> > ieee80211_find_node_for_beacon() check makes state updates work.
> > (I have received a patch submission which fixes probe responses
> > for iwm/iwx in order to interop with hidden SSID APs. And that
> > patch contains a workaround for this problem, which prompted me
> > to look into it.)
> >
> > The intention of avoiding "bloat" makes no sense.
> > There cannot be "bloat" of the node tree. Worst case we could end up
> > leaking memory when we replace an existing node in the tree with a
> > new node which has the same address (we would lose track of the pointer
> > to memory which stores the original node, and would never be able to
> > free this memory). There is no code in place which would prevent such
> > a leak. Simply always updating the existing node we have allocated is
> > the safer choice in the absence of proper collision handling.
> >
> > My opinion is that this function should simply be removed.
> > It is an incomplete attempt at dealing with an edge case (MAC address
> > collisions), which is not a problem this function alone could ever
> > hope to solve completely.
> >
> > ok?
> >
> > diff d32c3633e170510dd593f1e288e78acfddaff882
> > 32e6a77a0992dc0014e72c20005dfd9631b2055b
> > blob - bb6367fd1d5dd9f4641fedd96491069a8f0878dc
> > blob + 51d05f49c8b1ea58ea891f7b0f817e3bf94bed08
> > --- sys/net80211/ieee80211_input.c
> > +++ sys/net80211/ieee80211_input.c
> > @@ -1751,37 +1751,7 @@ ieee80211_recv_probe_resp(struct ieee80211com *ic,
> > str
> > ic->ic_stats.is_rx_chanmismatch++;
> > return;
> > }
> > - /*
> > - * Use mac, channel and rssi so we collect only the
> > - * best potential AP with the equal bssid while scanning.
> > - * Collecting all potential APs may result in bloat of
> > - * the node tree. This call will return NULL if the node
> > - * for this APs does not exist or if the new node is the
> > - * potential better one.
> > - */
> > - ni = ieee80211_find_node_for_beacon(ic, wh->i_addr2,
> > - &ic->ic_channels[chan], ssid, rxi->rxi_rssi);
> > - if (ni != NULL) {
> > - /*
> > - * If we are doing a directed scan for an AP with a hidden SSID
> > - * we must collect the SSID from a probe response to override
> > - * a non-zero-length SSID filled with zeroes that we may have
> > - * received earlier in a beacon.
> > - */
> > - if (isprobe && ssid[1] != 0 && ni->ni_essid[0] == '\0') {
> > - ni->ni_esslen = ssid[1];
> > - memset(ni->ni_essid, 0, sizeof(ni->ni_essid));
> > - /* we know that ssid[1] <= IEEE80211_NWID_LEN */
> > - memcpy(ni->ni_essid, &ssid[2], ssid[1]);
> > - }
> >
> > - /* Update channel in case AP has switched */
> > - if (ic->ic_opmode == IEEE80211_M_STA)
> > - ni->ni_chan = rni->ni_chan;
> > -
> > - return;
> > - }
> > -
> > #ifdef IEEE80211_DEBUG
> > if (ieee80211_debug > 1 &&
> > (ni == NULL || ic->ic_state == IEEE80211_S_SCAN ||
> > @@ -1977,6 +1947,13 @@ ieee80211_recv_probe_resp(struct ieee80211com *ic,
> > str
> > }
> > }
> >
> > + /*
> > + * Set our SSID if we do not know it yet.
> > + * If we are doing a directed scan for an AP with a hidden SSID
> > + * we must collect the SSID from a probe response to override
> > + * a non-zero-length SSID filled with zeroes that we may have
> > + * received earlier in a beacon.
> > + */
> > if (ssid[1] != 0 && ni->ni_essid[0] == '\0') {
> > ni->ni_esslen = ssid[1];
> > memset(ni->ni_essid, 0, sizeof(ni->ni_essid));
> > blob - 98cac0edbe5d0a070244293819042423b33e1e68
> > blob + 691dbc4f48afcdec88f15856c97fc3ebe123db70
> > --- sys/net80211/ieee80211_node.c
> > +++ sys/net80211/ieee80211_node.c
> > @@ -2025,30 +2025,6 @@ ieee80211_find_rxnode(struct ieee80211com *ic,
> > return ieee80211_ref_node(ni);
> > }
> >
> > -struct ieee80211_node *
> > -ieee80211_find_node_for_beacon(struct ieee80211com *ic,
> > - const u_int8_t *macaddr, const struct ieee80211_channel *chan,
> > - const char *ssid, u_int8_t rssi)
> > -{
> > - struct ieee80211_node *ni, *keep = NULL;
> > - int s, score = 0;
> > -
> > - if ((ni = ieee80211_find_node(ic, macaddr)) != NULL) {
> > - s = splnet();
> > -
> > - if (ni->ni_chan != chan && ni->ni_rssi >= rssi)
> > - score++;
> > - if (ssid[1] == 0 && ni->ni_esslen != 0)
> > - score++;
> > - if (score > 0)
> > - keep = ni;
> > -
> > - splx(s);
> > - }
> > -
> > - return (keep);
> > -}
> > -
> > void
> > ieee80211_node_tx_ba_clear(struct ieee80211_node *ni, int tid)
> > {
> > blob - ca7f9cd59bbc04c36318d64eaf8ef2036a07644e
> > blob + 4edf341acf6ae1ed4e6c3361bcc19a585dab636e
> > --- sys/net80211/ieee80211_node.h
> > +++ sys/net80211/ieee80211_node.h
> > @@ -524,10 +524,6 @@ struct ieee80211_node *ieee80211_find_rxnode(struct ie
> > const struct ieee80211_frame *);
> > struct ieee80211_node *ieee80211_find_txnode(struct ieee80211com *,
> > const u_int8_t *);
> > -struct ieee80211_node *
> > - ieee80211_find_node_for_beacon(struct ieee80211com *,
> > - const u_int8_t *, const struct ieee80211_channel *,
> > - const char *, u_int8_t);
> > void ieee80211_release_node(struct ieee80211com *,
> > struct ieee80211_node *);
> > void ieee80211_node_cleanup(struct ieee80211com *, struct ieee80211_node
> > *);
> >
> >
>
