nd6: Make the DAD callout MPSAFE

Interface addresses with pending duplicate address detection (DAD) live
in a global queue.  In this case, a callout is associated with each
entry.  The callout transmits neighbour solicitations until the system
decides the address is no longer tentative, or until a duplicate address
is discovered.  At this point the entry is dequeued and freed.  DAD may
be manually stopped as well.

The callout currently runs (and potentially transmits packets) with
Giant held.  Reorganize DAD queue locking to interlock properly with the
callout:

- Configure the callout to acquire the DAD queue lock before running.
  The lock is dropped before transmitting any packets.  Stop protecting
  the callout with Giant.
- When looking up DAD queue entries for an incoming NS or NA, don't
  bother fiddling with the DAD queue entry reference count.
- Split nd6_dad_starttimer() so that the caller is responsible to
  transmitting a NS if it so desires.
- Remove the DAD entry from the queue before stopping the timer.  Use a
  temporary reference to make sure that the entry doesn't get freed by
  the callout while we're draining.

Reported by:	mav
Reviewed by:	bz, hrs
Sponsored by:	The FreeBSD Foundation
MFC after:	2 weeks
Differential Revision:	https://reviews.freebsd.org/D31826

1 files changed, 65 insertions, 51 deletions

diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c
index 30d73f9d71a9..2100700733ad 100644
--- a/sys/netinet6/nd6_nbr.c
+++ b/sys/netinet6/nd6_nbr.c
@@ -84,9 +84,9 @@ static struct dadq *nd6_dad_find(struct ifaddr *, struct nd_opt_nonce *);
 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, int);
+static void nd6_dad_starttimer(struct dadq *, int);
 static void nd6_dad_stoptimer(struct dadq *);
-static void nd6_dad_timer(struct dadq *);
+static void nd6_dad_timer(void *);
 static void nd6_dad_duplicated(struct ifaddr *, struct dadq *);
 static void nd6_dad_ns_output(struct dadq *);
 static void nd6_dad_ns_input(struct ifaddr *, struct nd_opt_nonce *);
@@ -1135,26 +1135,31 @@ VNET_DEFINE_STATIC(struct rwlock, dad_rwlock);
 #define	V_dadq			VNET(dadq)
 #define	V_dad_rwlock		VNET(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)	
-#define	DADQ_WUNLOCK()		rw_wunlock(&V_dad_rwlock)	
+#define	DADQ_LOCKPTR()		(&V_dad_rwlock)
+#define	DADQ_LOCK_INIT()	rw_init(DADQ_LOCKPTR(), "nd6 DAD queue")
+#define	DADQ_RLOCK()		rw_rlock(DADQ_LOCKPTR())
+#define	DADQ_RUNLOCK()		rw_runlock(DADQ_LOCKPTR())
+#define	DADQ_WLOCK()		rw_wlock(DADQ_LOCKPTR())
+#define	DADQ_WUNLOCK()		rw_wunlock(DADQ_LOCKPTR())
+
+#define	DADQ_LOCK_ASSERT()	rw_assert(DADQ_LOCKPTR(), RA_LOCKED);
+#define	DADQ_RLOCK_ASSERT()	rw_assert(DADQ_LOCKPTR(), RA_RLOCKED);
+#define	DADQ_WLOCK_ASSERT()	rw_assert(DADQ_LOCKPTR(), RA_WLOCKED);
 
 static void
 nd6_dad_add(struct dadq *dp)
 {
+	DADQ_WLOCK_ASSERT();
 
-	DADQ_WLOCK();
 	TAILQ_INSERT_TAIL(&V_dadq, dp, dad_list);
 	dp->dad_ondadq = true;
-	DADQ_WUNLOCK();
 }
 
 static void
 nd6_dad_del(struct dadq *dp)
 {
+	DADQ_WLOCK_ASSERT();
 
-	DADQ_WLOCK();
 	if (dp->dad_ondadq) {
 		/*
 		 * Remove dp from the dadq and release the dadq's
@@ -1162,10 +1167,8 @@ nd6_dad_del(struct dadq *dp)
 		 */
 		TAILQ_REMOVE(&V_dadq, dp, dad_list);
 		dp->dad_ondadq = false;
-		DADQ_WUNLOCK();
 		nd6_dad_rele(dp);
-	} else
-		DADQ_WUNLOCK();
+	}
 }
 
 static struct dadq *
