Transitioning if_addr_lock to an rwlock

[John Baldwin]

On Thursday, December 29, 2011 5:55:39 pm Gleb Smirnoff wrote:
> On Thu, Dec 29, 2011 at 03:27:26PM -0500, John Baldwin wrote:
> J> - if_addr_uses.patch     This changes callers of the existing macros to use
> J>                          either read or write locks.  This is the patch that
> J>                          could use the most review.
> 
> Reviewing your patch I've found several problems not introduced by it,
> but already existing, and somewhat related to your patch:
> 
> 1) Unneeded use of _SAFE version of TAILQ:
> 
> igmp.c:3338
> in6.c:1339
> mld6.c:2993

I'll fix these.

> 2) Potential race when dropping a lock inside FOREACH loop:
> 
> igmp.c:2058
> mld6.c:1419
> mld6.c:1704 (this one isn't even a SAFE, so would crash earlier)

These are not easy to fix. :(  Dropping the lock is of course the
wrong thing to do.  However, there are a few layering violations that
make this hard to fix properly.  Actually, we might be able to use
an approach similar to that used in mld_ifdetach() and igmp_ifdetach()
to fix the cancel timers cases.  Patch below
 
> 3) I've found that in6_ifawithifp() doesn't do what it is supposed
> to, as well as uses incorrect locking during this. As last resort
> it should run through global list of addresses, not run throgh the
> ifp one again. Patch attached.

This looks good to me.  Maybe you can get bz@ to review it?

Index: netinet/igmp.c
===================================================================
--- netinet/igmp.c	(revision 229389)
+++ netinet/igmp.c	(working copy)
@@ -2004,7 +2003,7 @@
 {
 	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);
@@ -2050,14 +2049,8 @@
 			 * 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:
@@ -2076,6 +2069,10 @@
 		_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);
+	}
 }
 
 /*
Index: netinet6/mld6.c
===================================================================
--- netinet6/mld6.c	(revision 229389)
+++ netinet6/mld6.c	(working copy)
@@ -1656,7 +1656,7 @@
 {
 	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 @@
 			 * 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 @@
 		}
 	}
 	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);
+	}
 }
 
 /*

-- 
John Baldwin