Protect link layer network interface multicast address list manipulation

using ifp->if_addr_mtx:

- Initialize if_addr_mtx when ifnet is initialized.

- Destroy if_addr_mtx when ifnet is torn down.

- Rename ifmaof_ifpforaddr() to if_findmulti(); assert if_addr_mtx.
  Staticize.

- Extract ifmultiaddr allocation and initialization into if_allocmulti();
  accept a 'mflags' argument to indicate whether or not sleeping is
  permitted.  This centralizes error handling and address duplication.

- Extract ifmultiaddr tear-down and deallocation in if_freemulti().

- Re-structure if_addmulti() to hold if_addr_mtx around manipulation of
  the ifnet multicast address list and reference count manipulation.
  Make use of non-sleeping allocations.  Annotate the fact that we only
  generate routing socket events for explicit address addition, not
  implicit link layer address addition.

- Re-structure if_delmulti() to hold if_addr_mtx around manipulation of
  the ifnet multicast address list and reference count manipulation.
  Annotate the lack of a routing socket event for implicit link layer
  address removal.

- De-spl all and sundry.

Problem reported by:	Ed Maste <emaste at phaedrus dot sandvine dot ca>
MFC after:		1 week

2 files changed, 208 insertions, 120 deletions

diff --git a/sys/net/if.c b/sys/net/if.c
index f41b795..391fe9c 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -417,6 +417,8 @@ if_free(struct ifnet *ifp)
 {
 
 	if_free_type(ifp, ifp->if_type);
+
+	IF_ADDR_LOCK_DESTROY(ifp);
 }
 
 void
@@ -460,6 +462,7 @@ if_attach(struct ifnet *ifp)
 	TASK_INIT(&ifp->if_starttask, 0, if_start_deferred, ifp);
 	TASK_INIT(&ifp->if_linktask, 0, do_link_state_change, ifp);
 	IF_AFDATA_LOCK_INIT(ifp);
+	IF_ADDR_LOCK_INIT(ifp);
 	ifp->if_afdata_initialized = 0;
 	IFNET_WLOCK();
 	TAILQ_INSERT_TAIL(&ifnet, ifp, if_link);
@@ -1824,99 +1827,216 @@ if_allmulti(struct ifnet *ifp, int onswitch)
 	return (if_setflag(ifp, IFF_ALLMULTI, 0, &ifp->if_amcount, onswitch));
 }
 
+static struct ifmultiaddr *
+if_findmulti(struct ifnet *ifp, struct sockaddr *sa)
+{
+	struct ifmultiaddr *ifma;
+
+	IF_ADDR_LOCK_ASSERT(ifp);
+
+	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
+		if (sa_equal(ifma->ifma_addr, sa))
+			break;
+	}
+
+	return ifma;
+}
+
 /*
- * Add a multicast listenership to the interface in question.
- * The link layer provides a routine which converts
+ * Allocate a new ifmultiaddr and initialize based on passed arguments.  We
+ * make copies of passed sockaddrs.  The ifmultiaddr will not be added to
+ * the ifnet multicast address list here, so the caller must do that and
+ * other setup work (such as notifying the device driver).  The reference
+ * count is initialized to 1.
  */
