Implement reference counting for ifmultiaddr, in_multi, and in6_multi

structures. Detect when ifnet instances are detached from the network stack and perform appropriate cleanup to prevent memory leaks. This has been implemented in such a way as to be backwards ABI compatible. Kernel consumers are changed to use if_delmulti_ifma(); in_delmulti() is unable to detect interface removal by design, as it performs searches on structures which are removed with the interface. With this architectural change, the panics FreeBSD users have experienced with carp and pfsync should be resolved. Obtained from: p4 branch bms_netdev Reviewed by: andre Sponsored by: Garance A Drosehn Idea from: NetBSD MFC after: 1 month
2007-03-20 00:36:10 +00:00 · 2007-03-20 00:36:10 +00:00 · ec002fee99
commit ec002fee99
parent 9239bab2fe
6 changed files with 429 additions and 207 deletions
--- a/sys/net/if.c
+++ b/sys/net/if.c
@ -99,9 +99,18 @@ void	(*ng_ether_link_state_p)(struct ifnet *ifp, int state);

 struct mbuf *(*tbr_dequeue_ptr)(struct ifaltq *, int) = NULL;

+/*
+ * XXX: Style; these should be sorted alphabetically, and unprototyped
+ * static functions should be prototyped. Currently they are sorted by
+ * declaration order.
+ */
 static void	if_attachdomain(void *);
 static void	if_attachdomain1(struct ifnet *);
+static void	if_purgemaddrs(struct ifnet *);
 static int	ifconf(u_long, caddr_t);
+static struct ifmultiaddr *
+		if_findmulti(struct ifnet *, struct sockaddr *);
+static void	if_freemulti(struct ifmultiaddr *);
 static void	if_grow(void);
 static void	if_init(void *);
 static void	if_check(void *);
@ -113,6 +122,7 @@ static void	if_unroute(struct ifnet *, int flag, int fam);
 static void	link_rtrequest(int, struct rtentry *, struct rt_addrinfo *);
 static int	if_rtdel(struct radix_node *, void *);
 static int	ifhwioctl(u_long, struct ifnet *, caddr_t, struct thread *);
+static int	if_delmulti_locked(struct ifnet *, struct ifmultiaddr *, int);
 static void	if_start_deferred(void *context, int pending);
 static void	do_link_state_change(void *, int);
 static int	if_getgroup(struct ifgroupreq *, struct ifnet *);
@ -577,7 +587,7 @@ if_attachdomain1(struct ifnet *ifp)
 }

 /*
- * Remove any network addresses from an interface.
+ * Remove any unicast or broadcast network addresses from an interface.
 */

 void
@ -614,6 +624,21 @@ if_purgeaddrs(struct ifnet *ifp)
 	}
 }

+/*
+ * Remove any multicast network addresses from an interface.
+ */
+static void
+if_purgemaddrs(struct ifnet *ifp)
+{
+	struct ifmultiaddr *ifma;
+	struct ifmultiaddr *next;
+
+	IF_ADDR_LOCK(ifp);
+	TAILQ_FOREACH_SAFE(ifma, &ifp->if_multiaddrs, ifma_link, next)
+		if_delmulti_locked(ifp, ifma, 1);
+	IF_ADDR_UNLOCK(ifp);
+}
+
 /*
 * Detach an interface, removing it from the
 * list of "active" interfaces.
@ -675,6 +700,8 @@ if_detach(struct ifnet *ifp)
 	 */
 	in6_ifdetach(ifp);
 #endif
+	if_purgemaddrs(ifp);
+
 	/*
 	 * Remove link ifaddr pointer and maybe decrement if_index.
 	 * Clean up all addresses.
@ -1686,7 +1713,21 @@ ifhwioctl(u_long cmd, struct ifnet *ifp, caddr_t data, struct thread *td)

 		if (cmd == SIOCADDMULTI) {
 			struct ifmultiaddr *ifma;
-			error = if_addmulti(ifp, &ifr->ifr_addr, &ifma);
+
+			/*
+			 * Userland is only permitted to join groups once
+			 * via the if_addmulti() KPI, because it cannot hold
+			 * struct ifmultiaddr * between calls. It may also
+			 * lose a race while we check if the membership
+			 * already exists.
+			 */
+			IF_ADDR_LOCK(ifp);
+			ifma = if_findmulti(ifp, &ifr->ifr_addr);
+			IF_ADDR_UNLOCK(ifp);
+			if (ifma != NULL)
+				error = EADDRINUSE;
+			else
+				error = if_addmulti(ifp, &ifr->ifr_addr, &ifma);
 		} else {
 			error = if_delmulti(ifp, &ifr->ifr_addr);
 		}
