From owner-freebsd-current@FreeBSD.ORG Tue Nov 18 07:21:24 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id D6DB769F for ; Tue, 18 Nov 2014 07:21:24 +0000 (UTC) Received: from mail-pd0-x236.google.com (mail-pd0-x236.google.com [IPv6:2607:f8b0:400e:c02::236]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id A9BD4B1A for ; Tue, 18 Nov 2014 07:21:24 +0000 (UTC) Received: by mail-pd0-f182.google.com with SMTP id g10so7761682pdj.27 for ; Mon, 17 Nov 2014 23:21:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=VTxkMibZvJbEwJcSsE9U85tJe6dLP3czYDfpAzTmOwE=; b=Hox2zkMsO2HGq2MOpQS5GtT4YKGc+lmaz/tBIGOsiloXcN9LRVrJ4ywRi1CF1v5FZE mZFz6WVcWteyJcSLeKJoD+r/UcGnkq9iVOSJ7LCz/ALGq7Vx8fcqS/EI2Jwy9PRm3OaA pcWWf6diMxoTumCimAdlEvV1Uw+wUbixzc0SD68P0BTu82inGRU5fJV4qBOmr/vdBYei yLpUkKcAVRmwia+H2Ka7xz41C+WUmq5b1EMs25DdvkE2rHZRE8oYdExO4EImMobzTV6V pQrc8aNkUCQesuBrSRUQ017hpYlZFD40fD/PDb/LoiqmgRniug4TCwzMhmqk/wp3NhSn gL4g== X-Received: by 10.70.109.140 with SMTP id hs12mr35007287pdb.30.1416295284137; Mon, 17 Nov 2014 23:21:24 -0800 (PST) Received: from raichu ([198.244.104.6]) by mx.google.com with ESMTPSA id o6sm37132730pdl.3.2014.11.17.23.21.22 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 17 Nov 2014 23:21:23 -0800 (PST) Sender: Mark Johnston Date: Mon, 17 Nov 2014 23:21:18 -0800 From: Mark Johnston To: Ryan Stone Subject: Re: General Protection Fault in prelist_remove() Message-ID: <20141118072118.GA1883@raichu> References: <52372362.10506@bitfrost.no> <20130916171016.GA1509@charmander> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Cc: Hans Petter Selasky , FreeBSD Current X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Nov 2014 07:21:25 -0000 On Sat, Nov 15, 2014 at 04:17:35PM -0500, Ryan Stone wrote: > On Mon, Sep 16, 2013 at 1:10 PM, Mark Johnston wrote: > > I've partially fixed this at work by adding a rw lock to protect access > > to the the prefix, default router, and DAD lists. The patch is here: > > http://people.freebsd.org/~markj/patches/ndp-locking.diff > > Hi Mark, > > I've hit a bug in this patch today. The problem is in the locking of > the DAD list. Many functions (e.g. > nd6_dad_duplicated) call nd6_dad_find() to look up a dadq structure, > and then manipulate the structure with no lock held. The problem that > once nd6_dad_find() releases the ND lock there is nothing preventing > another thread from going in and free'ing the structure. This causes > a use-after-free in nd6_dad_duplicated. I have a setup which is > somehow triggering DAD on link-local addresses (I don't understand > why; I don't have duplicate mac addresses on the network as best that > I can tell) and with INVARIANTS on I very frequently get a crash in > nd6_dad_duplicated. > > > It looks to me that the only way to fix it is either introduce > referencing counting into the structure, or push the locking out of > nd6_dad_find() and into the callers. Any opinions? I spent some time looking at the code. There seem to be a few miscellaneous bugs in addition to races you described above; nd6_dad_na_input() can potentially call nd6_dad_duplicated() with a NULL dadq, for example. I wasn't able to specifically reproduce the use-after-free that you described, but it's pretty easy to trigger crashes in the DAD code, e.g. by assigning a duplicate address to an interface in a loop. It's hard to fix these races directly because of the different contexts in which the DAD entry queue is manipulated: entries can be added or removed with ioctls when addresses are added or removed, the DAD callout can modify the queue, and nd6_dad_duplicated() can be called in the packet processing path for some NS or NA packets. It seems to me that this can be simplified somewhat though, which makes it easier to fix the races. The patch below (against HEAD) simplifies the DAD code and tries to fix remaining races by adding a refcount to struct dadq. Note that hrs@ added a lock for the DAD queue in r266857, so my ndp-locking patch needs to be rebased. It simplifies the code by having the DAD callout do most of the work: when a NS or NA packet indicates that an address is a duplicate, we just increment a counter and wait for the corresponding DAD callout to flag the address. With the patch, DAD is only cancelled when the corresponding address is removed from an interface. It also fixes some improper handling of the ifa references for struct dadq. Note that there are still some races around the struct dadq counters themselves (the increments are non-atomic), and the handling of in6_flags should be atomic. A copy of the patch is here: https://people.freebsd.org/~markj/patches/nd6_dad_races.diff It survives some basic tests and stress testing that previously exposed some races on HEAD. Thanks, -Mark diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c index a917b76..819fe30 100644 --- a/sys/netinet6/nd6_nbr.c +++ b/sys/netinet6/nd6_nbr.c @@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$"); #include #include #include +#include #include #include @@ -1167,6 +1168,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); @@ -1183,12 +1185,22 @@ static VNET_DEFINE(struct rwlock, dad_rwlock); #define DADQ_WUNLOCK() rw_wunlock(&V_dad_rwlock) static void +nd6_dad_rele(struct dadq *dp) +{ + + if (refcount_release(&dp->dad_refcnt)) { + ifa_free(dp->dad_ifa); + free(dp, M_IP6NDP); + } +} + +static void nd6_dad_add(struct dadq *dp) { - ifa_ref(dp->dad_ifa); /* just for safety */ + refcount_acquire(&dp->dad_refcnt); DADQ_WLOCK(); - TAILQ_INSERT_TAIL(&V_dadq, (struct dadq *)dp, dad_list); + TAILQ_INSERT_TAIL(&V_dadq, dp, dad_list); DADQ_WUNLOCK(); } @@ -1196,10 +1208,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 +1221,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 +1242,8 @@ static void nd6_dad_stoptimer(struct dadq *dp) { - callout_stop(&dp->dad_timer_ch); + callout_drain(&dp->dad_timer_ch); + nd6_dad_rele(dp); } /* @@ -1275,8 +1290,9 @@ nd6_dad_start(struct ifaddr *ifa, int delay) } 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; } @@ -1303,9 +1319,11 @@ nd6_dad_start(struct ifaddr *ifa, int delay) * (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); @@ -1334,8 +1352,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 @@ -1354,38 +1380,30 @@ nd6_dad_timer(struct dadq *dp) } 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 dup; } 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 dup; } 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 dup; } /* 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 dup; } /* Need more checks? */ @@ -1396,33 +1414,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,13 +1437,11 @@ 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; } } - +dup: + nd6_dad_del(dp); + nd6_dad_rele(dp); done: CURVNET_RESTORE(); } @@ -1462,9 +1461,6 @@ nd6_dad_duplicated(struct ifaddr *ifa, struct dadq *dp) 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 +1501,6 @@ nd6_dad_duplicated(struct ifaddr *ifa, struct dadq *dp) break; } } - - nd6_dad_del(dp); - free(dp, M_IP6NDP); } static void @@ -1535,7 +1528,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 +1535,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 +1549,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 +1564,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); + } }