-int
-if_addmulti(struct ifnet *ifp, struct sockaddr *sa, struct ifmultiaddr **retifma)
+static struct ifmultiaddr *
+if_allocmulti(struct ifnet *ifp, struct sockaddr *sa, struct sockaddr *llsa,
+    int mflags)
 {
-	struct sockaddr *llsa, *dupsa;
-	int error, s;
 	struct ifmultiaddr *ifma;
+	struct sockaddr *dupsa;
+
+	MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma, M_IFMADDR, mflags |
+	    M_ZERO);
+	if (ifma == NULL)
+		return (NULL);
+
+	MALLOC(dupsa, struct sockaddr *, sa->sa_len, M_IFMADDR, mflags);
+	if (dupsa == NULL) {
+		FREE(ifma, M_IFMADDR);
+		return (NULL);
+	}
+	bcopy(sa, dupsa, sa->sa_len);
+	ifma->ifma_addr = dupsa;
+
+	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);
+}
+
+/*
+ * if_freemulti: free ifmultiaddr structure and possibly attached related
+ * addresses.  The caller is responsible for implementing reference
+ * counting, notifying the driver, handling routing messages, and releasing
+ * any dependent link layer state.
+ */
+static void
+if_freemulti(struct ifmultiaddr *ifma)
+{
+
+	KASSERT(ifma->ifma_refcount == 1, ("if_freemulti: refcount %d",
+	    ifma->ifma_refcount));
+	KASSERT(ifma->ifma_protospec == NULL,
+	    ("if_freemulti: protospec not NULL"));
+
+	if (ifma->ifma_lladdr != NULL)
+		FREE(ifma->ifma_lladdr, M_IFMADDR);
+	FREE(ifma->ifma_addr, M_IFMADDR);
+	FREE(ifma, M_IFMADDR);
+}
+
+/*
+ * Register an additional multicast address with a network interface.
+ *
+ * - If the address is already present, bump the reference count on the
+ *   address and return.
+ * - If the address is not link-layer, look up a link layer address.
+ * - Allocate address structures for one or both addresses, and attach to the
+ *   multicast address list on the interface.  If automatically adding a link
+ *   layer address, the protocol address will own a reference to the link
+ *   layer address, to be freed when it is freed.
+ * - Notify the network device driver of an addition to the multicast address
+ *   list.
+ *
+ * 'sa' points to caller-owned memory with the desired multicast address.
+ *
+ * 'retifma' will be used to return a pointer to the resulting multicast
+ * address reference, if desired.
+ */
+int
+if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
+    struct ifmultiaddr **retifma)
+{
+	struct ifmultiaddr *ifma, *ll_ifma;
+	struct sockaddr *llsa;
+	int error;
 
 	/*
-	 * If the matching multicast address already exists
-	 * then don't add a new one, just add a reference
+	 * If the address is already present, return a new reference to it;
+	 * otherwise, allocate storage and set up a new address.
 	 */
-	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
-		if (sa_equal(sa, ifma->ifma_addr)) {
-			ifma->ifma_refcount++;
-			if (retifma)
-				*retifma = ifma;
-			return 0;
-		}
+	IF_ADDR_LOCK(ifp);
+	ifma = if_findmulti(ifp, sa);
+	if (ifma != NULL) {
+		ifma->ifma_refcount++;
+		if (retifma != NULL)
+			*retifma = ifma;
+		IF_ADDR_UNLOCK(ifp);
+		return (0);
 	}
 
 	/*
-	 * Give the link layer a chance to accept/reject it, and also
-	 * find out which AF_LINK address this maps to, if it isn't one
-	 * already.
+	 * 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;
+		if (error)
+			goto unlock_out;
 	}
 
-	MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma, M_IFMADDR, M_WAITOK);
-	MALLOC(dupsa, struct sockaddr *, sa->sa_len, M_IFMADDR, M_WAITOK);
-	bcopy(sa, dupsa, sa->sa_len);
+	/*
+	 * Allocate the new address.  Don't hook it up yet, as we may also
+	 * need to allocate a link layer multicast address.
+	 */
+	ifma = if_allocmulti(ifp, sa, llsa, M_NOWAIT);
+	if (ifma == NULL) {
+		error = ENOMEM;
+		goto free_llsa_out;
+	}
 
-	ifma->ifma_addr = dupsa;
-	ifma->ifma_lladdr = llsa;
-	ifma->ifma_ifp = ifp;
-	ifma->ifma_refcount = 1;
-	ifma->ifma_protospec = NULL;
-	rt_newmaddrmsg(RTM_NEWMADDR, ifma);
+	/*
+	 * 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) {
+		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++;
+	}
 
 	/*
-	 * Some network interfaces can scan the address list at
-	 * interrupt time; lock them out.
+	 * 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.
 	 */
-	s = splimp();
 	TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link);
-	splx(s);
+
 	if (retifma != NULL)
 		*retifma = ifma;
 
-	if (llsa != NULL) {
-		TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link) {
-			if (sa_equal(ifma->ifma_addr, llsa))
-				break;
-		}
-		if (ifma) {
-			ifma->ifma_refcount++;
-		} else {
-			MALLOC(ifma, struct ifmultiaddr *, sizeof *ifma,
-			       M_IFMADDR, M_WAITOK);
-			MALLOC(dupsa, struct sockaddr *, llsa->sa_len,
-			       M_IFMADDR, M_WAITOK);
-			bcopy(llsa, dupsa, llsa->sa_len);
-			ifma->ifma_addr = dupsa;
-			ifma->ifma_lladdr = NULL;
-			ifma->ifma_ifp = ifp;
-			ifma->ifma_refcount = 1;
-			ifma->ifma_protospec = NULL;
-			s = splimp();
-			TAILQ_INSERT_HEAD(&ifp->if_multiaddrs, ifma, ifma_link);
-			splx(s);
-		}
-	}
+	/*
+	 * Must generate the message while holding the lock so that 'ifma'
+	 * pointer is still valid.
+	 *
+	 * XXXRW: How come we don't announce ll_ifma?
+	 */
+	rt_newmaddrmsg(RTM_NEWMADDR, ifma);
+	IF_ADDR_UNLOCK(ifp);
+
 	/*
 	 * We are certain we have added something, so call down to the
 	 * interface to let them know about it.
 	 */
 	if (ifp->if_ioctl != NULL) {
-		s = splimp();
 		IFF_LOCKGIANT(ifp);
 		(void) (*ifp->if_ioctl)(ifp, SIOCADDMULTI, 0);
 		IFF_UNLOCKGIANT(ifp);
-		splx(s);
 	}
 
