On Fri, Jul 13, 2012 at 08:16:56PM +0200, Stefan Sperling wrote:
> The ieee80211_dup_bss() function internally calls
> ieee80211_alloc_node_helper(), which means we should check
> the node cache for an existing entry before calling ieee80211_dup_bss().
>
> ieee80211_node.c already does this but there are some instances in
> ieee80211_input.c where we fail to check for existing cache entries first.
> This can leak nodes and distort our idea of how many nodes are in the cache.
>
> The diff below also prints a warning if the interface is in debug
> mode and a potential node leak is detected during the node cache
> cleanup timeout, which runs once per hour. This might make spotting
> such bugs easier in the future.
Additional tweaks:
Update the ic_nnodes counter, which tracks the number of nodes in
the RB tree, near the RB_INSERT call in ieee80211_setup_node().
The corresponding decrement is already located near the RB_FREE.
Assert that we're at IPL_NET when decrementing the counter (there
are currently no calls to this function outside of IPL_NET but
this makes the requirement explicit).
Index: ieee80211_input.c
===================================================================
RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
retrieving revision 1.120
diff -u -p -r1.120 ieee80211_input.c
--- ieee80211_input.c 13 Jul 2012 11:25:04 -0000 1.120
+++ ieee80211_input.c 13 Jul 2012 17:47:15 -0000
@@ -401,7 +401,9 @@ ieee80211_input(struct ifnet *ifp, struc
DPRINTF(("data from unknown src %s\n",
ether_sprintf(wh->i_addr2)));
/* NB: caller deals with reference */
- ni = ieee80211_dup_bss(ic, wh->i_addr2);
+ ni = ieee80211_find_node(ic, wh->i_addr2);
+ if (ni == NULL)
+ ni = ieee80211_dup_bss(ic, wh->i_addr2);
if (ni != NULL) {
IEEE80211_SEND_MGMT(ic, ni,
IEEE80211_FC0_SUBTYPE_DEAUTH,
@@ -1725,7 +1727,9 @@ ieee80211_recv_probe_req(struct ieee8021
}
if (ni == ic->ic_bss) {
- ni = ieee80211_dup_bss(ic, wh->i_addr2);
+ ni = ieee80211_find_node(ic, wh->i_addr2);
+ if (ni == NULL)
+ ni = ieee80211_dup_bss(ic, wh->i_addr2);
if (ni == NULL)
return;
DPRINTF(("new probe req from %s\n",
@@ -1905,7 +1909,9 @@ ieee80211_recv_assoc_req(struct ieee8021
DPRINTF(("deny %sassoc from %s, not authenticated\n",
reassoc ? "re" : "",
ether_sprintf((u_int8_t *)wh->i_addr2)));
- ni = ieee80211_dup_bss(ic, wh->i_addr2);
+ ni = ieee80211_find_node(ic, wh->i_addr2);
+ if (ni == NULL)
+ ni = ieee80211_dup_bss(ic, wh->i_addr2);
if (ni != NULL) {
IEEE80211_SEND_MGMT(ic, ni,
IEEE80211_FC0_SUBTYPE_DEAUTH,
Index: ieee80211_node.c
===================================================================
RCS file: /cvs/src/sys/net80211/ieee80211_node.c,v
retrieving revision 1.69
diff -u -p -r1.69 ieee80211_node.c
--- ieee80211_node.c 13 Jul 2012 09:46:33 -0000 1.69
+++ ieee80211_node.c 13 Jul 2012 18:23:25 -0000
@@ -189,8 +189,6 @@ ieee80211_alloc_node_helper(struct ieee8
if (ic->ic_nnodes >= ic->ic_max_nnodes)
return NULL;
ni = (*ic->ic_node_alloc)(ic);
- if (ni != NULL)
- ic->ic_nnodes++;
return ni;
}
@@ -815,6 +813,7 @@ ieee80211_setup_node(struct ieee80211com
#endif
s = splnet();
RB_INSERT(ieee80211_tree, &ic->ic_tree, ni);
+ ic->ic_nnodes++;
splx(s);
}
@@ -1081,6 +1080,8 @@ ieee80211_free_node(struct ieee80211com
if (ni == ic->ic_bss)
panic("freeing bss node");
+ splassert(IPL_NET);
+
DPRINTF(("%s\n", ether_sprintf(ni->ni_macaddr)));
#ifndef IEEE80211_STA_ONLY
timeout_del(&ni->ni_eapol_to);
@@ -1150,6 +1151,10 @@ ieee80211_clean_nodes(struct ieee80211co
struct ieee80211_node *ni, *next_ni;
u_int gen = ic->ic_scangen++; /* NB: ok 'cuz single-threaded*/
int s;
+#ifndef IEEE80211_STA_ONLY
+ int nnodes = 0;
+ struct ifnet *ifp = &ic->ic_if;
+#endif
s = splnet();
for (ni = RB_MIN(ieee80211_tree, &ic->ic_tree);
@@ -1159,6 +1164,9 @@ ieee80211_clean_nodes(struct ieee80211co
break;
if (ni->ni_scangen == gen) /* previously handled */
continue;
+#ifndef IEEE80211_STA_ONLY
+ nnodes++;
+#endif
ni->ni_scangen = gen;
if (ni->ni_refcnt > 0)
continue;
@@ -1195,6 +1203,7 @@ ieee80211_clean_nodes(struct ieee80211co
* the driver calls ieee80211_release_node().
*/
#ifndef IEEE80211_STA_ONLY
+ nnodes--;
if (ic->ic_opmode == IEEE80211_M_HOSTAP &&
ni->ni_state >= IEEE80211_STA_AUTH &&
ni->ni_state != IEEE80211_STA_COLLECT) {
@@ -1209,6 +1218,20 @@ ieee80211_clean_nodes(struct ieee80211co
ieee80211_free_node(ic, ni);
ic->ic_stats.is_node_timeout++;
}
+
+#ifndef IEEE80211_STA_ONLY
+ /*
+ * During a cache timeout we iterate over all nodes.
+ * Check for node leaks by comparing the actual number of cached
+ * nodes with the ic_nnodes count, which is maintained while adding
+ * and removing nodes from the cache.
+ */
+ if ((ifp->if_flags & IFF_DEBUG) && cache_timeout &&
+ nnodes != ic->ic_nnodes)
+ printf("%s: number of cached nodes is %d, expected %d,"
+ "possible nodes leak\n", ifp->if_xname, nnodes,
+ ic->ic_nnodes);
+#endif
splx(s);
}
