Date: Wed, 15 Aug 2012 20:10:11 GMT From: dfilter@FreeBSD.ORG (dfilter service) To: freebsd-wireless@FreeBSD.org Subject: Re: kern/170098: commit references a PR Message-ID: <201208152010.q7FKABmL091692@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/170098; it has been noted by GNATS. From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/170098: commit references a PR Date: Wed, 15 Aug 2012 20:01:39 +0000 (UTC) Author: adrian Date: Wed Aug 15 20:01:28 2012 New Revision: 239312 URL: http://svn.freebsd.org/changeset/base/239312 Log: Don't call the node iteration function inside the node table / node iterate lock. This causes LORs and deadlocks as some code paths will have the com lock held when calling ieee80211_iterate_nodes(). Here, the comlock isn't held during the node table and node iteration locks; and the callback isn't called with any (extra) lock held. PR: kern/170098 Submitted by: moonlightakkiy@yahoo.ca MFC after: 4 weeks Modified: head/sys/net80211/ieee80211_node.c head/sys/net80211/ieee80211_node.h Modified: head/sys/net80211/ieee80211_node.c ============================================================================== --- head/sys/net80211/ieee80211_node.c Wed Aug 15 19:59:13 2012 (r239311) +++ head/sys/net80211/ieee80211_node.c Wed Aug 15 20:01:28 2012 (r239312) @@ -2156,30 +2156,124 @@ ieee80211_node_timeout(void *arg) ieee80211_node_timeout, ic); } -void -ieee80211_iterate_nodes(struct ieee80211_node_table *nt, - ieee80211_iter_func *f, void *arg) +/* + * Iterate over the node table and return an array of ref'ed nodes. + * + * This is separated out from calling the actual node function so that + * no LORs will occur. + * + * If there are too many nodes (ie, the number of nodes doesn't fit + * within 'max_aid' entries) then the node references will be freed + * and an error will be returned. + * + * The responsibility of allocating and freeing "ni_arr" is up to + * the caller. + */ +int +ieee80211_iterate_nt(struct ieee80211_node_table *nt, + struct ieee80211_node **ni_arr, uint16_t max_aid) { - struct ieee80211_node *ni; u_int gen; + int i, j, ret; + struct ieee80211_node *ni; IEEE80211_NODE_ITERATE_LOCK(nt); - gen = ++nt->nt_scangen; -restart: IEEE80211_NODE_LOCK(nt); + + gen = ++nt->nt_scangen; + i = ret = 0; + + /* + * We simply assume here that since the node + * scan generation doesn't change (as + * we are holding both the node table and + * node table iteration locks), we can simply + * assign it to the node here. + */ TAILQ_FOREACH(ni, &nt->nt_node, ni_list) { - if (ni->ni_scangen != gen) { - ni->ni_scangen = gen; - (void) ieee80211_ref_node(ni); - IEEE80211_NODE_UNLOCK(nt); - (*f)(arg, ni); - ieee80211_free_node(ni); - goto restart; + if (i >= max_aid) { + ret = E2BIG; + if_printf(nt->nt_ic->ic_ifp, + "Node array overflow: max=%u", max_aid); + break; } + ni_arr[i] = ieee80211_ref_node(ni); + ni_arr[i]->ni_scangen = gen; + i++; } - IEEE80211_NODE_UNLOCK(nt); + /* + * It's safe to unlock here. + * + * If we're successful, the list is returned. + * If we're unsuccessful, the list is ignored + * and we remove our references. + * + * This avoids any potential LOR with + * ieee80211_free_node(). + */ + IEEE80211_NODE_UNLOCK(nt); IEEE80211_NODE_ITERATE_UNLOCK(nt); + + /* + * If ret is non-zero, we hit some kind of error. + * Rather than walking some nodes, we'll walk none + * of them. + */ + if (ret) { + for (j = 0; j < i; j++) { + /* ieee80211_free_node() locks by itself */ + ieee80211_free_node(ni_arr[j]); + } + } + + return (ret); +} + +/* + * Just a wrapper, so we don't have to change every ieee80211_iterate_nodes() + * reference in the source. + * + * Note that this fetches 'max_aid' from the first VAP, rather than finding + * the largest max_aid from all VAPs. + */ +void +ieee80211_iterate_nodes(struct ieee80211_node_table *nt, + ieee80211_iter_func *f, void *arg) +{ + struct ieee80211_node **ni_arr; + unsigned long size; + int i; + uint16_t max_aid; + + max_aid = TAILQ_FIRST(&nt->nt_ic->ic_vaps)->iv_max_aid; + size = max_aid * sizeof(struct ieee80211_node *); + ni_arr = (struct ieee80211_node **) malloc(size, M_80211_NODE, + M_NOWAIT | M_ZERO); + if (ni_arr == NULL) + return; + + /* + * If this fails, the node table won't have any + * valid entries - ieee80211_iterate_nt() frees + * the references to them. So don't try walking + * the table; just skip to the end and free the + * temporary memory. + */ + if (!ieee80211_iterate_nt(nt, ni_arr, max_aid)) + goto done; + + for (i = 0; i < max_aid; i++) { + if (ni_arr[i] == NULL) /* end of the list */ + break; + + (*f)(arg, ni_arr[i]); + /* ieee80211_free_node() locks by itself */ + ieee80211_free_node(ni_arr[i]); + } + +done: + free(ni_arr, M_80211_NODE); } void Modified: head/sys/net80211/ieee80211_node.h ============================================================================== --- head/sys/net80211/ieee80211_node.h Wed Aug 15 19:59:13 2012 (r239311) +++ head/sys/net80211/ieee80211_node.h Wed Aug 15 20:01:28 2012 (r239312) @@ -438,6 +438,8 @@ int ieee80211_node_delucastkey(struct ie void ieee80211_node_timeout(void *arg); typedef void ieee80211_iter_func(void *, struct ieee80211_node *); +int ieee80211_iterate_nt(struct ieee80211_node_table *, + struct ieee80211_node **, uint16_t); void ieee80211_iterate_nodes(struct ieee80211_node_table *, ieee80211_iter_func *, void *); _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201208152010.q7FKABmL091692>