Ping. I have had zero feedback on this so far. Anyone?
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 *);
>
>
