some more net80211 node leaks
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.
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 - 1.120
+++ ieee80211_input.c 13 Jul 2012 17:47:15 -
@@ -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.c13 Jul 2012 09:46:33 - 1.69
+++ ieee80211_node.c13 Jul 2012 18:12:27 -
@@ -1150,6 +1150,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 +1163,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 +1202,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 +1217,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);
}
Re: some more net80211 node leaks
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 - 1.120
+++ ieee80211_input.c 13 Jul 2012 17:47:15 -
@@ -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.c13 Jul 2012 09:46:33 - 1.69
+++ ieee80211_node.c13 Jul 2012 18:23:25 -
@@ -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 @@
