Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 11 Sep 2013 07:24:46 +0000 (UTC)
From:      Devin Teske <dteske@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r255470 - in stable/8/sys: net netinet netinet6
Message-ID:  <201309110724.r8B7OkdJ037593@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: dteske
Date: Wed Sep 11 07:24:46 2013
New Revision: 255470
URL: http://svnweb.freebsd.org/changeset/base/255470

Log:
  Merge from stable/9 SVN r250927: MFC: r249628, r249742
  
  - recover missing arp_ifinit() call.
  - plug static llentry leak (ipv4 & ipv6 were affected).
  
  PR:		kern/172985
  
  Also merge from stable/9 SVN r240313:
  
  Merge r238990 (manually resolving absence of r237263):
    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:	zont, rstone
    Submitted by:	Eric van Gyzen <eric_van_gyzen dell.com>

Modified:
  stable/8/sys/net/if_llatbl.c
  stable/8/sys/net/if_llatbl.h
  stable/8/sys/net/if_var.h
  stable/8/sys/net/if_vlan.c
  stable/8/sys/netinet/if_ether.c
  stable/8/sys/netinet/in.c
  stable/8/sys/netinet6/in6.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/net/   (props changed)
  stable/8/sys/netinet/   (props changed)
  stable/8/sys/netinet6/   (props changed)

Modified: stable/8/sys/net/if_llatbl.c
==============================================================================
--- stable/8/sys/net/if_llatbl.c	Wed Sep 11 07:11:14 2013	(r255469)
+++ stable/8/sys/net/if_llatbl.c	Wed Sep 11 07:24:46 2013	(r255470)
@@ -109,10 +109,19 @@ llentry_free(struct llentry *lle)
 	size_t pkts_dropped;
 	struct mbuf *next;
 
-	pkts_dropped = 0;
+	IF_AFDATA_WLOCK_ASSERT(lle->lle_tbl->llt_ifp);
 	LLE_WLOCK_ASSERT(lle);
+
+	/* XXX: guard against race with other llentry_free(). */
+	if (!(lle->la_flags & LLE_LINKED)) {
+		LLE_FREE_LOCKED(lle);
+		return (0);
+	}
+
 	LIST_REMOVE(lle, lle_next);
+	lle->la_flags &= ~(LLE_VALID | LLE_LINKED);
 
+	pkts_dropped = 0;
 	while ((lle->la_numheld > 0) && (lle->la_hold != NULL)) {
 		next = lle->la_hold->m_nextpkt;
 		m_freem(lle->la_hold);
@@ -125,7 +134,6 @@ llentry_free(struct llentry *lle)
 		("%s: la_numheld %d > 0, pkts_droped %zd", __func__, 
 		 lle->la_numheld, pkts_dropped));
 
-	lle->la_flags &= ~LLE_VALID;
 	LLE_FREE_LOCKED(lle);
 
 	return (pkts_dropped);
@@ -185,17 +193,16 @@ lltable_free(struct lltable *llt)
 	SLIST_REMOVE(&V_lltables, llt, lltable, llt_link);
 	LLTABLE_WUNLOCK();
 
-	for (i=0; i < LLTBL_HASHTBL_SIZE; i++) {
+	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) {
-			int canceled;
-
-			canceled = callout_drain(&lle->la_timer);
 			LLE_WLOCK(lle);
-			if (canceled)
+			if (callout_stop(&lle->la_timer))
 				LLE_REMREF(lle);
 			llentry_free(lle);
 		}
 	}
+	IF_AFDATA_WUNLOCK(llt->llt_ifp);
 
 	free(llt, M_LLTABLE);
 }

Modified: stable/8/sys/net/if_llatbl.h
==============================================================================
--- stable/8/sys/net/if_llatbl.h	Wed Sep 11 07:11:14 2013	(r255469)
+++ stable/8/sys/net/if_llatbl.h	Wed Sep 11 07:24:46 2013	(r255470)
@@ -97,26 +97,28 @@ struct llentry {
 #define	LLE_ADDREF(lle) do {					\
 	LLE_WLOCK_ASSERT(lle);					\
 	KASSERT((lle)->lle_refcnt >= 0,				\
-		("negative refcnt %d", (lle)->lle_refcnt));	\
+	    ("negative refcnt %d on lle %p",			\
+	    (lle)->lle_refcnt, (lle)));				\
 	(lle)->lle_refcnt++;					\
 } while (0)
 
 #define	LLE_REMREF(lle)	do {					\
 	LLE_WLOCK_ASSERT(lle);					\
-	KASSERT((lle)->lle_refcnt > 1,				\
-		("bogus refcnt %d", (lle)->lle_refcnt));	\
+	KASSERT((lle)->lle_refcnt > 0,				\
+	    ("bogus refcnt %d on lle %p",			\
+	    (lle)->lle_refcnt, (lle)));				\
 	(lle)->lle_refcnt--;					\
 } while (0)
 
 #define	LLE_FREE_LOCKED(lle) do {				\
