Improve carp(4) locking:

- Use the carp_sx to serialize not only CARP ioctls, but also carp_attach()
  and carp_detach().
- Use cif_mtx to lock only access to those the linked list.
- These locking changes allow us to do some memory allocations with M_WAITOK
  and also properly call callout_drain() in carp_destroy().
- In carp_attach() assert that ifaddr isn't attached. We always come here
  with a pristine address from in[6]_control().

Reviewed by:	oleg
Sponsored by:	Nginx, Inc.

1 files changed, 39 insertions, 62 deletions

diff --git a/sys/netinet/ip_carp.c b/sys/netinet/ip_carp.c
index ebfe8fd..2fa8820 100644
--- a/sys/netinet/ip_carp.c
+++ b/sys/netinet/ip_carp.c
@@ -256,7 +256,7 @@ SYSCTL_VNET_PCPUSTAT(_net_inet_carp, OID_AUTO, stats, struct carpstats,
 #define	CIF_LOCK(cif)		mtx_lock(&(cif)->cif_mtx)
 #define	CIF_UNLOCK(cif)		mtx_unlock(&(cif)->cif_mtx)
 #define	CIF_FREE(cif)	do {				\
-		CIF_LOCK_ASSERT(cif);			\
+		CIF_LOCK(cif);				\
 		if (TAILQ_EMPTY(&(cif)->cif_vrs))	\
 			carp_free_if(cif);		\
 		else					\
@@ -296,7 +296,6 @@ SYSCTL_VNET_PCPUSTAT(_net_inet_carp, OID_AUTO, stats, struct carpstats,
 static void	carp_input_c(struct mbuf *, struct carp_header *, sa_family_t);
 static struct carp_softc
 		*carp_alloc(struct ifnet *);
-static void	carp_detach_locked(struct ifaddr *);
 static void	carp_destroy(struct carp_softc *);
 static struct carp_if
 		*carp_alloc_if(struct ifnet *);
@@ -1250,8 +1249,6 @@ carp_multicast_setup(struct carp_if *cif, sa_family_t sa)
 	struct ifnet *ifp = cif->cif_ifp;
 	int error = 0;
 
-	CIF_LOCK_ASSERT(cif);
-
 	switch (sa) {
 #ifdef INET
 	case AF_INET:
@@ -1264,9 +1261,7 @@ carp_multicast_setup(struct carp_if *cif, sa_family_t sa)
 
 		imo->imo_membership = (struct in_multi **)malloc(
 		    (sizeof(struct in_multi *) * IP_MIN_MEMBERSHIPS), M_CARP,
-		    M_NOWAIT);
-		if (imo->imo_membership == NULL)
-			return (ENOMEM);
+		    M_WAITOK);
 		imo->imo_mfilters = NULL;
 		imo->imo_max_memberships = IP_MIN_MEMBERSHIPS;
 		imo->imo_multicast_vif = -1;
@@ -1296,9 +1291,7 @@ carp_multicast_setup(struct carp_if *cif, sa_family_t sa)
 
 		im6o->im6o_membership = (struct in6_multi **)malloc(
 		    (sizeof(struct in6_multi *) * IPV6_MIN_MEMBERSHIPS), M_CARP,
-		    M_ZERO | M_NOWAIT);
-		if (im6o->im6o_membership == NULL)
-			return (ENOMEM);
+		    M_ZERO | M_WAITOK);
 		im6o->im6o_mfilters = NULL;
 		im6o->im6o_max_memberships = IPV6_MIN_MEMBERSHIPS;
 		im6o->im6o_multicast_hlim = CARP_DFLTTL;
@@ -1355,7 +1348,8 @@ static void
 carp_multicast_cleanup(struct carp_if *cif, sa_family_t sa)
 {
 
-	CIF_LOCK_ASSERT(cif);
+	sx_assert(&carp_sx, SA_XLOCKED);
+
 	switch (sa) {
 #ifdef INET
 	case AF_INET:
@@ -1504,22 +1498,18 @@ carp_alloc(struct ifnet *ifp)
 	return (sc);
 }
 
-static int
+static void
 carp_grow_ifas(struct carp_softc *sc)
 {
 	struct ifaddr **new;
 
-	CARP_LOCK_ASSERT(sc);
-
-	new = malloc(sc->sc_ifasiz * 2, M_CARP, M_NOWAIT|M_ZERO);
-	if (new == NULL)
-		return (ENOMEM);
+	new = malloc(sc->sc_ifasiz * 2, M_CARP, M_WAITOK | M_ZERO);
+	CARP_LOCK(sc);
 	bcopy(sc->sc_ifas, new, sc->sc_ifasiz);
 	free(sc->sc_ifas, M_CARP);
 	sc->sc_ifas = new;
 	sc->sc_ifasiz *= 2;
-
-	return (0);
+	CARP_UNLOCK(sc);
 }
 
 static void
@@ -1528,17 +1518,20 @@ carp_destroy(struct carp_softc *sc)
 	struct ifnet *ifp = sc->sc_carpdev;
 	struct carp_if *cif = ifp->if_carp;
 
-	CIF_LOCK_ASSERT(cif);
+	sx_assert(&carp_sx, SA_XLOCKED);
+
+	if (sc->sc_suppress)
+		carp_demote_adj(-V_carp_ifdown_adj, "vhid removed");
+	CARP_UNLOCK(sc);
 
+	CIF_LOCK(cif);
 	TAILQ_REMOVE(&cif->cif_vrs, sc, sc_list);
+	CIF_UNLOCK(cif);
 
 	mtx_lock(&carp_mtx);
 	LIST_REMOVE(sc, sc_next);
 	mtx_unlock(&carp_mtx);
 
-	CARP_LOCK(sc);
-	if (sc->sc_suppress)
-		carp_demote_adj(-V_carp_ifdown_adj, "vhid removed");
 	callout_drain(&sc->sc_ad_tmo);
 #ifdef INET
 	callout_drain(&sc->sc_md_tmo);
@@ -1807,8 +1800,7 @@ carp_attach(struct ifaddr *ifa, int vhid)
 	struct carp_softc *sc;
 	int index, error;
 
-	if (ifp->if_carp == NULL)
-		return (ENOPROTOOPT);
+	KASSERT(ifa->ifa_carp == NULL, ("%s: ifa %p attached", __func__, ifa));
 
 	switch (ifa->ifa_addr->sa_family) {
 #ifdef INET
@@ -1822,40 +1814,32 @@ carp_attach(struct ifaddr *ifa, int vhid)
 		return (EPROTOTYPE);
 	}
 
+	sx_xlock(&carp_sx);
+	if (ifp->if_carp == NULL) {
+		sx_xunlock(&carp_sx);
+		return (ENOPROTOOPT);
+	}
+
 	CIF_LOCK(cif);
 	IFNET_FOREACH_CARP(ifp, sc)
 		if (sc->sc_vhid == vhid)
 			break;
+	CIF_UNLOCK(cif);
 	if (sc == NULL) {
-		CIF_UNLOCK(cif);
+		sx_xunlock(&carp_sx);
 		return (ENOENT);
 	}
 
-	if (ifa->ifa_carp) {
-		if (ifa->ifa_carp->sc_vhid != vhid)
-			carp_detach_locked(ifa);
-		else {
-			CIF_UNLOCK(cif);
-			return (0);
-		}
-	}
-
 	error = carp_multicast_setup(cif, ifa->ifa_addr->sa_family);
 	if (error) {
 		CIF_FREE(cif);
+		sx_xunlock(&carp_sx);
 		return (error);
 	}
 
-	CARP_LOCK(sc);
 	index = sc->sc_naddrs + sc->sc_naddrs6 + 1;
 	if (index > sc->sc_ifasiz / sizeof(struct ifaddr *))
-		if ((error = carp_grow_ifas(sc)) != 0) {
-			carp_multicast_cleanup(cif,
-			    ifa->ifa_addr->sa_family);
-			CARP_UNLOCK(sc);
-			CIF_FREE(cif);
-			return (error);
-		}
+		carp_grow_ifas(sc);
 
 	switch (ifa->ifa_addr->sa_family) {
 #ifdef INET
@@ -1873,14 +1857,15 @@ carp_attach(struct ifaddr *ifa, int vhid)
 	}
 
 	ifa_ref(ifa);
+
+	CARP_LOCK(sc);
 	sc->sc_ifas[index - 1] = ifa;
 	ifa->ifa_carp = sc;
-
 	carp_hmac_prepare(sc);
 	carp_sc_state(sc);
-
 	CARP_UNLOCK(sc);
-	CIF_UNLOCK(cif);
+
+	sx_xunlock(&carp_sx);
 
 	return (0);
 }
@@ -1890,25 +1875,14 @@ carp_detach(struct ifaddr *ifa)
 {
 	struct ifnet *ifp = ifa->ifa_ifp;
 	struct carp_if *cif = ifp->if_carp;
-
-	CIF_LOCK(cif);
-	carp_detach_locked(ifa);
-	CIF_FREE(cif);
-}
-
-static void
-carp_detach_locked(struct ifaddr *ifa)
-{
-	struct ifnet *ifp = ifa->ifa_ifp;
-	struct carp_if *cif = ifp->if_carp;
 	struct carp_softc *sc = ifa->ifa_carp;
 	int i, index;
 
 	KASSERT(sc != NULL, ("%s: %p not attached", __func__, ifa));
 
-	CIF_LOCK_ASSERT(cif);
-	CARP_LOCK(sc);
+	sx_xlock(&carp_sx);
 
+	CARP_LOCK(sc);
 	/* Shift array. */
 	index = sc->sc_naddrs + sc->sc_naddrs6;
 	for (i = 0; i < index; i++)
@@ -1943,11 +1917,14 @@ carp_detach_locked(struct ifaddr *ifa)
 	carp_hmac_prepare(sc);
 	carp_sc_state(sc);
 
-	if (sc->sc_naddrs == 0 && sc->sc_naddrs6 == 0) {
-		CARP_UNLOCK(sc);
+	if (sc->sc_naddrs == 0 && sc->sc_naddrs6 == 0)
 		carp_destroy(sc);
-	} else
+	else
 		CARP_UNLOCK(sc);
+
+	CIF_FREE(cif);
+
+	sx_xunlock(&carp_sx);
 }
 
 static void