Date: Mon, 8 Dec 2014 04:44:40 +0000 (UTC) From: Mark Johnston <markj@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r275593 - head/sys/netinet6 Message-ID: <201412080444.sB84ieRg053316@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: markj Date: Mon Dec 8 04:44:40 2014 New Revision: 275593 URL: https://svnweb.freebsd.org/changeset/base/275593 Log: Add refcounting to IPv6 DAD objects and simplify the DAD code to fix a number of races which could cause double frees or use-after-frees when performing DAD on an address. In particular, an IPv6 address can now only be marked as a duplicate from the DAD callout. Differential Revision: https://reviews.freebsd.org/D1258 Reviewed by: ae, hrs Reported by: rstone MFC after: 1 month Modified: head/sys/netinet6/nd6.c head/sys/netinet6/nd6.h head/sys/netinet6/nd6_nbr.c Modified: head/sys/netinet6/nd6.c ============================================================================== --- head/sys/netinet6/nd6.c Mon Dec 8 04:35:34 2014 (r275592) +++ head/sys/netinet6/nd6.c Mon Dec 8 04:44:40 2014 (r275593) @@ -153,6 +153,8 @@ nd6_init(void) callout_init(&V_nd6_slowtimo_ch, 0); callout_reset(&V_nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL * hz, nd6_slowtimo, curvnet); + + nd6_dad_init(); } #ifdef VIMAGE Modified: head/sys/netinet6/nd6.h ============================================================================== --- head/sys/netinet6/nd6.h Mon Dec 8 04:35:34 2014 (r275592) +++ head/sys/netinet6/nd6.h Mon Dec 8 04:44:40 2014 (r275593) @@ -428,6 +428,7 @@ void nd6_ns_input(struct mbuf *, int, in void nd6_ns_output(struct ifnet *, const struct in6_addr *, const struct in6_addr *, struct llentry *, int); caddr_t nd6_ifptomac(struct ifnet *); +void nd6_dad_init(void); void nd6_dad_start(struct ifaddr *, int); void nd6_dad_stop(struct ifaddr *); Modified: head/sys/netinet6/nd6_nbr.c ============================================================================== --- head/sys/netinet6/nd6_nbr.c Mon Dec 8 04:35:34 2014 (r275592) +++ head/sys/netinet6/nd6_nbr.c Mon Dec 8 04:44:40 2014 (r275593) @@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$"); #include <sys/syslog.h> #include <sys/queue.h> #include <sys/callout.h> +#include <sys/refcount.h> #include <net/if.h> #include <net/if_types.h> @@ -81,6 +82,7 @@ struct dadq; static struct dadq *nd6_dad_find(struct ifaddr *); static void nd6_dad_add(struct dadq *dp); static void nd6_dad_del(struct dadq *dp); +static void nd6_dad_rele(struct dadq *); static void nd6_dad_starttimer(struct dadq *, int); static void nd6_dad_stoptimer(struct dadq *); static void nd6_dad_timer(struct dadq *); @@ -1167,6 +1169,7 @@ struct dadq { int dad_na_icount; struct callout dad_timer_ch; struct vnet *dad_vnet; + u_int dad_refcnt; }; static VNET_DEFINE(TAILQ_HEAD(, dadq), dadq); @@ -1174,9 +1177,6 @@ static VNET_DEFINE(struct rwlock, dad_rw #define V_dadq VNET(dadq) #define V_dad_rwlock VNET(dad_rwlock) -#define DADQ_LOCK_INIT() rw_init(&V_dad_rwlock, "nd6 DAD queue") -#define DADQ_LOCK_DESTROY() rw_destroy(&V_dad_rwlock) -#define DADQ_LOCK_INITIALIZED() rw_initialized(&V_dad_rwlock) #define DADQ_RLOCK() rw_rlock(&V_dad_rwlock) #define DADQ_RUNLOCK() rw_runlock(&V_dad_rwlock) #define DADQ_WLOCK() rw_wlock(&V_dad_rwlock) @@ -1186,9 +1186,8 @@ static void nd6_dad_add(struct dadq *dp) { - ifa_ref(dp->dad_ifa); /* just for safety */ DADQ_WLOCK(); - TAILQ_INSERT_TAIL(&V_dadq, (struct dadq *)dp, dad_list); + TAILQ_INSERT_TAIL(&V_dadq, dp, dad_list); DADQ_WUNLOCK(); } @@ -1196,10 +1195,10 @@ static void nd6_dad_del(struct dadq *dp) { - ifa_free(dp->dad_ifa); DADQ_WLOCK(); - TAILQ_REMOVE(&V_dadq, (struct dadq *)dp, dad_list); + TAILQ_REMOVE(&V_dadq, dp, dad_list); DADQ_WUNLOCK(); + nd6_dad_rele(dp); } static struct dadq * @@ -1209,8 +1208,10 @@ nd6_dad_find(struct ifaddr *ifa) DADQ_RLOCK(); TAILQ_FOREACH(dp, &V_dadq, dad_list) - if (dp->dad_ifa == ifa) + if (dp->dad_ifa == ifa) { + refcount_acquire(&dp->dad_refcnt); break; + } DADQ_RUNLOCK(); return (dp); @@ -1228,7 +1229,25 @@ static void nd6_dad_stoptimer(struct dadq *dp) { - callout_stop(&dp->dad_timer_ch); + callout_drain(&dp->dad_timer_ch); +} + +static void +nd6_dad_rele(struct dadq *dp) +{ + + if (refcount_release(&dp->dad_refcnt)) { + ifa_free(dp->dad_ifa); + free(dp, M_IP6NDP); + } +} + +void +nd6_dad_init(void) +{ + + rw_init(&V_dad_rwlock, "nd6 DAD queue"); + TAILQ_INIT(&V_dadq); } /* @@ -1241,11 +1260,6 @@ nd6_dad_start(struct ifaddr *ifa, int de struct dadq *dp; char ip6buf[INET6_ADDRSTRLEN]; - if (DADQ_LOCK_INITIALIZED() == 0) { - DADQ_LOCK_INIT(); - TAILQ_INIT(&V_dadq); - } - /* * If we don't need DAD, don't do it. * There are several cases: @@ -1275,12 +1289,13 @@ nd6_dad_start(struct ifaddr *ifa, int de } if (ND_IFINFO(ifa->ifa_ifp)->flags & ND6_IFF_IFDISABLED) return; - if (nd6_dad_find(ifa) != NULL) { + if ((dp = nd6_dad_find(ifa)) != NULL) { /* DAD already in progress */ + nd6_dad_rele(dp); return; } - dp = malloc(sizeof(*dp), M_IP6NDP, M_NOWAIT); + dp = malloc(sizeof(*dp), M_IP6NDP, M_NOWAIT | M_ZERO); if (dp == NULL) { log(LOG_ERR, "nd6_dad_start: memory allocation failed for " "%s(%s)\n", @@ -1288,7 +1303,6 @@ nd6_dad_start(struct ifaddr *ifa, int de ifa->ifa_ifp ? if_name(ifa->ifa_ifp) : "???"); return; } - bzero(dp, sizeof(*dp)); callout_init(&dp->dad_timer_ch, 0); #ifdef VIMAGE dp->dad_vnet = curvnet; @@ -1303,9 +1317,11 @@ nd6_dad_start(struct ifaddr *ifa, int de * (re)initialization. */ dp->dad_ifa = ifa; + ifa_ref(dp->dad_ifa); dp->dad_count = V_ip6_dad_count; dp->dad_ns_icount = dp->dad_na_icount = 0; dp->dad_ns_ocount = dp->dad_ns_tcount = 0; + refcount_init(&dp->dad_refcnt, 1); nd6_dad_add(dp); if (delay == 0) { nd6_dad_ns_output(dp, ifa); @@ -1324,8 +1340,6 @@ nd6_dad_stop(struct ifaddr *ifa) { struct dadq *dp; - if (DADQ_LOCK_INITIALIZED() == 0) - return; dp = nd6_dad_find(ifa); if (!dp) { /* DAD wasn't started yet */ @@ -1334,8 +1348,16 @@ nd6_dad_stop(struct ifaddr *ifa) nd6_dad_stoptimer(dp); + /* + * The DAD queue entry may have been removed by nd6_dad_timer() while + * we were waiting for it to stop, so re-do the lookup. + */ + nd6_dad_rele(dp); + if (nd6_dad_find(ifa) == NULL) + return; + nd6_dad_del(dp); - free(dp, M_IP6NDP); + nd6_dad_rele(dp); } static void @@ -1350,42 +1372,34 @@ nd6_dad_timer(struct dadq *dp) /* Sanity check */ if (ia == NULL) { log(LOG_ERR, "nd6_dad_timer: called with null parameter\n"); - goto done; + goto err; } if (ND_IFINFO(ifp)->flags & ND6_IFF_IFDISABLED) { /* Do not need DAD for ifdisabled interface. */ - TAILQ_REMOVE(&V_dadq, (struct dadq *)dp, dad_list); log(LOG_ERR, "nd6_dad_timer: cancel DAD on %s because of " "ND6_IFF_IFDISABLED.\n", ifp->if_xname); - free(dp, M_IP6NDP); - dp = NULL; - ifa_free(ifa); - goto done; + goto err; } if (ia->ia6_flags & IN6_IFF_DUPLICATED) { log(LOG_ERR, "nd6_dad_timer: called with duplicated address " "%s(%s)\n", ip6_sprintf(ip6buf, &ia->ia_addr.sin6_addr), ifa->ifa_ifp ? if_name(ifa->ifa_ifp) : "???"); - goto done; + goto err; } if ((ia->ia6_flags & IN6_IFF_TENTATIVE) == 0) { log(LOG_ERR, "nd6_dad_timer: called with non-tentative address " "%s(%s)\n", ip6_sprintf(ip6buf, &ia->ia_addr.sin6_addr), ifa->ifa_ifp ? if_name(ifa->ifa_ifp) : "???"); - goto done; + goto err; } /* timeouted with IFF_{RUNNING,UP} check */ if (dp->dad_ns_tcount > V_dad_maxtry) { nd6log((LOG_INFO, "%s: could not run DAD, driver problem?\n", if_name(ifa->ifa_ifp))); - - nd6_dad_del(dp); - free(dp, M_IP6NDP); - dp = NULL; - goto done; + goto err; } /* Need more checks? */ @@ -1396,33 +1410,16 @@ nd6_dad_timer(struct dadq *dp) nd6_dad_ns_output(dp, ifa); nd6_dad_starttimer(dp, (long)ND_IFINFO(ifa->ifa_ifp)->retrans * hz / 1000); + goto done; } else { /* * We have transmitted sufficient number of DAD packets. * See what we've got. */ - int duplicate; - - duplicate = 0; - - if (dp->dad_na_icount) { - /* - * the check is in nd6_dad_na_input(), - * but just in case - */ - duplicate++; - } - - if (dp->dad_ns_icount) { - /* We've seen NS, means DAD has failed. */ - duplicate++; - } - - if (duplicate) { - /* (*dp) will be freed in nd6_dad_duplicated() */ + if (dp->dad_ns_icount > 0 || dp->dad_na_icount > 0) + /* We've seen NS or NA, means DAD has failed. */ nd6_dad_duplicated(ifa, dp); - dp = NULL; - } else { + else { /* * We are done with DAD. No NA came, no NS came. * No duplicate address found. Check IFDISABLED flag @@ -1436,18 +1433,15 @@ nd6_dad_timer(struct dadq *dp) "%s: DAD complete for %s - no duplicates found\n", if_name(ifa->ifa_ifp), ip6_sprintf(ip6buf, &ia->ia_addr.sin6_addr))); - - nd6_dad_del(dp); - free(dp, M_IP6NDP); - dp = NULL; } } - +err: + nd6_dad_del(dp); done: CURVNET_RESTORE(); } -void +static void nd6_dad_duplicated(struct ifaddr *ifa, struct dadq *dp) { struct in6_ifaddr *ia = (struct in6_ifaddr *)ifa; @@ -1462,9 +1456,6 @@ nd6_dad_duplicated(struct ifaddr *ifa, s ia->ia6_flags &= ~IN6_IFF_TENTATIVE; ia->ia6_flags |= IN6_IFF_DUPLICATED; - /* We are done with DAD, with duplicate address found. (failure) */ - nd6_dad_stoptimer(dp); - ifp = ifa->ifa_ifp; log(LOG_ERR, "%s: DAD complete for %s - duplicate found\n", if_name(ifp), ip6_sprintf(ip6buf, &ia->ia_addr.sin6_addr)); @@ -1505,9 +1496,6 @@ nd6_dad_duplicated(struct ifaddr *ifa, s break; } } - - nd6_dad_del(dp); - free(dp, M_IP6NDP); } static void @@ -1535,7 +1523,6 @@ nd6_dad_ns_input(struct ifaddr *ifa) struct ifnet *ifp; const struct in6_addr *taddr6; struct dadq *dp; - int duplicate; if (ifa == NULL) panic("ifa == NULL in nd6_dad_ns_input"); @@ -1543,8 +1530,9 @@ nd6_dad_ns_input(struct ifaddr *ifa) ia = (struct in6_ifaddr *)ifa; ifp = ifa->ifa_ifp; taddr6 = &ia->ia_addr.sin6_addr; - duplicate = 0; dp = nd6_dad_find(ifa); + if (dp == NULL) + return; /* Quickhack - completely ignore DAD NS packets */ if (V_dad_ignore_ns) { @@ -1556,26 +1544,10 @@ nd6_dad_ns_input(struct ifaddr *ifa) return; } - /* - * if I'm yet to start DAD, someone else started using this address - * first. I have a duplicate and you win. - */ - if (dp == NULL || dp->dad_ns_ocount == 0) - duplicate++; - /* XXX more checks for loopback situation - see nd6_dad_timer too */ - if (duplicate) { - nd6_dad_duplicated(ifa, dp); - dp = NULL; /* will be freed in nd6_dad_duplicated() */ - } else { - /* - * not sure if I got a duplicate. - * increment ns count and see what happens. - */ - if (dp) - dp->dad_ns_icount++; - } + dp->dad_ns_icount++; + nd6_dad_rele(dp); } static void @@ -1587,9 +1559,8 @@ nd6_dad_na_input(struct ifaddr *ifa) panic("ifa == NULL in nd6_dad_na_input"); dp = nd6_dad_find(ifa); - if (dp) + if (dp != NULL) { dp->dad_na_icount++; - - /* remove the address. */ - nd6_dad_duplicated(ifa, dp); + nd6_dad_rele(dp); + } }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201412080444.sB84ieRg053316>