@@ -1173,10 +1176,12 @@ nd6_dad_find(struct ifaddr *ifa, struct nd_opt_nonce *n)
 {
 	struct dadq *dp;
 
-	DADQ_RLOCK();
+	DADQ_LOCK_ASSERT();
+
 	TAILQ_FOREACH(dp, &V_dadq, dad_list) {
 		if (dp->dad_ifa != ifa)
 			continue;
+
 		/*
 		 * Skip if the nonce matches the received one.
 		 * +2 in the length is required because of type and
@@ -1185,42 +1190,35 @@ nd6_dad_find(struct ifaddr *ifa, struct nd_opt_nonce *n)
 		if (n != NULL &&
 		    n->nd_opt_nonce_len == (ND_OPT_NONCE_LEN + 2) / 8 &&
 		    memcmp(&n->nd_opt_nonce[0], &dp->dad_nonce[0],
-		        ND_OPT_NONCE_LEN) == 0) {
+		    ND_OPT_NONCE_LEN) == 0) {
 			dp->dad_ns_lcount++;
 			continue;
 		}
-		refcount_acquire(&dp->dad_refcnt);
 		break;
 	}
-	DADQ_RUNLOCK();
 
 	return (dp);
 }
 
 static void
-nd6_dad_starttimer(struct dadq *dp, int ticks, int send_ns)
+nd6_dad_starttimer(struct dadq *dp, int ticks)
 {
+	DADQ_WLOCK_ASSERT();
 
-	NET_EPOCH_ASSERT();
-
-	if (send_ns != 0)
-		nd6_dad_ns_output(dp);
-	callout_reset(&dp->dad_timer_ch, ticks,
-	    (void (*)(void *))nd6_dad_timer, (void *)dp);
+	callout_reset(&dp->dad_timer_ch, ticks, nd6_dad_timer, dp);
 }
 
 static void
 nd6_dad_stoptimer(struct dadq *dp)
 {
-
 	callout_drain(&dp->dad_timer_ch);
 }
 
 static void
 nd6_dad_rele(struct dadq *dp)
 {
-
 	if (refcount_release(&dp->dad_refcnt)) {
+		KASSERT(!dp->dad_ondadq, ("dp %p still on DAD queue", dp));
 		ifa_free(dp->dad_ifa);
 		free(dp, M_IP6NDP);
 	}
@@ -1229,8 +1227,7 @@ nd6_dad_rele(struct dadq *dp)
 void
 nd6_dad_init(void)
 {
-
-	rw_init(&V_dad_rwlock, "nd6 DAD queue");
+	DADQ_LOCK_INIT();
 	TAILQ_INIT(&V_dadq);
 }
 
@@ -1243,7 +1240,6 @@ nd6_dad_start(struct ifaddr *ifa, int delay)
 	struct in6_ifaddr *ia = (struct in6_ifaddr *)ifa;
 	struct dadq *dp;
 	char ip6buf[INET6_ADDRSTRLEN];
-	struct epoch_tracker et;
 
 	KASSERT((ia->ia6_flags & IN6_IFF_TENTATIVE) != 0,
 	    ("starting DAD on non-tentative address %p", ifa));
@@ -1265,12 +1261,13 @@ nd6_dad_start(struct ifaddr *ifa, int delay)
 	    (ND_IFINFO(ifa->ifa_ifp)->flags & ND6_IFF_IFDISABLED) != 0)
 		return;
 
+	DADQ_WLOCK();
 	if ((dp = nd6_dad_find(ifa, NULL)) != NULL) {
 		/*
 		 * DAD is already in progress.  Let the existing entry
 		 * finish it.
 		 */
-		nd6_dad_rele(dp);
+		DADQ_WUNLOCK();
 		return;
 	}
 
@@ -1282,7 +1279,8 @@ nd6_dad_start(struct ifaddr *ifa, int delay)
 			ifa->ifa_ifp ? if_name(ifa->ifa_ifp) : "???");
 		return;
 	}
-	callout_init(&dp->dad_timer_ch, 0);
+	callout_init_rw(&dp->dad_timer_ch, DADQ_LOCKPTR(),
+	    CALLOUT_RETURNUNLOCKED);
 #ifdef VIMAGE
 	dp->dad_vnet = curvnet;
 #endif
@@ -1305,9 +1303,8 @@ nd6_dad_start(struct ifaddr *ifa, int delay)
 	/* Add this to the dadq and add a reference for the dadq. */
 	refcount_init(&dp->dad_refcnt, 1);
 	nd6_dad_add(dp);
-	NET_EPOCH_ENTER(et);
-	nd6_dad_starttimer(dp, delay, 0);
-	NET_EPOCH_EXIT(et);
+	nd6_dad_starttimer(dp, delay);
+	DADQ_WUNLOCK();
 }
 
 /*
@@ -1318,29 +1315,36 @@ nd6_dad_stop(struct ifaddr *ifa)
 {
 	struct dadq *dp;
 
+	DADQ_WLOCK();
 	dp = nd6_dad_find(ifa, NULL);
-	if (!dp) {
+	if (dp == NULL) {
+		DADQ_WUNLOCK();
 		/* DAD wasn't started yet */
 		return;
 	}
 
-	nd6_dad_stoptimer(dp);
+	/*
+	 * Acquire a temporary reference so that we can safely stop the callout.
+	 */
+	(void)refcount_acquire(&dp->dad_refcnt);
 	nd6_dad_del(dp);
+	DADQ_WUNLOCK();
 
-	/* Release this function's reference, acquired by nd6_dad_find(). */
+	nd6_dad_stoptimer(dp);
 	nd6_dad_rele(dp);
 }
 
 static void
