Introduce `sc_mtx' mutex(9) to protect the most of pflow_softc

structure. Protect the `send_nam', `sc_flowsrc' and `sc_flowdst'
pflow_softc members by existing `sc_lock' rwlock(9).

This partially fixes locking inconsistency of pflow_softc. The following
work will be done with separate diffs.

Also, pass `sc' instead of NULL to pflow_get_mbuf() while calling from
pflow_sendout_ipfix_tmpl(). This fixes the NULL dereference.

ok bluhm@

2 files changed, 70 insertions, 19 deletions

diff --git a/sys/net/if_pflow.c b/sys/net/if_pflow.c
index c1d07be296c..0345c7fb966 100644
--- a/sys/net/if_pflow.c
+++ b/sys/net/if_pflow.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_pflow.c,v 1.100 2023/11/09 08:53:20 mvs Exp $	*/
+/*	$OpenBSD: if_pflow.c,v 1.101 2023/12/08 23:13:40 mvs Exp $	*/
 
 /*
  * Copyright (c) 2011 Florian Obser <florian@narrans.de>
@@ -29,6 +29,7 @@
 #include <sys/kernel.h>
 #include <sys/socketvar.h>
 #include <sys/sysctl.h>
+#include <sys/mutex.h>
 
 #include <net/if.h>
 #include <net/if_types.h>
@@ -147,6 +148,7 @@ pflow_clone_create(struct if_clone *ifc, int unit)
 
 	pflowif = malloc(sizeof(*pflowif), M_DEVBUF, M_WAITOK|M_ZERO);
 	rw_init(&pflowif->sc_lock, "pflowlk");
+	mtx_init(&pflowif->sc_mtx, IPL_MPFLOOR);
 	MGET(pflowif->send_nam, M_WAIT, MT_SONAME);
 	pflowif->sc_version = PFLOW_PROTO_DEFAULT;
 
@@ -347,6 +349,8 @@ pflow_set(struct pflow_softc *sc, struct pflowreq *pflowr)
 		}
 	}
 
+	rw_assert_wrlock(&sc->sc_lock);
+
 	pflow_flush(sc);
 
 	if (pflowr->addrmask & PFLOW_MASK_DSTIP) {
@@ -461,6 +465,8 @@ pflow_set(struct pflow_softc *sc, struct pflowreq *pflowr)
 		sc->so = NULL;
 	}
 
+	mtx_enter(&sc->sc_mtx);
+
 	/* error check is above */
 	if (pflowr->addrmask & PFLOW_MASK_VERSION)
 		sc->sc_version = pflowr->version;
@@ -479,6 +485,8 @@ pflow_set(struct pflow_softc *sc, struct pflowreq *pflowr)
 		break;
 	}
 
+	mtx_leave(&sc->sc_mtx);
+
 	return (0);
 }
 
@@ -504,10 +512,12 @@ pflowioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 		NET_LOCK();
 		if ((ifp->if_flags & IFF_UP) && sc->so != NULL) {
 			ifp->if_flags |= IFF_RUNNING;
+			mtx_enter(&sc->sc_mtx);
 			sc->sc_gcounter=pflowstats.pflow_flows;
 			/* send templates on startup */
 			if (sc->sc_version == PFLOW_PROTO_10)
 				pflow_sendout_ipfix_tmpl(sc);
+			mtx_leave(&sc->sc_mtx);
 		} else
 			ifp->if_flags &= ~IFF_RUNNING;
 		rw_exit_read(&sc->sc_lock);
@@ -519,19 +529,28 @@ pflowioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 			ifr->ifr_mtu = MCLBYTES;
 		if (ifr->ifr_mtu < ifp->if_mtu)
 			pflow_flush(sc);
+		mtx_enter(&sc->sc_mtx);
 		pflow_setmtu(sc, ifr->ifr_mtu);
+		mtx_leave(&sc->sc_mtx);
 		break;
 
 	case SIOCGETPFLOW:
 		bzero(&pflowr, sizeof(pflowr));
 
+		/* XXXSMP: enforce lock order */
+		NET_UNLOCK();
+		rw_enter_read(&sc->sc_lock);
+		NET_LOCK();
 		if (sc->sc_flowsrc != NULL)
 			memcpy(&pflowr.flowsrc, sc->sc_flowsrc,
 			    sc->sc_flowsrc->sa_len);
 		if (sc->sc_flowdst != NULL)
 			memcpy(&pflowr.flowdst, sc->sc_flowdst,
 			    sc->sc_flowdst->sa_len);
+		rw_exit_read(&sc->sc_lock);
+		mtx_enter(&sc->sc_mtx);
 		pflowr.version = sc->sc_version;
+		mtx_leave(&sc->sc_mtx);
 
 		if ((error = copyout(&pflowr, ifr->ifr_data,
 		    sizeof(pflowr))))
@@ -557,9 +576,11 @@ pflowioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 
 		if ((ifp->if_flags & IFF_UP) && sc->so != NULL) {
 			ifp->if_flags |= IFF_RUNNING;
+			mtx_enter(&sc->sc_mtx);
 			sc->sc_gcounter=pflowstats.pflow_flows;
 			if (sc->sc_version == PFLOW_PROTO_10)
 				pflow_sendout_ipfix_tmpl(sc);
+			mtx_leave(&sc->sc_mtx);
 		} else
 			ifp->if_flags &= ~IFF_RUNNING;
 		rw_exit_write(&sc->sc_lock);
@@ -575,7 +596,6 @@ pflowioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 int
 pflow_calc_mtu(struct pflow_softc *sc, int mtu, int hdrsz)
 {
-
 	sc->sc_maxcount4 = (mtu - hdrsz -
 	    sizeof(struct udpiphdr)) / sizeof(struct pflow_ipfix_flow4);
 	sc->sc_maxcount6 = (mtu - hdrsz -
@@ -622,6 +642,8 @@ pflow_get_mbuf(struct pflow_softc *sc, u_int16_t set_id)
 	struct pflow_header	 h;
 	struct mbuf		*m;
 
+	MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
+
 	MGETHDR(m, M_DONTWAIT, MT_DATA);
 	if (m == NULL) {
 		pflowstats.pflow_onomem++;
@@ -791,6 +813,7 @@ export_pflow(struct pf_state *st)
 	sk = st->key[st->direction == PF_IN ? PF_SK_WIRE : PF_SK_STACK];
 
 	SLIST_FOREACH(sc, &pflowif_list, sc_next) {
+		mtx_enter(&sc->sc_mtx);
 		switch (sc->sc_version) {
 		case PFLOW_PROTO_5:
 			if( sk->af == AF_INET )
@@ -803,6 +826,7 @@ export_pflow(struct pf_state *st)
 		default: /* NOTREACHED */
 			break;
 		}
+		mtx_leave(&sc->sc_mtx);
 	}
 
 	return (0);
@@ -864,6 +888,8 @@ copy_flow_to_m(struct pflow_flow *flow, struct pflow_softc *sc)
 {
 	int		ret = 0;
 
+	MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
+
 	if (sc->sc_mbuf == NULL) {
 		if ((sc->sc_mbuf = pflow_get_mbuf(sc, 0)) == NULL)
 			return (ENOBUFS);
@@ -888,6 +914,8 @@ copy_flow_ipfix_4_to_m(struct pflow_ipfix_flow4 *flow, struct pflow_softc *sc)
 {
 	int		ret = 0;
 
+	MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
+
 	if (sc->sc_mbuf == NULL) {
 		if ((sc->sc_mbuf =
 		    pflow_get_mbuf(sc, PFLOW_IPFIX_TMPL_IPV4_ID)) == NULL) {
@@ -915,6 +943,8 @@ copy_flow_ipfix_6_to_m(struct pflow_ipfix_flow6 *flow, struct pflow_softc *sc)
 {
 	int		ret = 0;
 
+	MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
+
 	if (sc->sc_mbuf6 == NULL) {
 		if ((sc->sc_mbuf6 =
 		    pflow_get_mbuf(sc, PFLOW_IPFIX_TMPL_IPV6_ID)) == NULL) {
@@ -1011,6 +1041,7 @@ pflow_timeout(void *v)
 {
 	struct pflow_softc	*sc = v;
 
+	mtx_enter(&sc->sc_mtx);
 	switch (sc->sc_version) {
 	case PFLOW_PROTO_5:
 		pflow_sendout_v5(sc);
@@ -1021,6 +1052,7 @@ pflow_timeout(void *v)
 	default: /* NOTREACHED */
 		break;
 	}
+	mtx_leave(&sc->sc_mtx);
 }
 
 void
@@ -1028,7 +1060,9 @@ pflow_timeout6(void *v)
 {
 	struct pflow_softc	*sc = v;
 
+	mtx_enter(&sc->sc_mtx);
 	pflow_sendout_ipfix(sc, AF_INET6);
+	mtx_leave(&sc->sc_mtx);
 }
 
 void
@@ -1036,12 +1070,15 @@ pflow_timeout_tmpl(void *v)
 {
 	struct pflow_softc	*sc = v;
 
+	mtx_enter(&sc->sc_mtx);
 	pflow_sendout_ipfix_tmpl(sc);
+	mtx_leave(&sc->sc_mtx);
 }
 
 void
 pflow_flush(struct pflow_softc *sc)
 {
+	mtx_enter(&sc->sc_mtx);
 	switch (sc->sc_version) {
 	case PFLOW_PROTO_5:
 		pflow_sendout_v5(sc);
@@ -1053,6 +1090,7 @@ pflow_flush(struct pflow_softc *sc)
 	default: /* NOTREACHED */
 		break;
 	}
+	mtx_leave(&sc->sc_mtx);
 }
 
 int
@@ -1063,6 +1101,8 @@ pflow_sendout_v5(struct pflow_softc *sc)
 	struct ifnet		*ifp = &sc->sc_if;
 	struct timespec		tv;
 
+	MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
+
 	timeout_del(&sc->sc_tmo);
 
 	if (m == NULL)
@@ -1099,6 +1139,8 @@ pflow_sendout_ipfix(struct pflow_softc *sc, sa_family_t af)
 	u_int32_t			 count;
 	int				 set_length;
 
+	MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
+
 	switch (af) {
 	case AF_INET:
 		m = sc->sc_mbuf;
@@ -1158,12 +1200,14 @@ pflow_sendout_ipfix_tmpl(struct pflow_softc *sc)
 	struct pflow_v10_header		*h10;
 	struct ifnet			*ifp = &sc->sc_if;
 
+	MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
+
 	timeout_del(&sc->sc_tmo_tmpl);
 
 	if (!(ifp->if_flags & IFF_RUNNING)) {
 		return (0);
 	}
-	m = pflow_get_mbuf(NULL, 0);
+	m = pflow_get_mbuf(sc, 0);
 	if (m == NULL)
 		return (0);
 	if (m_copyback(m, 0, sizeof(struct pflow_ipfix_tmpl),
@@ -1196,6 +1240,8 @@ pflow_sendout_ipfix_tmpl(struct pflow_softc *sc)
 int
 pflow_sendout_mbuf(struct pflow_softc *sc, struct mbuf *m)
 {
+	rw_assert_anylock(&sc->sc_lock);
+
 	counters_pkt(sc->sc_if.if_counters,
 	            ifc_opackets, ifc_obytes, m->m_pkthdr.len);
 
diff --git a/sys/net/if_pflow.h b/sys/net/if_pflow.h
index 9d7577074c0..430ddb3c0d2 100644
--- a/sys/net/if_pflow.h
+++ b/sys/net/if_pflow.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_pflow.h,v 1.19 2022/11/23 15:12:27 mvs Exp $	*/
+/*	$OpenBSD: if_pflow.h,v 1.20 2023/12/08 23:13:40 mvs Exp $	*/
 
 /*
  * Copyright (c) 2008 Henning Brauer <henning@openbsd.org>
@@ -171,37 +171,42 @@ struct pflow_ipfix_flow6 {
 
 /*
  * Locks used to protect struct members and global data
+ *       I       immutable after creation
  *       N       net lock
+ *       m       this pflow_softc' `sc_mtx'
  *       p       this pflow_softc' `sc_lock'
  */
 
 struct pflow_softc {
+	struct mutex		 sc_mtx;
 	struct rwlock		 sc_lock;
 
 	int			 sc_dying;	/* [N] */
 	struct ifnet		 sc_if;
 
-	unsigned int		 sc_count;
-	unsigned int		 sc_count4;
-	unsigned int		 sc_count6;
-	unsigned int		 sc_maxcount;
-	unsigned int		 sc_maxcount4;
-	unsigned int		 sc_maxcount6;
-	u_int64_t		 sc_gcounter;
-	u_int32_t		 sc_sequence;
+	unsigned int		 sc_count;	/* [m] */
+	unsigned int		 sc_count4;	/* [m] */
+	unsigned int		 sc_count6;	/* [m] */
+	unsigned int		 sc_maxcount;	/* [m] */
+	unsigned int		 sc_maxcount4;	/* [m] */
+	unsigned int		 sc_maxcount6;	/* [m] */
+	u_int64_t		 sc_gcounter;	/* [m] */
+	u_int32_t		 sc_sequence;	/* [m] */
 	struct timeout		 sc_tmo;
 	struct timeout		 sc_tmo6;
 	struct timeout		 sc_tmo_tmpl;
 	struct mbuf_queue	 sc_outputqueue;
 	struct task		 sc_outputtask;
 	struct socket		*so;		/* [p] */
-	struct mbuf		*send_nam;
-	struct sockaddr		*sc_flowsrc;
-	struct sockaddr		*sc_flowdst;
-	struct pflow_ipfix_tmpl	 sc_tmpl_ipfix;
-	u_int8_t		 sc_version;
-	struct mbuf		*sc_mbuf;	/* current cumulative mbuf */
-	struct mbuf		*sc_mbuf6;	/* current cumulative mbuf */
+	struct mbuf		*send_nam;	/* [p] */
+	struct sockaddr		*sc_flowsrc;	/* [p] */
+	struct sockaddr		*sc_flowdst;	/* [p] */
+	struct pflow_ipfix_tmpl	 sc_tmpl_ipfix;	/* [I] */
+	u_int8_t		 sc_version;	/* [m] */
+	struct mbuf		*sc_mbuf;	/* [m] current cumulative
+						    mbuf */
+	struct mbuf		*sc_mbuf6;	/* [m] current cumulative
+						    mbuf */
 	SLIST_ENTRY(pflow_softc) sc_next;
 };