Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 23 Jul 2005 00:00:09 GMT
From:      Robert Watson <rwatson@FreeBSD.org>
To:        Perforce Change Reviews <perforce@freebsd.org>
Subject:   PERFORCE change 80817 for review
Message-ID:  <200507230000.j6N009R2050416@repoman.freebsd.org>

next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=80817

Change 80817 by rwatson@rwatson_zoo on 2005/07/22 23:59:30

	Pass M_ZERO to MALLOC() when allocating multicast addresses.  This
	isn't strictly necessary, but will avoid confusing when looking at
	the structure later in a debugger.
	
	Since we now allow M_NOWAIT to be used throughout the multicast
	code, rearrange the if_addmulti() code to no longer use the "detect
	race and recover" approach, but instead detect allocation failure
	and abort.  This simplifies the logic quite a bit, and avoids having
	lots of memory floating around that may or may not need to be freed.

Affected files ...

.. //depot/projects/netsmp/src/sys/net/if.c#5 edit

Differences ...

==== //depot/projects/netsmp/src/sys/net/if.c#5 (text+ko) ====

@@ -1834,9 +1834,10 @@
 
 	IF_ADDR_LOCK_ASSERT(ifp);
 
-	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link)
+	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
 		if (sa_equal(ifma->ifma_addr, sa))
 			break;
+	}
 
 	return ifma;
 }
@@ -1868,10 +1869,8 @@
 	struct ifmultiaddr *ifma;
 	struct sockaddr *dupsa;
 
-	KASSERT(ifp != NULL, ("if_allocmulti: NULL ifp"));
-	KASSERT(sa != NULL, ("if_allocmulti: NULL sa"));
-
-	MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma, M_IFMADDR, mflags);
+	MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma, M_IFMADDR, mflags |
+	    M_ZERO);
 	if (ifma == NULL)
 		return (NULL);
 
@@ -1883,23 +1882,24 @@
 	bcopy(sa, dupsa, sa->sa_len);
 	ifma->ifma_addr = dupsa;
 
-	if (llsa != NULL) {
-		MALLOC(dupsa, struct sockaddr *, llsa->sa_len, M_IFMADDR,
-		    mflags);
-		if (dupsa == NULL) {
-			FREE(ifma->ifma_addr, M_IFMADDR);
-			FREE(ifma, M_IFMADDR);
-			return (NULL);
-		}
-		bcopy(llsa, dupsa, llsa->sa_len);
-		ifma->ifma_lladdr = llsa;
-	} else
-		ifma->ifma_lladdr = NULL;
-
 	ifma->ifma_ifp = ifp;
 	ifma->ifma_refcount = 1;
 	ifma->ifma_protospec = NULL;
 
+	if (llsa == NULL) {
+		ifma->ifma_lladdr = NULL;
+		return (ifma);
+	}
+
+	MALLOC(dupsa, struct sockaddr *, llsa->sa_len, M_IFMADDR, mflags);
+	if (dupsa == NULL) {
+		FREE(ifma->ifma_addr, M_IFMADDR);
+		FREE(ifma, M_IFMADDR);
+		return (NULL);
+	}
+	bcopy(llsa, dupsa, llsa->sa_len);
+	ifma->ifma_lladdr = dupsa;
+
 	return (ifma);
 }
 
@@ -1946,15 +1946,13 @@
 if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
     struct ifmultiaddr **retifma)
 {
-	struct ifmultiaddr *ifma, *ll_ifma, *new_ifma, *new_ll_ifma;
+	struct ifmultiaddr *ifma, *ll_ifma;
 	struct sockaddr *llsa;
 	int error;
 
 	/*
 	 * If the address is already present, return a new reference to it;
-	 * otherwise, allocate storage and set up a new address.  Since we
-	 * release the interface lock, we have to check for races in which
-	 * another thread may also have set up the same address.
+	 * otherwise, allocate storage and set up a new address.
 	 */
 	IF_ADDR_LOCK(ifp);
 	ifma = if_findmulti(ifp, sa);
@@ -1965,69 +1963,59 @@
 		IF_ADDR_UNLOCK(ifp);
 		return (0);
 	}
