hello,
On 2023-12-08 15:09, Vitaliy Makkoveev wrote:
> 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.
I'm now running 7.4+syspatches and the below patch, I'll let you know
if I see any recurrence of the issue.
thanks,
.jh
> 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;
> };
>
>