-nd6_dad_timer(struct dadq *dp)
+nd6_dad_timer(void *arg)
 {
-	CURVNET_SET(dp->dad_vnet);
+	struct dadq *dp = arg;
 	struct ifaddr *ifa = dp->dad_ifa;
 	struct ifnet *ifp = dp->dad_ifa->ifa_ifp;
 	struct in6_ifaddr *ia = (struct in6_ifaddr *)ifa;
 	char ip6buf[INET6_ADDRSTRLEN];
 	struct epoch_tracker et;
 
+	CURVNET_SET(dp->dad_vnet);
 	KASSERT(ia != NULL, ("DAD entry %p with no address", dp));
 
 	NET_EPOCH_ENTER(et);
@@ -1381,17 +1385,18 @@ nd6_dad_timer(struct dadq *dp)
 		 * We have more NS to go.  Send NS packet for DAD.
 		 */
 		nd6_dad_starttimer(dp,
-		    (long)ND_IFINFO(ifa->ifa_ifp)->retrans * hz / 1000, 1);
+		    (long)ND_IFINFO(ifa->ifa_ifp)->retrans * hz / 1000);
+		nd6_dad_ns_output(dp);
 		goto done;
 	} else {
 		/*
 		 * We have transmitted sufficient number of DAD packets.
 		 * See what we've got.
 		 */
-		if (dp->dad_ns_icount > 0 || dp->dad_na_icount > 0)
+		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);
-		else if (V_dad_enhanced != 0 &&
+		} else if (V_dad_enhanced != 0 &&
 		    dp->dad_ns_lcount > 0 &&
 		    dp->dad_ns_lcount > dp->dad_loopbackprobe) {
 			/*
@@ -1412,8 +1417,8 @@ nd6_dad_timer(struct dadq *dp)
 			dp->dad_count =
 			    dp->dad_ns_ocount + V_nd6_mmaxtries - 1;
 			nd6_dad_starttimer(dp,
-			    (long)ND_IFINFO(ifa->ifa_ifp)->retrans * hz / 1000,
-			    1);
+			    (long)ND_IFINFO(ifa->ifa_ifp)->retrans * hz / 1000);
+			nd6_dad_ns_output(dp);
 			goto done;
 		} else {
 			/*
@@ -1439,6 +1444,7 @@ nd6_dad_timer(struct dadq *dp)
 	}
 err:
 	nd6_dad_del(dp);
+	DADQ_WUNLOCK();
 done:
 	NET_EPOCH_EXIT(et);
 	CURVNET_RESTORE();
@@ -1498,6 +1504,10 @@ nd6_dad_duplicated(struct ifaddr *ifa, struct dadq *dp)
 	}
 }
 
+/*
+ * Transmit a neighbour solicitation for the purpose of DAD.  Returns with the
+ * DAD queue unlocked.
+ */
 static void
 nd6_dad_ns_output(struct dadq *dp)
 {
@@ -1505,11 +1515,15 @@ nd6_dad_ns_output(struct dadq *dp)
 	struct ifnet *ifp = dp->dad_ifa->ifa_ifp;
 	int i;
 
+	DADQ_WLOCK_ASSERT();
+
 	dp->dad_ns_tcount++;
 	if ((ifp->if_flags & IFF_UP) == 0) {
+		DADQ_WUNLOCK();
 		return;
 	}
 	if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0) {
+		DADQ_WUNLOCK();
 		return;
 	}
 
@@ -1526,6 +1540,7 @@ nd6_dad_ns_output(struct dadq *dp)
 		 * should work well in almost all cases.
 		 */
 	}
+	DADQ_WUNLOCK();
 	nd6_ns_output(ifp, NULL, NULL, &ia->ia_addr.sin6_addr,
 	    (uint8_t *)&dp->dad_nonce[0]);
 }
@@ -1541,12 +1556,11 @@ nd6_dad_ns_input(struct ifaddr *ifa, struct nd_opt_nonce *ndopt_nonce)
 	/* Ignore Nonce option when Enhanced DAD is disabled. */
 	if (V_dad_enhanced == 0)
 		ndopt_nonce = NULL;
+	DADQ_RLOCK();
 	dp = nd6_dad_find(ifa, ndopt_nonce);
-	if (dp == NULL)
-		return;
-
-	dp->dad_ns_icount++;
-	nd6_dad_rele(dp);
+	if (dp != NULL)
+		dp->dad_ns_icount++;
+	DADQ_RUNLOCK();
 }
 
 static void
@@ -1557,9 +1571,9 @@ nd6_dad_na_input(struct ifaddr *ifa)
 	if (ifa == NULL)
 		panic("ifa == NULL in nd6_dad_na_input");
 
+	DADQ_RLOCK();
 	dp = nd6_dad_find(ifa, NULL);
-	if (dp != NULL) {
+	if (dp != NULL)
 		dp->dad_na_icount++;
-		nd6_dad_rele(dp);
-	}
+	DADQ_RUNLOCK();
 }