@ -2207,7 +2248,7 @@ static void
 if_freemulti(struct ifmultiaddr *ifma)
 {

-	KASSERT(ifma->ifma_refcount == 1, ("if_freemulti: refcount %d",
+	KASSERT(ifma->ifma_refcount == 0, ("if_freemulti: refcount %d",
 	    ifma->ifma_refcount));
 	KASSERT(ifma->ifma_protospec == NULL,
 	    ("if_freemulti: protospec not NULL"));
@ -2293,6 +2334,7 @@ if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
 		if (ll_ifma == NULL) {
 			ll_ifma = if_allocmulti(ifp, llsa, NULL, M_NOWAIT);
 			if (ll_ifma == NULL) {
+				--ifma->ifma_refcount;
 				if_freemulti(ifma);
 				error = ENOMEM;
 				goto free_llsa_out;
@ -2301,6 +2343,7 @@ if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
 			    ifma_link);
 		} else
 			ll_ifma->ifma_refcount++;
+		ifma->ifma_llifma = ll_ifma;
 	}

 	/*
@ -2316,8 +2359,6 @@ if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
 	/*
 	 * 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);
@ -2347,61 +2388,176 @@ if_addmulti(struct ifnet *ifp, struct sockaddr *sa,
 }

 /*
- * Remove a reference to a multicast address on this interface.  Yell
- * if the request does not match an existing membership.
+ * Delete a multicast group membership by network-layer group address.
+ *
+ * Returns ENOENT if the entry could not be found. If ifp no longer
+ * exists, results are undefined. This entry point should only be used
+ * from subsystems which do appropriate locking to hold ifp for the
+ * duration of the call.
+ * Network-layer protocol domains must use if_delmulti_ifma().
 */
 int
 if_delmulti(struct ifnet *ifp, struct sockaddr *sa)
 {
-	struct ifmultiaddr *ifma, *ll_ifma;
+	struct ifmultiaddr *ifma;
+	int lastref;
+#ifdef INVARIANTS
+	struct ifnet *oifp;
+
+	IFNET_RLOCK();
+	TAILQ_FOREACH(oifp, &ifnet, if_link)
+		if (ifp == oifp)
+			break;
+	if (ifp != oifp)
+		ifp = NULL;
+	IFNET_RUNLOCK();
+
+	KASSERT(ifp != NULL, ("%s: ifnet went away", __func__));
+#endif
+	if (ifp == NULL)
+		return (ENOENT);

 	IF_ADDR_LOCK(ifp);
+	lastref = 0;
 	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;
-	}
-
-	sa = ifma->ifma_lladdr;
-	if (sa != NULL)
-		ll_ifma = if_findmulti(ifp, sa);
-	else
-		ll_ifma = NULL;
-
-	/*
-	 * XXXRW: How come we don't announce ll_ifma?
-	 */
-	rt_newmaddrmsg(RTM_DELMADDR, ifma);
-
-	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 (ifma != NULL)
+		lastref = if_delmulti_locked(ifp, ifma, 0);
 	IF_ADDR_UNLOCK(ifp);

-	/*
-	 * Make sure the interface driver is notified
-	 * in the case of a link layer mcast group being left.
-	 */
-	if (ifp->if_ioctl) {
+	if (ifma == NULL)
+		return (ENOENT);
+
+	if (lastref && ifp->if_ioctl != NULL) {
 		IFF_LOCKGIANT(ifp);
-		(void) (*ifp->if_ioctl)(ifp, SIOCDELMULTI, 0);
+		(void)(*ifp->if_ioctl)(ifp, SIOCDELMULTI, 0);
 		IFF_UNLOCKGIANT(ifp);
 	}

-	return 0;
+	return (0);
+}
+
+/*
+ * Delete a multicast group membership by group membership pointer.
+ * Network-layer protocol domains must use this routine.
+ *
+ * It is safe to call this routine if the ifp disappeared. Callers should
+ * hold IFF_LOCKGIANT() to avoid a LOR in case the hardware needs to be
+ * reconfigured.
+ */
+void
+if_delmulti_ifma(struct ifmultiaddr *ifma)
+{
+	struct ifnet *ifp;
+	int lastref;
+
+	ifp = ifma->ifma_ifp;
+#ifdef DIAGNOSTIC
+	if (ifp == NULL) {
+		printf("%s: ifma_ifp seems to be detached\n", __func__);
+	} else {
+		struct ifnet *oifp;
+
+		IFNET_RLOCK();
+		TAILQ_FOREACH(oifp, &ifnet, if_link)
+			if (ifp == oifp)
+				break;
+		if (ifp != oifp) {
+			printf("%s: ifnet %p disappeared\n", __func__, ifp);
+			ifp = NULL;
+		}
+		IFNET_RUNLOCK();
+	}
+#endif
+	/*
+	 * If and only if the ifnet instance exists: Acquire the address lock.
+	 */
+	if (ifp != NULL)
+		IF_ADDR_LOCK(ifp);
+
+	lastref = if_delmulti_locked(ifp, ifma, 0);
+
+	if (ifp != NULL) {
+		/*
+		 * If and only if the ifnet instance exists:
+		 *  Release the address lock.
+		 *  If the group was left: update the hardware hash filter.
+		 */
+		IF_ADDR_UNLOCK(ifp);
+		if (lastref && ifp->if_ioctl != NULL) {
+			IFF_LOCKGIANT(ifp);
+			(void)(*ifp->if_ioctl)(ifp, SIOCDELMULTI, 0);
+			IFF_UNLOCKGIANT(ifp);
+		}
+	}
+}
+
+/*
+ * Perform deletion of network-layer and/or link-layer multicast address.
+ *
+ * Return 0 if the reference count was decremented.
+ * Return 1 if the final reference was released, indicating that the
+ * hardware hash filter should be reprogrammed.
+ */
+static int
+if_delmulti_locked(struct ifnet *ifp, struct ifmultiaddr *ifma, int detaching)
+{
+	struct ifmultiaddr *ll_ifma;
+
+	if (ifp != NULL && ifma->ifma_ifp != NULL) {
+		KASSERT(ifma->ifma_ifp == ifp,
+		    ("%s: inconsistent ifp %p", __func__, ifp));
+		IF_ADDR_LOCK_ASSERT(ifp);
+	}
+
+	ifp = ifma->ifma_ifp;
+
+	/*
+	 * If the ifnet is detaching, null out references to ifnet,
+	 * so that upper protocol layers will notice, and not attempt
+	 * to obtain locks for an ifnet which no longer exists.
+	 * It is OK to call rt_newmaddrmsg() with a NULL ifp.
+	 */
+	if (detaching) {
+#ifdef DIAGNOSTIC
+		printf("%s: detaching ifnet instance %p\n", __func__, ifp);
+#endif
+		ifma->ifma_ifp = NULL;
+	}
+
+	if (--ifma->ifma_refcount > 0)
+		return 0;
+
+	rt_newmaddrmsg(RTM_DELMADDR, ifma);
+
+	/*
+	 * If this ifma is a network-layer ifma, a link-layer ifma may
+	 * have been associated with it. Release it first if so.
+	 */
+	ll_ifma = ifma->ifma_llifma;
+	if (ll_ifma != NULL) {
+		KASSERT(ifma->ifma_lladdr != NULL,
+		    ("%s: llifma w/o lladdr", __func__));
+		if (detaching)
+			ll_ifma->ifma_ifp = NULL;	/* XXX */
+		if (--ll_ifma->ifma_refcount == 0) {
+			if (ifp != NULL) {
+				TAILQ_REMOVE(&ifp->if_multiaddrs, ll_ifma,
+				    ifma_link);
+			}
+			if_freemulti(ll_ifma);
+		}
+	}
+
+	if (ifp != NULL)
+		TAILQ_REMOVE(&ifp->if_multiaddrs, ifma, ifma_link);
+
+	if_freemulti(ifma);
+
+	/*
+	 * The last reference to this instance of struct ifmultiaddr
+	 * was released; the hardware should be notified of this change.
+	 */
+	return 1;
 }

 /*
--- a/sys/net/if_var.h
+++ b/sys/net/if_var.h
@ -600,8 +600,6 @@ struct ifprefix {
 /*
 * Multicast address structure.  This is analogous to the ifaddr
 * structure except that it keeps track of multicast addresses.
- * Also, the reference count here is a count of requests for this
- * address, not a count of pointers to this structure.
 */
 struct ifmultiaddr {
 	TAILQ_ENTRY(ifmultiaddr) ifma_link; /* queue macro glue */
@ -610,6 +608,7 @@ struct ifmultiaddr {
 	struct	ifnet *ifma_ifp;	/* back-pointer to interface */
 	u_int	ifma_refcount;		/* reference count */
 	void	*ifma_protospec;	/* protocol-specific state, if any */
+	struct	ifmultiaddr *ifma_llifma; /* pointer to ifma for ifma_lladdr */
 };

 #ifdef _KERNEL
@ -667,6 +666,7 @@ int	if_allmulti(struct ifnet *, int);
 struct	ifnet* if_alloc(u_char);
 void	if_attach(struct ifnet *);
 int	if_delmulti(struct ifnet *, struct sockaddr *);
+void	if_delmulti_ifma(struct ifmultiaddr *);
 void	if_detach(struct ifnet *);
 void	if_purgeaddrs(struct ifnet *);
 void	if_down(struct ifnet *);
--- a/sys/netgraph/ng_ether.c
+++ b/sys/netgraph/ng_ether.c
@ -501,7 +501,7 @@ ng_ether_rcvmsg(node_p node, item_p item, hook_p lasthook)
 		case NGM_ETHER_ADD_MULTI:
 		    {
 			struct sockaddr_dl sa_dl;
-			struct ifmultiaddr *ifm;
+			struct ifmultiaddr *ifma;

 			if (msg->header.arglen != ETHER_ADDR_LEN) {
 				error = EINVAL;
@ -513,8 +513,23 @@ ng_ether_rcvmsg(node_p node, item_p item, hook_p lasthook)
 			sa_dl.sdl_alen = ETHER_ADDR_LEN;
 			bcopy((void *)msg->data, LLADDR(&sa_dl),
 			    ETHER_ADDR_LEN);
-			error = if_addmulti(priv->ifp,
-			    (struct sockaddr *)&sa_dl, &ifm);
+			/*
+			 * Netgraph is only permitted to join groups once
+			 * via the if_addmulti() KPI, because it cannot hold
+			 * struct ifmultiaddr * between calls. It may also
+			 * lose a race while we check if the membership
+			 * already exists.
+			 */
+			IF_ADDR_LOCK(priv->ifp);
+			ifma = if_findmulti(priv->ifp,
+			    (struct sockaddr *)&sa_dl);
+			IF_ADDR_UNLOCK(priv->ifp);
+			if (ifma != NULL) {
+				error = EADDRINUSE;
+			} else {
+				error = if_addmulti(priv->ifp,
+				    (struct sockaddr *)&sa_dl, &ifma);
+			}
 			break;
 		    }
 		case NGM_ETHER_DEL_MULTI:
--- a/sys/netinet/in.c
+++ b/sys/netinet/in.c
@ -64,6 +64,7 @@ static int	in_scrubprefix(struct in_ifaddr *);
 static void	in_socktrim(struct sockaddr_in *);
 static int	in_ifinit(struct ifnet *,
 	    struct in_ifaddr *, struct sockaddr_in *, int);
+static void	in_purgemaddrs(struct ifnet *);

 static int subnetsarelocal = 0;
 SYSCTL_INT(_net_inet_ip, OID_AUTO, subnets_are_local, CTLFLAG_RW,
@ -976,120 +977,159 @@ in_broadcast(in, ifp)
 	return (0);
 #undef ia
 }
+
 /*
 * Add an address to the list of IP multicast addresses for a given interface.
 */
 struct in_multi *
-in_addmulti(ap, ifp)
-	register struct in_addr *ap;
-	register struct ifnet *ifp;
+in_addmulti(struct in_addr *ap, struct ifnet *ifp)
 {
-	register struct in_multi *inm;
-	int error;
-	struct sockaddr_in sin;
-	struct ifmultiaddr *ifma;
+	struct in_multi *inm;
+
+	inm = NULL;

 	IFF_LOCKGIANT(ifp);
 	IN_MULTI_LOCK();
-	/*
-	 * Call generic routine to add membership or increment
-	 * refcount.  It wants addresses in the form of a sockaddr,
-	 * so we build one here (being careful to zero the unused bytes).
-	 */
-	bzero(&sin, sizeof sin);
-	sin.sin_family = AF_INET;
-	sin.sin_len = sizeof sin;
-	sin.sin_addr = *ap;
-	error = if_addmulti(ifp, (struct sockaddr *)&sin, &ifma);
-	if (error) {
-		IN_MULTI_UNLOCK();
-		IFF_UNLOCKGIANT(ifp);
-		return 0;
-	}

-	/*
-	 * If ifma->ifma_protospec is null, then if_addmulti() created
-	 * a new record.  Otherwise, we are done.
-	 */
-	if (ifma->ifma_protospec != NULL) {
-		IN_MULTI_UNLOCK();
-		IFF_UNLOCKGIANT(ifp);
-		return ifma->ifma_protospec;
-	}
+	IN_LOOKUP_MULTI(*ap, ifp, inm);
+	if (inm != NULL) {
+		/*
+		 * If we already joined this group, just bump the
+		 * refcount and return it.
+		 */
+		KASSERT(inm->inm_refcount >= 1,
+		    ("%s: bad refcount %d", __func__, inm->inm_refcount));
+		++inm->inm_refcount;
+	} else do {
+		struct sockaddr_in sin;
+		struct ifmultiaddr *ifma;
+		struct in_multi *ninm;
+		int error;

-	inm = (struct in_multi *)malloc(sizeof(*inm), M_IPMADDR,
-	    M_NOWAIT | M_ZERO);
-	if (inm == NULL) {
-		IN_MULTI_UNLOCK();
-		IFF_UNLOCKGIANT(ifp);
-		return (NULL);
-	}
+		bzero(&sin, sizeof sin);
+		sin.sin_family = AF_INET;
+		sin.sin_len = sizeof(struct sockaddr_in);
+		sin.sin_addr = *ap;

-	inm->inm_addr = *ap;
-	inm->inm_ifp = ifp;
-	inm->inm_ifma = ifma;
-	ifma->ifma_protospec = inm;
-	LIST_INSERT_HEAD(&in_multihead, inm, inm_link);
+		/*
+		 * Check if a link-layer group is already associated
+		 * with this network-layer group on the given ifnet.
+		 * If so, bump the refcount on the existing network-layer
+		 * group association and return it.
+		 */
+		error = if_addmulti(ifp, (struct sockaddr *)&sin, &ifma);
+		if (error)
+			break;
+		if (ifma->ifma_protospec != NULL) {
+			inm = (struct in_multi *)ifma->ifma_protospec;
+#ifdef INVARIANTS
+			if (inm->inm_ifma != ifma || inm->inm_ifp != ifp ||
+			    inm->inm_addr.s_addr != ap->s_addr)
+				panic("%s: ifma is inconsistent", __func__);
+#endif
+			++inm->inm_refcount;
+			break;
+		}
+
+		/*
+		 * A new membership is needed; construct it and
+		 * perform the IGMP join.
+		 */
+		ninm = malloc(sizeof(*ninm), M_IPMADDR, M_NOWAIT | M_ZERO);
+		if (ninm == NULL) {
+			if_delmulti_ifma(ifma);
+			break;
+		}
+		ninm->inm_addr = *ap;
+		ninm->inm_ifp = ifp;
+		ninm->inm_ifma = ifma;
+		ninm->inm_refcount = 1;
+		ifma->ifma_protospec = ninm;
+		LIST_INSERT_HEAD(&in_multihead, ninm, inm_link);
+
+		igmp_joingroup(ninm);
+
+		inm = ninm;
+	} while (0);

-	/*
-	 * Let IGMP know that we have joined a new IP multicast group.
-	 */
-	igmp_joingroup(inm);
 	IN_MULTI_UNLOCK();
 	IFF_UNLOCKGIANT(ifp);
+
 	return (inm);
 }

 /*
 * Delete a multicast address record.
+ * It is OK to call this routine if the underlying ifnet went away.
+ *
+ * XXX: To deal with the ifp going away, we cheat; the link-layer code in net
+ * will set ifma_ifp to NULL when the associated ifnet instance is detached
+ * from the system.
+ * The only reason we need to violate layers and check ifma_ifp here at all
+ * is because certain hardware drivers still require Giant to be held,
+ * and it must always be taken before other locks.
 */
 void
-in_delmulti(inm)
-	register struct in_multi *inm;
+in_delmulti(struct in_multi *inm)
 {
 	struct ifnet *ifp;

-	ifp = inm->inm_ifp;
-	IFF_LOCKGIANT(ifp);
+	KASSERT(inm->inm_ifma != NULL, ("%s: no ifma", __func__));
+	ifp = inm->inm_ifma->ifma_ifp;
+
+	if (ifp != NULL) {
+		/*
+		 * Sanity check that netinet's notion of ifp is the
+		 * same as net's.
+		 */
+		KASSERT(inm->inm_ifp == ifp, ("%s: bad ifp", __func__));
+		IFF_LOCKGIANT(ifp);
+	}
+
 	IN_MULTI_LOCK();
 	in_delmulti_locked(inm);
 	IN_MULTI_UNLOCK();
-	IFF_UNLOCKGIANT(ifp);
-}

-void
-in_delmulti_locked(inm)
-	register struct in_multi *inm;
-{
-	struct ifmultiaddr *ifma;
-	struct in_multi my_inm;
-
-	ifma = inm->inm_ifma;
-	my_inm.inm_ifp = NULL ; /* don't send the leave msg */
-	if (ifma->ifma_refcount == 1) {
-		/*
-		 * No remaining claims to this record; let IGMP know that
-		 * we are leaving the multicast group.
-		 * But do it after the if_delmulti() which might reset
-		 * the interface and nuke the packet.
-		 */
-		my_inm = *inm ;
-		ifma->ifma_protospec = NULL;
-		LIST_REMOVE(inm, inm_link);
-		free(inm, M_IPMADDR);
-	}
-	/* XXX - should be separate API for when we have an ifma? */
-	if_delmulti(ifma->ifma_ifp, ifma->ifma_addr);
-	if (my_inm.inm_ifp != NULL)
-		igmp_leavegroup(&my_inm);
+	if (ifp != NULL)
+		IFF_UNLOCKGIANT(ifp);
 }

 /*
- * Delete all multicast address records associated with the ifp.
+ * Delete a multicast address record, with locks held.
+ *
+ * It is OK to call this routine if the ifp went away.
+ * Assumes that caller holds the IN_MULTI lock, and that
+ * Giant was taken before other locks if required by the hardware.
 */
 void
-in_delmulti_ifp(ifp)
-	register struct ifnet *ifp;
+in_delmulti_locked(struct in_multi *inm)
+{
+	struct ifmultiaddr *ifma;
+
+	IN_MULTI_LOCK_ASSERT();
+	KASSERT(inm->inm_refcount >= 1, ("%s: freeing freed inm", __func__));
+
+	if (--inm->inm_refcount == 0) {
+		igmp_leavegroup(inm);
+
+		ifma = inm->inm_ifma;
+		KASSERT(ifma->ifma_protospec == inm,
+		    ("%s: ifma_protospec != inm", __func__));
+		ifma->ifma_protospec = NULL;
+
+		LIST_REMOVE(inm, inm_link);
+		free(inm, M_IPMADDR);
+
+		if_delmulti_ifma(ifma);
+	}
+}
+
+/*
+ * Delete all IPv4 multicast address records, and associated link-layer
+ * multicast address records, associated with ifp.
+ */
+static void
+in_purgemaddrs(struct ifnet *ifp)
 {
 	struct in_multi *inm;
 	struct in_multi *oinm;
@ -1114,5 +1154,5 @@ in_ifdetach(ifp)

 	in_pcbpurgeif0(&ripcbinfo, ifp);
 	in_pcbpurgeif0(&udbinfo, ifp);
-	in_delmulti_ifp(ifp);
+	in_purgemaddrs(ifp);
 }
--- a/sys/netinet/in_var.h
+++ b/sys/netinet/in_var.h
@ -165,6 +165,7 @@ struct in_multi {
 	u_int	inm_timer;		/* IGMP membership report timer */
 	u_int	inm_state;		/*  state of the membership */
 	struct	router_info *inm_rti;	/* router info*/
+	u_int	inm_refcount;		/* reference count */
 };

 #ifdef _KERNEL
@ -248,7 +249,6 @@ struct	route;
 struct	in_multi *in_addmulti(struct in_addr *, struct ifnet *);
 void	in_delmulti(struct in_multi *);
 void	in_delmulti_locked(struct in_multi *);
-void	in_delmulti_ifp(struct ifnet *ifp);
 int	in_control(struct socket *, u_long, caddr_t, struct ifnet *,
 	    struct thread *);
 void	in_rtqdrain(void);
--- a/sys/netinet6/mld6.c
+++ b/sys/netinet6/mld6.c
@ -550,101 +550,112 @@ in6_addmulti(maddr6, ifp, errorp, delay)
 	int *errorp, delay;
 {
 	struct in6_multi *in6m;
-	struct ifmultiaddr *ifma;
-	struct sockaddr_in6 sa6;
-	int	s = splnet();

 	*errorp = 0;
+	in6m = NULL;

-	/*
-	 * Call generic routine to add membership or increment
-	 * refcount.  It wants addresses in the form of a sockaddr,
-	 * so we build one here (being careful to zero the unused bytes).
-	 */
-	bzero(&sa6, sizeof(sa6));
-	sa6.sin6_family = AF_INET6;
-	sa6.sin6_len = sizeof(struct sockaddr_in6);
-	sa6.sin6_addr = *maddr6;
-	*errorp = if_addmulti(ifp, (struct sockaddr *)&sa6, &ifma);
-	if (*errorp) {
-		splx(s);
-		return 0;
-	}
+	IFF_LOCKGIANT(ifp);
+	/*IN6_MULTI_LOCK();*/

-	/*
-	 * If ifma->ifma_protospec is null, then if_addmulti() created
-	 * a new record.  Otherwise, we are done.
-	 */
-	if (ifma->ifma_protospec != NULL) {
-		splx(s);
-		return ifma->ifma_protospec;
-	}
+	IN6_LOOKUP_MULTI(*maddr6, ifp, in6m);
+	if (in6m != NULL) {
+		/*
+		 * If we already joined this group, just bump the
+		 * refcount and return it.
+		 */
+		KASSERT(in6m->in6m_refcount >= 1,
+		    ("%s: bad refcount %d", __func__, in6m->in6m_refcount));
+		++in6m->in6m_refcount;
+	} else do {
+		struct in6_multi *nin6m;
+		struct ifmultiaddr *ifma;
+		struct sockaddr_in6 sa6;

-	/* XXX - if_addmulti uses M_WAITOK.  Can this really be called
-	   at interrupt time?  If so, need to fix if_addmulti. XXX */
-	in6m = (struct in6_multi *)malloc(sizeof(*in6m), M_IP6MADDR, M_NOWAIT);
-	if (in6m == NULL) {
-		splx(s);
-		return (NULL);
-	}
+		bzero(&sa6, sizeof(sa6));
+		sa6.sin6_family = AF_INET6;
+		sa6.sin6_len = sizeof(struct sockaddr_in6);
+		sa6.sin6_addr = *maddr6;

-	bzero(in6m, sizeof *in6m);
-	in6m->in6m_addr = *maddr6;
-	in6m->in6m_ifp = ifp;
-	in6m->in6m_refcount = 1;
-	in6m->in6m_ifma = ifma;
-	ifma->ifma_protospec = in6m;
-	in6m->in6m_timer_ch = malloc(sizeof(*in6m->in6m_timer_ch), M_IP6MADDR,
-	    M_NOWAIT);
-	if (in6m->in6m_timer_ch == NULL) {
-		free(in6m, M_IP6MADDR);
-		splx(s);
-		return (NULL);
-	}
-	LIST_INSERT_HEAD(&in6_multihead, in6m, in6m_entry);
+		*errorp = if_addmulti(ifp, (struct sockaddr *)&sa6, &ifma);
+		if (*errorp)
+			break;

-	callout_init(in6m->in6m_timer_ch, 0);
-	in6m->in6m_timer = delay;
-	if (in6m->in6m_timer > 0) {
-		in6m->in6m_state = MLD_REPORTPENDING;
-		mld_starttimer(in6m);
+		/*
+		 * If ifma->ifma_protospec is null, then if_addmulti() created
+		 * a new record.  Otherwise, bump refcount, and we are done.
+		 */
+		if (ifma->ifma_protospec != NULL) {
+			in6m = ifma->ifma_protospec;
+			++in6m->in6m_refcount;
+			break;
+		}

-		splx(s);
-		return (in6m);
-	}
+		nin6m = malloc(sizeof(*nin6m), M_IP6MADDR, M_NOWAIT | M_ZERO);
+		if (nin6m == NULL) {
+			if_delmulti_ifma(ifma);
+			break;
+		}
+
+		nin6m->in6m_addr = *maddr6;
+		nin6m->in6m_ifp = ifp;
+		nin6m->in6m_refcount = 1;
+		nin6m->in6m_ifma = ifma;
+		ifma->ifma_protospec = nin6m;
+
+		nin6m->in6m_timer_ch = malloc(sizeof(*nin6m->in6m_timer_ch),
+		    M_IP6MADDR, M_NOWAIT);
+		if (nin6m->in6m_timer_ch == NULL) {
+			free(nin6m, M_IP6MADDR);
+			if_delmulti_ifma(ifma);
+			break;
+		}
+
+		LIST_INSERT_HEAD(&in6_multihead, nin6m, in6m_entry);
+
+		callout_init(nin6m->in6m_timer_ch, 0);
+		nin6m->in6m_timer = delay;
+		if (nin6m->in6m_timer > 0) {
+			nin6m->in6m_state = MLD_REPORTPENDING;
+			mld_starttimer(nin6m);
+		}
+
+		mld6_start_listening(nin6m);
+
+		in6m = nin6m;
+
+	} while (0);
+
+	/*IN6_MULTI_UNLOCK();*/
+	IFF_UNLOCKGIANT(ifp);

-	/*
-	 * Let MLD6 know that we have joined a new IPv6 multicast
-	 * group.
-	 */
-	mld6_start_listening(in6m);
-	splx(s);
 	return (in6m);
 }

 /*
 * Delete a multicast address record.
+ *
+ * TODO: Locking, as per netinet.
 */
 void
-in6_delmulti(in6m)
-	struct in6_multi *in6m;
+in6_delmulti(struct in6_multi *in6m)
 {
-	struct ifmultiaddr *ifma = in6m->in6m_ifma;
-	int	s = splnet();
+	struct ifmultiaddr *ifma;

-	if (ifma->ifma_refcount == 1) {
-		/*
-		 * No remaining claims to this record; let MLD6 know
-		 * that we are leaving the multicast group.
-		 */
+	KASSERT(in6m->in6m_refcount >= 1, ("%s: freeing freed in6m", __func__));
+
+	if (--in6m->in6m_refcount == 0) {
 		mld_stoptimer(in6m);
 		mld6_stop_listening(in6m);
+
+		ifma = in6m->in6m_ifma;
+		KASSERT(ifma->ifma_protospec == in6m,
+		    ("%s: ifma_protospec != in6m", __func__));
 		ifma->ifma_protospec = NULL;
+
 		LIST_REMOVE(in6m, in6m_entry);
 		free(in6m->in6m_timer_ch, M_IP6MADDR);
 		free(in6m, M_IP6MADDR);
+
+		if_delmulti_ifma(ifma);
 	}
-	/* XXX - should be separate API for when we have an ifma? */
-	if_delmulti(ifma->ifma_ifp, ifma->ifma_addr);
-	splx(s);
 }