Make `so' dereference safe within pflow_output_process().

sosend() has sleep points, so the kernel lock serialisation within
pflow_output_process() doesn't work as expected. The pflow(4) interface
associated socket `so' could be overwritten by concurrent pflowioctl()
thread.

Introduce pflow(4) interface's `sc_lock' rwlock(9) to make `so'
dereference safe. Since the solock() of udp(4) sockets uses netlock as
backend, the `sc_lock' should be taken first. This expands a little
netlock relocking within pflowioctl().

pflow_sendout_mbuf() called by pflow_output_process(), now called
without kernel lock held, so the mp safe counters_pkt(9) used instead
of manual `if_opackets' increment.

Since if_detach() does partial ifnet destruction, now it can't be called
before we finish pflow_output_process() task, otherwise we introduce use
after free for interface counters. In other hand, we need to deny
pflowioctl() to reschedule pflow_output_process() task. The `sc_dyind'
flag introduced for that.

Tested by Hrvoje Popovski.

ok bluhm@

diff --git a/sys/net/if_pflow.c b/sys/net/if_pflow.c
index 46fcabc..ba90b30 100644 (file)
--- a/sys/net/if_pflow.c
+++ b/sys/net/if_pflow.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_pflow.c,v 1.97 2022/11/11 10:51:46 dlg Exp $       */
+/*     $OpenBSD: if_pflow.c,v 1.98 2022/11/23 15:12:27 mvs Exp $       */
 
 /*
  * Copyright (c) 2011 Florian Obser <florian@narrans.de>
@@ -134,11 +134,11 @@ pflow_output_process(void *arg)
        struct mbuf *m;
 
        mq_delist(&sc->sc_outputqueue, &ml);
-       KERNEL_LOCK();
+       rw_enter_read(&sc->sc_lock);
        while ((m = ml_dequeue(&ml)) != NULL) {
                pflow_sendout_mbuf(sc, m);
        }
-       KERNEL_UNLOCK();
+       rw_exit_read(&sc->sc_lock);
 }
 
 int
@@ -148,6 +148,7 @@ pflow_clone_create(struct if_clone *ifc, int unit)
        struct pflow_softc      *pflowif;
 
        pflowif = malloc(sizeof(*pflowif), M_DEVBUF, M_WAITOK|M_ZERO);
+       rw_init(&pflowif->sc_lock, "pflowlk");
        MGET(pflowif->send_nam, M_WAIT, MT_SONAME);
        pflowif->sc_version = PFLOW_PROTO_DEFAULT;
 
@@ -256,6 +257,7 @@ pflow_clone_create(struct if_clone *ifc, int unit)
        mq_init(&pflowif->sc_outputqueue, 8192, IPL_SOFTNET);
        pflow_setmtu(pflowif, ETHERMTU);
        pflow_init_timeouts(pflowif);
+       if_counters_alloc(ifp);
        if_attach(ifp);
        if_alloc_sadl(ifp);
 
@@ -277,6 +279,7 @@ pflow_clone_destroy(struct ifnet *ifp)
        error = 0;
 
        NET_LOCK();
+       sc->sc_dying = 1;
        SLIST_REMOVE(&pflowif_list, sc, pflow_softc, sc_next);
        NET_UNLOCK();
 
@@ -477,10 +480,17 @@ pflowioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
        struct pflowreq          pflowr;
        int                      error;
 
+       if (sc->sc_dying)
+               return ENXIO;
+
        switch (cmd) {
        case SIOCSIFADDR:
        case SIOCSIFDSTADDR:
        case SIOCSIFFLAGS:
+               /* XXXSMP: enforce lock order */
+               NET_UNLOCK();
+               rw_enter_read(&sc->sc_lock);
+               NET_LOCK();
                if ((ifp->if_flags & IFF_UP) && sc->so != NULL) {
                        ifp->if_flags |= IFF_RUNNING;
                        sc->sc_gcounter=pflowstats.pflow_flows;
@@ -489,6 +499,7 @@ pflowioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
                                pflow_sendout_ipfix_tmpl(sc);
                } else
                        ifp->if_flags &= ~IFF_RUNNING;
+               rw_exit_read(&sc->sc_lock);
                break;
        case SIOCSIFMTU:
                if (ifr->ifr_mtu < PFLOW_MINMTU)
@@ -525,10 +536,13 @@ pflowioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 
                /* XXXSMP breaks atomicity */
                NET_UNLOCK();
+               rw_enter_write(&sc->sc_lock);
                error = pflow_set(sc, &pflowr);
                NET_LOCK();
-               if (error != 0)
+               if (error != 0) {
+                       rw_exit_write(&sc->sc_lock);
                        return (error);
+               }
 
                if ((ifp->if_flags & IFF_UP) && sc->so != NULL) {
                        ifp->if_flags |= IFF_RUNNING;
@@ -537,6 +551,7 @@ pflowioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
                                pflow_sendout_ipfix_tmpl(sc);
                } else
                        ifp->if_flags &= ~IFF_RUNNING;
+               rw_exit_write(&sc->sc_lock);
 
                break;
 
@@ -1198,8 +1213,8 @@ pflow_sendout_ipfix_tmpl(struct pflow_softc *sc)
 int
 pflow_sendout_mbuf(struct pflow_softc *sc, struct mbuf *m)
 {
-       sc->sc_if.if_opackets++;
-       sc->sc_if.if_obytes += m->m_pkthdr.len;
+       counters_pkt(sc->sc_if.if_counters,
+                   ifc_opackets, ifc_obytes, m->m_pkthdr.len);
 
        if (sc->so == NULL) {
                m_freem(m);

diff --git a/sys/net/if_pflow.h b/sys/net/if_pflow.h
index b495ac3..9d75770 100644 (file)
--- a/sys/net/if_pflow.h
+++ b/sys/net/if_pflow.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_pflow.h,v 1.18 2022/08/12 16:38:50 mvs Exp $       */
+/*     $OpenBSD: if_pflow.h,v 1.19 2022/11/23 15:12:27 mvs Exp $       */
 
 /*
  * Copyright (c) 2008 Henning Brauer <henning@openbsd.org>
@@ -169,7 +169,16 @@ struct pflow_ipfix_flow6 {
 
 #ifdef _KERNEL
 
+/*
+ * Locks used to protect struct members and global data
+ *       N       net lock
+ *       p       this pflow_softc' `sc_lock'
+ */
+
 struct pflow_softc {
+       struct rwlock            sc_lock;
+
+       int                      sc_dying;      /* [N] */
        struct ifnet             sc_if;
 
        unsigned int             sc_count;
@@ -185,7 +194,7 @@ struct pflow_softc {
        struct timeout           sc_tmo_tmpl;
        struct mbuf_queue        sc_outputqueue;
        struct task              sc_outputtask;
-       struct socket           *so;
+       struct socket           *so;            /* [p] */
        struct mbuf             *send_nam;
        struct sockaddr         *sc_flowsrc;
        struct sockaddr         *sc_flowdst;