Convert all IPv4 and IPv6 multicast memberships into using a STAILQ

instead of a linear array.

The multicast memberships for the inpcb structure are protected by a
non-sleepable lock, INP_WLOCK(), which needs to be dropped when
calling the underlying possibly sleeping if_ioctl() method. When using
a linear array to keep track of multicast memberships, the computed
memory location of the multicast filter may suddenly change, due to
concurrent insertion or removal of elements in the linear array. This
in turn leads to various invalid memory access issues and kernel
panics.

To avoid this problem, put all multicast memberships on a STAILQ based
list. Then the memory location of the IPv4 and IPv6 multicast filters
become fixed during their lifetime and use after free and memory leak
issues are easier to track, for example by: vmstat -m | grep multi

All list manipulation has been factored into inline functions
including some macros, to easily allow for a future hash-list
implementation, if needed.

This patch has been tested by pho@ .

Differential Revision: https://reviews.freebsd.org/D20080
Reviewed by:	markj @
MFC after:	1 week
Sponsored by:	Mellanox Technologies

1 files changed, 21 insertions, 24 deletions

diff --git a/sys/netpfil/pf/if_pfsync.c b/sys/netpfil/pf/if_pfsync.c
index 45b1e090f95c..0566593b7616 100644
--- a/sys/netpfil/pf/if_pfsync.c
+++ b/sys/netpfil/pf/if_pfsync.c
@@ -264,7 +264,7 @@ static void	pfsync_push(struct pfsync_bucket *);
 static void	pfsync_push_all(struct pfsync_softc *);
 static void	pfsyncintr(void *);
 static int	pfsync_multicast_setup(struct pfsync_softc *, struct ifnet *,
-		    void *);
+		    struct in_mfilter *imf);
 static void	pfsync_multicast_cleanup(struct pfsync_softc *);
 static void	pfsync_pointers_init(void);
 static void	pfsync_pointers_uninit(void);
@@ -430,8 +430,7 @@ pfsync_clone_destroy(struct ifnet *ifp)
 	pfsync_drop(sc);
 
 	if_free(ifp);
-	if (sc->sc_imo.imo_membership)
-		pfsync_multicast_cleanup(sc);
+	pfsync_multicast_cleanup(sc);
 	mtx_destroy(&sc->sc_mtx);
 	mtx_destroy(&sc->sc_bulk_mtx);
 
@@ -1373,10 +1372,9 @@ pfsyncioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 
 	case SIOCSETPFSYNC:
 	    {
-		struct ip_moptions *imo = &sc->sc_imo;
+		struct in_mfilter *imf = NULL;
 		struct ifnet *sifp;
 		struct ip *ip;
-		void *mship = NULL;
 
 		if ((error = priv_check(curthread, PRIV_NETINET_PF)) != 0)
 			return (error);
@@ -1396,8 +1394,7 @@ pfsyncioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 		    pfsyncr.pfsyncr_syncpeer.s_addr == 0 ||
 		    pfsyncr.pfsyncr_syncpeer.s_addr ==
 		    htonl(INADDR_PFSYNC_GROUP)))
-			mship = malloc((sizeof(struct in_multi *) *
-			    IP_MIN_MEMBERSHIPS), M_PFSYNC, M_WAITOK | M_ZERO);
+			imf = ip_mfilter_alloc(M_WAITOK, 0, 0);
 
 		PFSYNC_LOCK(sc);
 		if (pfsyncr.pfsyncr_syncpeer.s_addr == 0)
@@ -1419,8 +1416,7 @@ pfsyncioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 			if (sc->sc_sync_if)
 				if_rele(sc->sc_sync_if);
 			sc->sc_sync_if = NULL;
-			if (imo->imo_membership)
-				pfsync_multicast_cleanup(sc);
+			pfsync_multicast_cleanup(sc);
 			PFSYNC_UNLOCK(sc);
 			break;
 		}
@@ -1436,14 +1432,13 @@ pfsyncioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 			PFSYNC_BUCKET_UNLOCK(&sc->sc_buckets[c]);
 		}
 
-		if (imo->imo_membership)
-			pfsync_multicast_cleanup(sc);
+		pfsync_multicast_cleanup(sc);
 
 		if (sc->sc_sync_peer.s_addr == htonl(INADDR_PFSYNC_GROUP)) {
-			error = pfsync_multicast_setup(sc, sifp, mship);
+			error = pfsync_multicast_setup(sc, sifp, imf);
 			if (error) {
 				if_rele(sifp);
-				free(mship, M_PFSYNC);
+				ip_mfilter_free(imf);
 				PFSYNC_UNLOCK(sc);
 				return (error);
 			}
@@ -2353,7 +2348,8 @@ pfsyncintr(void *arg)
 }
 
 static int
-pfsync_multicast_setup(struct pfsync_softc *sc, struct ifnet *ifp, void *mship)
+pfsync_multicast_setup(struct pfsync_softc *sc, struct ifnet *ifp,
+    struct in_mfilter *imf)
 {
 	struct ip_moptions *imo = &sc->sc_imo;
 	int error;
@@ -2361,16 +2357,14 @@ pfsync_multicast_setup(struct pfsync_softc *sc, struct ifnet *ifp, void *mship)
 	if (!(ifp->if_flags & IFF_MULTICAST))
 		return (EADDRNOTAVAIL);
 
-	imo->imo_membership = (struct in_multi **)mship;
-	imo->imo_max_memberships = IP_MIN_MEMBERSHIPS;
 	imo->imo_multicast_vif = -1;
 
 	if ((error = in_joingroup(ifp, &sc->sc_sync_peer, NULL,
-	    &imo->imo_membership[0])) != 0) {
-		imo->imo_membership = NULL;
+	    &imf->imf_inm)) != 0)
 		return (error);
-	}
-	imo->imo_num_memberships++;
+
+	ip_mfilter_init(&imo->imo_head);
+	ip_mfilter_insert(&imo->imo_head, imf);
 	imo->imo_multicast_ifp = ifp;
 	imo->imo_multicast_ttl = PFSYNC_DFLTTL;
 	imo->imo_multicast_loop = 0;
@@ -2382,10 +2376,13 @@ static void
 pfsync_multicast_cleanup(struct pfsync_softc *sc)
 {
 	struct ip_moptions *imo = &sc->sc_imo;
+	struct in_mfilter *imf;
 
-	in_leavegroup(imo->imo_membership[0], NULL);
-	free(imo->imo_membership, M_PFSYNC);
-	imo->imo_membership = NULL;
+	while ((imf = ip_mfilter_first(&imo->imo_head)) != NULL) {
+		ip_mfilter_remove(&imo->imo_head, imf);
+		in_leavegroup(imf->imf_inm, NULL);
+		ip_mfilter_free(imf);
+	}
 	imo->imo_multicast_ifp = NULL;
 }
 
@@ -2404,7 +2401,7 @@ pfsync_detach_ifnet(struct ifnet *ifp)
 		 * is going away. We do need to ensure we don't try to do
 		 * cleanup later.
 		 */
-		sc->sc_imo.imo_membership = NULL;
+		ip_mfilter_init(&sc->sc_imo.imo_head);
 		sc->sc_imo.imo_multicast_ifp = NULL;
 		sc->sc_sync_if = NULL;
 	}