From 339bbf6205da2b0dae21ecff6e54ee8d3955c4c5 Mon Sep 17 00:00:00 2001 From: mvs Date: Fri, 8 Dec 2023 23:13:40 +0000 Subject: [PATCH] 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@ --- sys/net/if_pflow.c | 52 +++++++++++++++++++++++++++++++++++++++++++--- sys/net/if_pflow.h | 37 +++++++++++++++++++-------------- 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 @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -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 @@ -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; }; -- 2.20.1