On Thu, Dec 07, 2023 at 11:58:31PM -0500, Johan Huldtgren wrote:
> hello,
>
[skip]
>
> Would this apply against 7.4 or only -current? Machine is currently
> running 7.4+syspatches (and sashan@ patch from earlier in this thread)
> if I need to get on -current that's not a big deal it would just take
> a few more days once I get back home tomorrow.
>
> thanks,
>
> .jh
>
Hi,
No, this diff is applicable for -CURRENT only. The diff below can be
applied to 7.4. It also contains timeout(9) related hunks from
-CURRENT, but they can be assumed as non functional change.
Index: sys/net/if_pflow.c
===================================================================
RCS file: /cvs/src/sys/net/if_pflow.c,v
retrieving revision 1.99
diff -u -p -r1.99 if_pflow.c
--- sys/net/if_pflow.c 13 Apr 2023 02:19:05 -0000 1.99
+++ sys/net/if_pflow.c 8 Dec 2023 11:59:09 -0000
@@ -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>
@@ -71,7 +72,6 @@ void pflow_output_process(void *);
int pflow_clone_create(struct if_clone *, int);
int pflow_clone_destroy(struct ifnet *);
int pflow_set(struct pflow_softc *, struct pflowreq *);
-void pflow_init_timeouts(struct pflow_softc *);
int pflow_calc_mtu(struct pflow_softc *, int, int);
void pflow_setmtu(struct pflow_softc *, int);
int pflowvalidsockaddr(const struct sockaddr *, int);
@@ -148,6 +148,7 @@ pflow_clone_create(struct if_clone *ifc,
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;
@@ -255,7 +256,11 @@ pflow_clone_create(struct if_clone *ifc,
ifp->if_flags &= ~IFF_RUNNING; /* not running, need receiver */
mq_init(&pflowif->sc_outputqueue, 8192, IPL_SOFTNET);
pflow_setmtu(pflowif, ETHERMTU);
- pflow_init_timeouts(pflowif);
+
+ timeout_set_proc(&pflowif->sc_tmo, pflow_timeout, pflowif);
+ timeout_set_proc(&pflowif->sc_tmo6, pflow_timeout6, pflowif);
+ timeout_set_proc(&pflowif->sc_tmo_tmpl, pflow_timeout_tmpl, pflowif);
+
if_counters_alloc(ifp);
if_attach(ifp);
if_alloc_sadl(ifp);
@@ -282,12 +287,10 @@ pflow_clone_destroy(struct ifnet *ifp)
SLIST_REMOVE(&pflowif_list, sc, pflow_softc, sc_next);
NET_UNLOCK();
- if (timeout_initialized(&sc->sc_tmo))
- timeout_del(&sc->sc_tmo);
- if (timeout_initialized(&sc->sc_tmo6))
- timeout_del(&sc->sc_tmo6);
- if (timeout_initialized(&sc->sc_tmo_tmpl))
- timeout_del(&sc->sc_tmo_tmpl);
+ timeout_del(&sc->sc_tmo);
+ timeout_del(&sc->sc_tmo6);
+ timeout_del(&sc->sc_tmo_tmpl);
+
pflow_flush(sc);
task_del(net_tq(ifp->if_index), &sc->sc_outputtask);
taskq_barrier(net_tq(ifp->if_index));
@@ -346,6 +349,8 @@ pflow_set(struct pflow_softc *sc, struct
}
}
+ rw_assert_wrlock(&sc->sc_lock);
+
pflow_flush(sc);
if (pflowr->addrmask & PFLOW_MASK_DSTIP) {
@@ -460,12 +465,27 @@ pflow_set(struct pflow_softc *sc, struct
sc->so = NULL;
}
+ mtx_enter(&sc->sc_mtx);
+
/* error check is above */
if (pflowr->addrmask & PFLOW_MASK_VERSION)
sc->sc_version = pflowr->version;
pflow_setmtu(sc, ETHERMTU);
- pflow_init_timeouts(sc);
+
+ switch (sc->sc_version) {
+ case PFLOW_PROTO_5:
+ timeout_del(&sc->sc_tmo6);
+ timeout_del(&sc->sc_tmo_tmpl);
+ break;
+ case PFLOW_PROTO_10:
+ timeout_add_sec(&sc->sc_tmo_tmpl, PFLOW_TMPL_TIMEOUT);
+ break;
+ default: /* NOTREACHED */
+ break;
+ }
+
+ mtx_leave(&sc->sc_mtx);
return (0);
}
@@ -492,10 +512,12 @@ pflowioctl(struct ifnet *ifp, u_long cmd
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);
@@ -507,19 +529,28 @@ pflowioctl(struct ifnet *ifp, u_long cmd
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))))
@@ -545,9 +576,11 @@ pflowioctl(struct ifnet *ifp, u_long cmd
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);
@@ -560,38 +593,9 @@ pflowioctl(struct ifnet *ifp, u_long cmd
return (0);
}
-void
-pflow_init_timeouts(struct pflow_softc *sc)
-{
- switch (sc->sc_version) {
- case PFLOW_PROTO_5:
- if (timeout_initialized(&sc->sc_tmo6))
- timeout_del(&sc->sc_tmo6);
- if (timeout_initialized(&sc->sc_tmo_tmpl))
- timeout_del(&sc->sc_tmo_tmpl);
- if (!timeout_initialized(&sc->sc_tmo))
- timeout_set_proc(&sc->sc_tmo, pflow_timeout, sc);
- break;
- case PFLOW_PROTO_10:
- if (!timeout_initialized(&sc->sc_tmo_tmpl))
- timeout_set_proc(&sc->sc_tmo_tmpl, pflow_timeout_tmpl,
- sc);
- if (!timeout_initialized(&sc->sc_tmo))
- timeout_set_proc(&sc->sc_tmo, pflow_timeout, sc);
- if (!timeout_initialized(&sc->sc_tmo6))
- timeout_set_proc(&sc->sc_tmo6, pflow_timeout6, sc);
-
- timeout_add_sec(&sc->sc_tmo_tmpl, PFLOW_TMPL_TIMEOUT);
- break;
- default: /* NOTREACHED */
- break;
- }
-}
-
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 -
@@ -638,6 +642,8 @@ pflow_get_mbuf(struct pflow_softc *sc, u
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++;
@@ -807,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 )
@@ -819,6 +826,7 @@ export_pflow(struct pf_state *st)
default: /* NOTREACHED */
break;
}
+ mtx_leave(&sc->sc_mtx);
}
return (0);
@@ -880,6 +888,8 @@ copy_flow_to_m(struct pflow_flow *flow,
{
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);
@@ -904,6 +914,8 @@ copy_flow_ipfix_4_to_m(struct pflow_ipfi
{
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) {
@@ -931,6 +943,8 @@ copy_flow_ipfix_6_to_m(struct pflow_ipfi
{
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) {
@@ -1027,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);
@@ -1037,6 +1052,7 @@ pflow_timeout(void *v)
default: /* NOTREACHED */
break;
}
+ mtx_leave(&sc->sc_mtx);
}
void
@@ -1044,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
@@ -1052,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);
@@ -1069,6 +1090,7 @@ pflow_flush(struct pflow_softc *sc)
default: /* NOTREACHED */
break;
}
+ mtx_leave(&sc->sc_mtx);
}
int
@@ -1079,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)
@@ -1115,6 +1139,8 @@ pflow_sendout_ipfix(struct pflow_softc *
u_int32_t count;
int set_length;
+ MUTEX_ASSERT_LOCKED(&sc->sc_mtx);
+
switch (af) {
case AF_INET:
m = sc->sc_mbuf;
@@ -1174,12 +1200,14 @@ pflow_sendout_ipfix_tmpl(struct pflow_so
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),
@@ -1212,6 +1240,8 @@ pflow_sendout_ipfix_tmpl(struct pflow_so
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);
Index: sys/net/if_pflow.h
===================================================================
RCS file: /cvs/src/sys/net/if_pflow.h,v
retrieving revision 1.19
diff -u -p -r1.19 if_pflow.h
--- sys/net/if_pflow.h 23 Nov 2022 15:12:27 -0000 1.19
+++ sys/net/if_pflow.h 8 Dec 2023 11:59:09 -0000
@@ -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;
};