-	if ((lle)->lle_refcnt <= 1)				\
+	if ((lle)->lle_refcnt == 1)				\
 		(lle)->lle_tbl->llt_free((lle)->lle_tbl, (lle));\
 	else {							\
-		(lle)->lle_refcnt--;				\
+		LLE_REMREF(lle);				\
 		LLE_WUNLOCK(lle);				\
 	}							\
 	/* guard against invalid refs */			\
-	lle = NULL;						\
+	(lle) = NULL;						\
 } while (0)
 
 #define	LLE_FREE(lle) do {					\
@@ -167,9 +169,10 @@ MALLOC_DECLARE(M_LLTABLE);
 #define	LLE_VALID	0x0008	/* ll_addr is valid */
 #define	LLE_PROXY	0x0010	/* proxy entry ??? */
 #define	LLE_PUB		0x0020	/* publish entry ??? */
+#define	LLE_LINKED	0x0040	/* linked to lookup structure */
+#define	LLE_EXCLUSIVE	0x2000	/* return lle xlocked  */
 #define	LLE_DELETE	0x4000	/* delete on a lookup - match LLE_IFADDR */
 #define	LLE_CREATE	0x8000	/* create on a lookup miss */
-#define	LLE_EXCLUSIVE	0x2000	/* return lle xlocked  */
 
 #define LLATBL_HASH(key, mask) \
 	(((((((key >> 8) ^ key) >> 8) ^ key) >> 8) ^ key) & mask)

Modified: stable/8/sys/net/if_var.h
==============================================================================
--- stable/8/sys/net/if_var.h	Wed Sep 11 07:11:14 2013	(r255469)
+++ stable/8/sys/net/if_var.h	Wed Sep 11 07:24:46 2013	(r255470)
@@ -406,6 +406,8 @@ EVENTHANDLER_DECLARE(group_change_event,
 #define	IF_AFDATA_DESTROY(ifp)	rw_destroy(&(ifp)->if_afdata_lock)
 
 #define	IF_AFDATA_LOCK_ASSERT(ifp)	rw_assert(&(ifp)->if_afdata_lock, RA_LOCKED)
+#define	IF_AFDATA_RLOCK_ASSERT(ifp)	rw_assert(&(ifp)->if_afdata_lock, RA_RLOCKED)
+#define	IF_AFDATA_WLOCK_ASSERT(ifp)	rw_assert(&(ifp)->if_afdata_lock, RA_WLOCKED)
 #define	IF_AFDATA_UNLOCK_ASSERT(ifp)	rw_assert(&(ifp)->if_afdata_lock, RA_UNLOCKED)
 
 int	if_handoff(struct ifqueue *ifq, struct mbuf *m, struct ifnet *ifp,

Modified: stable/8/sys/net/if_vlan.c
==============================================================================
--- stable/8/sys/net/if_vlan.c	Wed Sep 11 07:11:14 2013	(r255469)
+++ stable/8/sys/net/if_vlan.c	Wed Sep 11 07:24:46 2013	(r255470)
@@ -41,6 +41,7 @@
 #include <sys/cdefs.h>
 __FBSDID("$FreeBSD$");
 
+#include "opt_inet.h"
 #include "opt_vlan.h"
 
 #include <sys/param.h>
@@ -65,6 +66,11 @@ __FBSDID("$FreeBSD$");
 #include <net/if_vlan_var.h>
 #include <net/vnet.h>
 
+#ifdef INET
+#include <netinet/in.h>
+#include <netinet/if_ether.h>
+#endif
+
 #define VLANNAME	"vlan"
 #define	VLAN_DEF_HWIDTH	4
 #define	VLAN_IFFLAGS	(IFF_BROADCAST | IFF_MULTICAST)

Modified: stable/8/sys/netinet/if_ether.c
==============================================================================
--- stable/8/sys/netinet/if_ether.c	Wed Sep 11 07:11:14 2013	(r255469)
+++ stable/8/sys/netinet/if_ether.c	Wed Sep 11 07:24:46 2013	(r255470)
@@ -167,38 +167,30 @@ arp_ifscrub(struct ifnet *ifp, uint32_t 
 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);
+
+	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);
-			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();
 }
 

