Fix races between in_lltable_prefix_free(), lla_lookup(),

llentry_free() and arptimer():

o Use callout_init_rw() for lle timeout, this allows us safely
  disestablish them.
  - This allows us to simplify the arptimer() and make it
    race safe.
o Consistently use ifp->if_afdata_lock to lock access to
  linked lists in the lle hashes.
o Introduce new lle flag LLE_LINKED, which marks an entry that
  is attached to the hash.
  - Use LLE_LINKED to avoid double unlinking via consequent
    calls to llentry_free().
  - Mark lle with LLE_DELETED via |= operation istead of =,
    so that other flags won't be lost.
o Make LLE_ADDREF(), LLE_REMREF() and LLE_FREE_LOCKED() more
  consistent and provide more informative KASSERTs.

The patch is a collaborative work of all submitters and myself.

PR:		kern/165863
Submitted by:	Andrey Zonov <andrey zonov.org>
Submitted by:	Ryan Stone <rysto32 gmail.com>
Submitted by:	Eric van Gyzen <eric_van_gyzen dell.com>

2 files changed, 35 insertions, 42 deletions

diff --git a/sys/netinet/if_ether.c b/sys/netinet/if_ether.c
index df1f980..8e2daeb 100644
--- a/sys/netinet/if_ether.c
+++ b/sys/netinet/if_ether.c
@@ -163,49 +163,40 @@ arp_ifscrub(struct ifnet *ifp, uint32_t addr)
 static void
 arptimer(void *arg)
 {
+	struct llentry *lle = (struct llentry *)arg;
 	struct ifnet *ifp;
-	struct llentry   *lle;
-	int pkts_dropped;
+	size_t pkts_dropped;
+
+	if (lle->la_flags & LLE_STATIC) {
+		LLE_WUNLOCK(lle);
+		return;
+	}
 
-	KASSERT(arg != NULL, ("%s: arg NULL", __func__));
-	lle = (struct llentry *)arg;
 	ifp = lle->lle_tbl->llt_ifp;
 	CURVNET_SET(ifp->if_vnet);
+
+	if (lle->la_flags != LLE_DELETED) {
+		int evt;
+
+		if (lle->la_flags & LLE_VALID)
+			evt = LLENTRY_EXPIRED;
+		else
+			evt = LLENTRY_TIMEDOUT;
+		EVENTHANDLER_INVOKE(lle_event, lle, evt);
+	}
+
+	callout_stop(&lle->la_timer);
+
+	/* XXX: LOR avoidance. We still have ref on lle. */
+	LLE_WUNLOCK(lle);
 	IF_AFDATA_LOCK(ifp);
 	LLE_WLOCK(lle);
-	if (lle->la_flags & LLE_STATIC)
-		LLE_WUNLOCK(lle);
-	else {
-		if (!callout_pending(&lle->la_timer) &&
-		    callout_active(&lle->la_timer)) {
-			callout_stop(&lle->la_timer);
-			LLE_REMREF(lle);
-
-			if (lle->la_flags != LLE_DELETED) {
-				int evt;
-
-				if (lle->la_flags & LLE_VALID)
-					evt = LLENTRY_EXPIRED;
-				else
-					evt = LLENTRY_TIMEDOUT;
-				EVENTHANDLER_INVOKE(lle_event, lle, evt);
-			}
 
-			pkts_dropped = llentry_free(lle);
-			ARPSTAT_ADD(dropped, pkts_dropped);
-			ARPSTAT_INC(timeouts);
-		} else {
-#ifdef DIAGNOSTIC
-			struct sockaddr *l3addr = L3_ADDR(lle);
-			log(LOG_INFO,
-			    "arptimer issue: %p, IPv4 address: \"%s\"\n", lle,
-			    inet_ntoa(
-			        ((const struct sockaddr_in *)l3addr)->sin_addr));
-#endif
-			LLE_WUNLOCK(lle);
-		}
-	}
+	LLE_REMREF(lle);
+	pkts_dropped = llentry_free(lle);
 	IF_AFDATA_UNLOCK(ifp);
+	ARPSTAT_ADD(dropped, pkts_dropped);
+	ARPSTAT_INC(timeouts);
 	CURVNET_RESTORE();
 }
 
diff --git a/sys/netinet/in.c b/sys/netinet/in.c
index 5292d10..69f40b9 100644
--- a/sys/netinet/in.c
+++ b/sys/netinet/in.c
@@ -1280,7 +1280,6 @@ in_lltable_new(const struct sockaddr *l3addr, u_int flags)
 	if (lle == NULL)		/* NB: caller generates msg */
 		return NULL;
 
-	callout_init(&lle->base.la_timer, CALLOUT_MPSAFE);
 	/*
 	 * For IPv4 this will trigger "arpresolve" to generate
 	 * an ARP request.
@@ -1290,7 +1289,10 @@ in_lltable_new(const struct sockaddr *l3addr, u_int flags)
 	lle->base.lle_refcnt = 1;
 	lle->base.lle_free = in_lltable_free;
 	LLE_LOCK_INIT(&lle->base);
-	return &lle->base;
+	callout_init_rw(&lle->base.la_timer, &lle->base.lle_lock,
+	    CALLOUT_RETURNUNLOCKED);
+
+	return (&lle->base);
 }
 
 #define IN_ARE_MASKED_ADDR_EQUAL(d, a, m)	(			\
@@ -1306,6 +1308,7 @@ in_lltable_prefix_free(struct lltable *llt, const struct sockaddr *prefix,
 	int i;
 	size_t pkts_dropped;
 
+	IF_AFDATA_WLOCK(llt->llt_ifp);
 	for (i = 0; i < LLTBL_HASHTBL_SIZE; i++) {
 		LIST_FOREACH_SAFE(lle, &llt->lle_head[i], lle_next, next) {
 			/*
@@ -1315,17 +1318,15 @@ in_lltable_prefix_free(struct lltable *llt, const struct sockaddr *prefix,
 			if (IN_ARE_MASKED_ADDR_EQUAL(satosin(L3_ADDR(lle)),
 			    pfx, msk) && ((flags & LLE_STATIC) ||
 			    !(lle->la_flags & LLE_STATIC))) {
-				int canceled;
-
-				canceled = callout_drain(&lle->la_timer);
 				LLE_WLOCK(lle);
-				if (canceled)
+				if (callout_stop(&lle->la_timer))
 					LLE_REMREF(lle);
 				pkts_dropped = llentry_free(lle);
 				ARPSTAT_ADD(dropped, pkts_dropped);
 			}
 		}
 	}
+	IF_AFDATA_WUNLOCK(llt->llt_ifp);
 }
 
 
@@ -1457,11 +1458,12 @@ in_lltable_lookup(struct lltable *llt, u_int flags, const struct sockaddr *l3add
 
 		lle->lle_tbl  = llt;
 		lle->lle_head = lleh;
+		lle->la_flags |= LLE_LINKED;
 		LIST_INSERT_HEAD(lleh, lle, lle_next);
 	} else if (flags & LLE_DELETE) {
 		if (!(lle->la_flags & LLE_IFADDR) || (flags & LLE_IFADDR)) {
 			LLE_WLOCK(lle);
-			lle->la_flags = LLE_DELETED;
+			lle->la_flags |= LLE_DELETED;
 			EVENTHANDLER_INVOKE(lle_event, lle, LLENTRY_DELETED);
 			LLE_WUNLOCK(lle);
 #ifdef DIAGNOSTIC