-	return 0;
+	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);
 }
 
 /*
@@ -1926,72 +2046,53 @@ if_addmulti(struct ifnet *ifp, struct sockaddr *sa, struct ifmultiaddr **retifma
 int
 if_delmulti(struct ifnet *ifp, struct sockaddr *sa)
 {
-	struct ifmultiaddr *ifma;
-	int s;
+	struct ifmultiaddr *ifma, *ll_ifma;
 
-	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link)
-		if (sa_equal(sa, ifma->ifma_addr))
-			break;
-	if (ifma == NULL)
+	IF_ADDR_LOCK(ifp);
+	ifma = if_findmulti(ifp, sa);
+	if (ifma == NULL) {
+		IF_ADDR_UNLOCK(ifp);
 		return ENOENT;
+	}
 
 	if (ifma->ifma_refcount > 1) {
 		ifma->ifma_refcount--;
+		IF_ADDR_UNLOCK(ifp);
 		return 0;
 	}
 
-	rt_newmaddrmsg(RTM_DELMADDR, ifma);
 	sa = ifma->ifma_lladdr;
-	s = splimp();
-	TAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifma_link);
-	/*
-	 * Make sure the interface driver is notified
-	 * in the case of a link layer mcast group being left.
-	 */
-	if (ifp->if_ioctl && ifma->ifma_addr->sa_family == AF_LINK && sa == 0) {
-		IFF_LOCKGIANT(ifp);
-		(void) (*ifp->if_ioctl)(ifp, SIOCDELMULTI, 0);
-		IFF_UNLOCKGIANT(ifp);
-	}
-	splx(s);
-	free(ifma->ifma_addr, M_IFMADDR);
-	free(ifma, M_IFMADDR);
-	if (sa == NULL)
-		return 0;
+	if (sa != NULL)
+		ll_ifma = if_findmulti(ifp, sa);
+	else
+		ll_ifma = NULL;
 
 	/*
-	 * Now look for the link-layer address which corresponds to
-	 * this network address.  It had been squirreled away in
-	 * ifma->ifma_lladdr for this purpose (so we don't have
-	 * to call ifp->if_resolvemulti() again), and we saved that
-	 * value in sa above.  If some nasty deleted the
-	 * link-layer address out from underneath us, we can deal because
-	 * the address we stored was is not the same as the one which was
-	 * in the record for the link-layer address.  (So we don't complain
-	 * in that case.)
+	 * XXXRW: How come we don't announce ll_ifma?
 	 */
-	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link)
-		if (sa_equal(sa, ifma->ifma_addr))
-			break;
-	if (ifma == NULL)
-		return 0;
+	rt_newmaddrmsg(RTM_DELMADDR, ifma);
 
-	if (ifma->ifma_refcount > 1) {
-		ifma->ifma_refcount--;
-		return 0;
+	TAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifma_link);
+	if_freemulti(ifma);
+
+	if (ll_ifma != NULL) {
+		if (ll_ifma->ifma_refcount == 1) {
+			TAILQ_REMOVE(&ifp->if_multiaddrs, ll_ifma, ifma_link);
+			if_freemulti(ll_ifma);
+		} else
+			ll_ifma->ifma_refcount--;
 	}
+	IF_ADDR_UNLOCK(ifp);
 
-	s = splimp();
-	TAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifma_link);
-	if (ifp->if_ioctl != NULL) {
+	/*
+	 * Make sure the interface driver is notified
+	 * in the case of a link layer mcast group being left.
+	 */
+	if (ifp->if_ioctl) {
 		IFF_LOCKGIANT(ifp);
 		(void) (*ifp->if_ioctl)(ifp, SIOCDELMULTI, 0);
 		IFF_UNLOCKGIANT(ifp);
 	}
-	splx(s);
-	free(ifma->ifma_addr, M_IFMADDR);
-	free(sa, M_IFMADDR);
-	free(ifma, M_IFMADDR);
 
 	return 0;
 }
@@ -2070,18 +2171,6 @@ if_setlladdr(struct ifnet *ifp, const u_char *lladdr, int len)
 	return (0);
 }
 
-struct ifmultiaddr *
-ifmaof_ifpforaddr(struct sockaddr *sa, struct ifnet *ifp)
-{
-	struct ifmultiaddr *ifma;
-
-	TAILQ_FOREACH(ifma, &ifp->if_multiaddrs, ifma_link)
-		if (sa_equal(ifma->ifma_addr, sa))
-			break;
-
-	return ifma;
-}
-
 /*
  * The name argument must be a pointer to storage which will last as
  * long as the interface does.  For physical devices, the result of
diff --git a/sys/net/if_var.h b/sys/net/if_var.h
index 6c4991a..6b3c56c 100644
--- a/sys/net/if_var.h
+++ b/sys/net/if_var.h
@@ -645,7 +645,6 @@ struct	ifaddr *ifa_ifwithnet(struct sockaddr *);
 struct	ifaddr *ifa_ifwithroute(int, struct sockaddr *, struct sockaddr *);
 struct	ifaddr *ifaof_ifpforaddr(struct sockaddr *, struct ifnet *);
 
-struct	ifmultiaddr *ifmaof_ifpforaddr(struct sockaddr *, struct ifnet *);
 int	if_simloop(struct ifnet *ifp, struct mbuf *m, int af, int hlen);
 
 typedef	void *if_com_alloc_t(u_char type, struct ifnet *ifp);