Modified: stable/8/sys/netinet/in.c
==============================================================================
--- stable/8/sys/netinet/in.c	Wed Sep 11 07:11:14 2013	(r255469)
+++ stable/8/sys/netinet/in.c	Wed Sep 11 07:24:46 2013	(r255470)
@@ -1350,7 +1350,6 @@ in_lltable_new(const struct sockaddr *l3
 	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.
@@ -1359,7 +1358,10 @@ in_lltable_new(const struct sockaddr *l3
 	lle->l3_addr4 = *(const struct sockaddr_in *)l3addr;
 	lle->base.lle_refcnt = 1;
 	LLE_LOCK_INIT(&lle->base);
-	return &lle->base;
+	callout_init_rw(&lle->base.la_timer, &lle->base.lle_lock,
+	    CALLOUT_RETURNUNLOCKED);
+
+	return (&lle->base);
 }
 
 /*
@@ -1392,7 +1394,8 @@ in_lltable_prefix_free(struct lltable *l
 	register int i;
 	size_t pkts_dropped;
 
-	for (i=0; i < LLTBL_HASHTBL_SIZE; i++) {
+	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) {
 
 		        /* 
@@ -1402,17 +1405,15 @@ in_lltable_prefix_free(struct lltable *l
 			if (IN_ARE_MASKED_ADDR_EQUAL((struct sockaddr_in *)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);
 }
 
 
@@ -1545,15 +1546,20 @@ in_lltable_lookup(struct lltable *llt, u
 
 		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_WUNLOCK(lle);
+			lle->la_flags |= LLE_DELETED;
 #ifdef DIAGNOSTIC
-			log(LOG_INFO, "ifaddr cache = %p  is deleted\n", lle);	
+			log(LOG_INFO, "ifaddr cache = %p is deleted\n", lle);
 #endif
+			if ((lle->la_flags &
+			    (LLE_STATIC | LLE_IFADDR)) == LLE_STATIC)
+				llentry_free(lle);
+			else
+				LLE_WUNLOCK(lle);
 		}
 		lle = (void *)-1;
 		

Modified: stable/8/sys/netinet6/in6.c
==============================================================================
--- stable/8/sys/netinet6/in6.c	Wed Sep 11 07:11:14 2013	(r255469)
+++ stable/8/sys/netinet6/in6.c	Wed Sep 11 07:24:46 2013	(r255470)
@@ -1377,10 +1377,10 @@ in6_purgeaddr(struct ifaddr *ifa)
 	nd6_dad_stop(ifa);
 
 	/* Remove local address entry from lltable. */
-	IF_AFDATA_LOCK(ifp);
-	lla_lookup(LLTABLE6(ifp), (LLE_DELETE | LLE_IFADDR),
-	    (struct sockaddr *)&ia->ia_addr);
-	IF_AFDATA_UNLOCK(ifp);
+	memcpy(&addr, &ia->ia_addr, sizeof(ia->ia_addr));
+	memcpy(&mask, &ia->ia_prefixmask, sizeof(ia->ia_prefixmask));
+	lltable_prefix_free(AF_INET6, (struct sockaddr *)&addr,
+		(struct sockaddr *)&mask, LLE_STATIC);
 
 	/*
 	 * initialize for rtmsg generation
@@ -1393,8 +1393,6 @@ in6_purgeaddr(struct ifaddr *ifa)
 	/* */
 	bzero(&rt0, sizeof(rt0));
 	rt0.rt_gateway = (struct sockaddr *)&gateway;
-	memcpy(&mask, &ia->ia_prefixmask, sizeof(ia->ia_prefixmask));
-	memcpy(&addr, &ia->ia_addr, sizeof(ia->ia_addr));
 	rt_mask(&rt0) = (struct sockaddr *)&mask;
 	rt_key(&rt0) = (struct sockaddr *)&addr;
 	rt0.rt_flags = RTF_HOST | RTF_STATIC;
@@ -2392,23 +2390,22 @@ in6_lltable_prefix_free(struct lltable *
 	 * (flags & LLE_STATIC) means deleting all entries 
 	 * including static ND6 entries
 	 */
-	for (i=0; i < LLTBL_HASHTBL_SIZE; i++) {
+	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) {
 			if (IN6_ARE_MASKED_ADDR_EQUAL(
-				    &((struct sockaddr_in6 *)L3_ADDR(lle))->sin6_addr, 
-				    &pfx->sin6_addr, 
-				    &msk->sin6_addr) &&
-			    ((flags & LLE_STATIC) || !(lle->la_flags & LLE_STATIC))) {
-				int canceled;
-
-				canceled = callout_drain(&lle->la_timer);
+			    &satosin6(L3_ADDR(lle))->sin6_addr,
+			    &pfx->sin6_addr, &msk->sin6_addr) &&
+			    ((flags & LLE_STATIC) ||
+			    !(lle->la_flags & LLE_STATIC))) {
 				LLE_WLOCK(lle);
-				if (canceled)
+				if (callout_stop(&lle->la_timer))
 					LLE_REMREF(lle);
 				llentry_free(lle);
 			}
 		}
 	}
+	IF_AFDATA_WUNLOCK(llt->llt_ifp);
 }
 
 static int
@@ -2500,15 +2497,20 @@ in6_lltable_lookup(struct lltable *llt, 
 
 		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_WUNLOCK(lle);
+			lle->la_flags |= LLE_DELETED;
 #ifdef DIAGNOSTIC
-			log(LOG_INFO, "ifaddr cache = %p  is deleted\n", lle);	
-#endif	
+			log(LOG_INFO, "ifaddr cache = %p is deleted\n", lle);
+#endif
+			if ((lle->la_flags &
+			    (LLE_STATIC | LLE_IFADDR)) == LLE_STATIC)
+				llentry_free(lle);
+			else
+				LLE_WUNLOCK(lle);
 		}
 		lle = (void *)-1;
 	}



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201309110724.r8B7OkdJ037593>