-	IF_ADDR_UNLOCK(ifp);
 
 	/*
-	 * The address isn't already present; perform a link layer
-	 * resolution if it's not already a link layer address, and allocate
-	 * and set up the address.  As of this point, if llsa is non-NULL,
-	 * we must free the sockaddr if we don't need it.
+	 * The address isn't already present; resolve the protocol address
+	 * into a link layer address, and then look that up, bump its
+	 * refcount or allocate an ifma for that also.  If 'llsa' was
+	 * returned, we will need to free it later.
 	 */
+	llsa = NULL;
+	ll_ifma = NULL;
 	if (ifp->if_resolvemulti != NULL) {
 		error = ifp->if_resolvemulti(ifp, &llsa, sa);
 		if (error)
-		    return error;
-	} else
-		llsa = NULL;
-
-	new_ifma = if_allocmulti(ifp, sa, llsa, M_NOWAIT);
-	if (new_ifma == NULL) {
-		if (llsa != NULL)
-			free(llsa, M_IFMADDR);
-		return (ENOMEM);
+			goto unlock_out;
 	}
-	if (llsa != NULL) {
-		new_ll_ifma = if_allocmulti(ifp, llsa, NULL, M_NOWAIT);
-		if (new_ll_ifma == NULL) {
-			if_freemulti(new_ifma);
-			if (llsa != NULL)
-				free(llsa, M_IFMADDR);
-			return (ENOMEM);
-		}
-	} else
-		new_ll_ifma = NULL;	/* gcc */
 
 	/*
-	 * Now check to see if we lost the race, and continue inserting if
-	 * not.  Note that we need to separately consider the link layer
-	 * and protocol layer multicast addresses.
+	 * Allocate the new address.  Don't hook it up yet, as we may also
+	 * need to allocate a link layer multicast address.
 	 */
-	IF_ADDR_LOCK(ifp);
-	ifma = if_findmulti(ifp, sa);
-	if (llsa != NULL)
-		ll_ifma = if_findmulti(ifp, llsa);
-	else
-		ll_ifma = NULL;	/* gcc */
-
-	if (ifma != NULL) {
-		if_freemulti(new_ifma);
-		ifma->ifma_refcount++;
-	} else {
-		ifma = new_ifma;
-		TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link);
+	ifma = if_allocmulti(ifp, sa, llsa, M_NOWAIT);
+	if (ifma == NULL) {
+		error = ENOMEM;
+		goto free_llsa_out;
 	}
 
+	/*
+	 * If a link layer address is found, we'll need to see if it's
+	 * already present in the address list, or allocate is as well.
+	 * When this block finishes, the link layer address will be on the
+	 * list.
+	 */
 	if (llsa != NULL) {
-		if (ll_ifma != NULL) {
-			if_freemulti(new_ll_ifma);
-			ll_ifma->ifma_refcount++;
-		} else {
-			ll_ifma = new_ll_ifma;
+		ll_ifma = if_findmulti(ifp, llsa);
+		if (ll_ifma == NULL) {
+			ll_ifma = if_allocmulti(ifp, llsa, NULL, M_NOWAIT);
+			if (ll_ifma == NULL) {
+				if_freemulti(ifma);
+				error = ENOMEM;
+				goto free_llsa_out;
+			}
 			TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ll_ifma,
 			    ifma_link);
-		}
+		} else
+			ll_ifma->ifma_refcount++;
 	}
 
+	/*
+	 * We now have a new multicast address, ifma, and possibly a new or
+	 * referenced link layer address.  Add the primary address to the
+	 * ifnet address list.
+	 */
+	TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link);
+
 	if (retifma != NULL)
 		*retifma = ifma;
 
@@ -2052,7 +2040,16 @@
 
 	if (llsa != NULL)
 		FREE(llsa, M_IFMADDR);
+
 	return (0);
+
+free_llsa_out:
+	if (llsa != NULL)
+		FREE(llsa, M_IFMADDR);
+
+unlock_out:
+	IF_ADDR_UNLOCK(ifp);
+	return (error);
 }
 
 /*
@@ -2078,7 +2075,10 @@
 	}
 
 	sa = ifma->ifma_lladdr;
-	ll_ifma = if_findmulti(ifp, sa);
+	if (sa != NULL)
+		ll_ifma = if_findmulti(ifp, sa);
+	else
+		ll_ifma = NULL;
 
 	/*
 	 * XXXRW: How come we don't announce ll_ifma?



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