svn commit: r229420 - in head/sys: netinet netinet6

[John Baldwin]

Author: jhb
Date: Tue Jan  3 20:34:52 2012
New Revision: 229420
URL: http://svn.freebsd.org/changeset/base/229420

Log:
  When cancelling multicast timers on an interface, don't release the
  reference on a group in the leaving state while iterating over the loop.
  Instead, use the same approach used in igmp_ifdetach() and mld_ifdetach()
  of placing the groups to free on pending release list and then releasing
  the references after dropping the IF_ADDR_LOCK.  This closes an ugly race
  where the code was dropping the lock in the middle of iterating over the
  list.  It also fixes some additional potential use-after-free bugs since
  the cancellation routine also applied other changes to the group after
  dropping the reference.  Now those changes are performed before the
  reference is dropped and the group is potentially freed.
  
  Prodded to fix by:	glebius
  Reviewed by:	bz
  MFC after:	1 week

Modified:
  head/sys/netinet/igmp.c
  head/sys/netinet6/mld6.c

Modified: head/sys/netinet/igmp.c
==============================================================================
--- head/sys/netinet/igmp.c	Tue Jan  3 20:28:17 2012	(r229419)
+++ head/sys/netinet/igmp.c	Tue Jan  3 20:34:52 2012	(r229420)
@@ -2003,7 +2003,7 @@ igmp_v3_cancel_link_timers(struct igmp_i
 {
 	struct ifmultiaddr	*ifma;
 	struct ifnet		*ifp;
-	struct in_multi		*inm;
+	struct in_multi		*inm, *tinm;
 
 	CTR3(KTR_IGMPV3, "%s: cancel v3 timers on ifp %p(%s)", __func__,
 	    igi->igi_ifp, igi->igi_ifp->if_xname);
@@ -2049,14 +2049,8 @@ igmp_v3_cancel_link_timers(struct igmp_i
 			 * transition to REPORTING to ensure the host leave
 			 * message is sent upstream to the old querier --
 			 * transition to NOT would lose the leave and race.
-			 *
-			 * SMPNG: Must drop and re-acquire IF_ADDR_LOCK
-			 * around inm_release_locked(), as it is not
-			 * a recursive mutex.
 			 */
-			IF_ADDR_UNLOCK(ifp);
-			inm_release_locked(inm);
-			IF_ADDR_LOCK(ifp);
+			SLIST_INSERT_HEAD(&igi->igi_relinmhead, inm, inm_nrele);
 			/* FALLTHROUGH */
 		case IGMP_G_QUERY_PENDING_MEMBER:
 		case IGMP_SG_QUERY_PENDING_MEMBER:
@@ -2075,6 +2069,10 @@ igmp_v3_cancel_link_timers(struct igmp_i
 		_IF_DRAIN(&inm->inm_scq);
 	}
 	IF_ADDR_UNLOCK(ifp);
+	SLIST_FOREACH_SAFE(inm, &igi->igi_relinmhead, inm_nrele, tinm) {
+		SLIST_REMOVE_HEAD(&igi->igi_relinmhead, inm_nrele);
+		inm_release_locked(inm);
+	}
 }
 
 /*

Modified: head/sys/netinet6/mld6.c
==============================================================================
--- head/sys/netinet6/mld6.c	Tue Jan  3 20:28:17 2012	(r229419)
+++ head/sys/netinet6/mld6.c	Tue Jan  3 20:34:52 2012	(r229420)
@@ -1656,7 +1656,7 @@ mld_v2_cancel_link_timers(struct mld_ifi
 {
 	struct ifmultiaddr	*ifma;
 	struct ifnet		*ifp;
-	struct in6_multi		*inm;
+	struct in6_multi	*inm, *tinm;
 
 	CTR3(KTR_MLD, "%s: cancel v2 timers on ifp %p(%s)", __func__,
 	    mli->mli_ifp, mli->mli_ifp->if_xname);
@@ -1695,14 +1695,9 @@ mld_v2_cancel_link_timers(struct mld_ifi
 			 * If we are leaving the group and switching
 			 * version, we need to release the final
 			 * reference held for issuing the INCLUDE {}.
-			 *
-			 * SMPNG: Must drop and re-acquire IF_ADDR_LOCK
-			 * around in6m_release_locked(), as it is not
-			 * a recursive mutex.
 			 */
-			IF_ADDR_UNLOCK(ifp);
-			in6m_release_locked(inm);
-			IF_ADDR_LOCK(ifp);
+			SLIST_INSERT_HEAD(&mli->mli_relinmhead, inm,
+			    in6m_nrele);
 			/* FALLTHROUGH */
 		case MLD_G_QUERY_PENDING_MEMBER:
 		case MLD_SG_QUERY_PENDING_MEMBER:
@@ -1720,6 +1715,10 @@ mld_v2_cancel_link_timers(struct mld_ifi
 		}
 	}
 	IF_ADDR_UNLOCK(ifp);
+	SLIST_FOREACH_SAFE(inm, &mli->mli_relinmhead, in6m_nrele, tinm) {
+		SLIST_REMOVE_HEAD(&mli->mli_relinmhead, in6m_nrele);
+		in6m_release_locked(inm);
+	}
 }
 
 /*