tech-net archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

fix for bridge called from hard irq



Hi,
following a discussion on current-users about bridge(4) forwarding packets
in (possibly) hardware interrupt context, I've come up with the attached
patch. What it does is just defer bridge_forward() processing to a
software interrupt, with bridge_input() queuing mbufs for processing
to the ifp->if_snd queue.
AFAIK bridge_input() is the only entry point in bridge(4) which can be
called from a hardware interrupt, all others being called from
software interrupt or thread context. I've added a KASSERT() in
bridge_ipf() to check for this.

I've lightly tested this in a Xen dom0 context, with a wm(4) and xvif
part of the bridge; stp enabled on wm(4) (and also enabled on the cisco
switch at the other end), BRIDGE_IPF and ipf enabled on the bridge with
a few simple rules (including one 'block return-rst').

I'd like to get this commited RSN to try to get it in netbsd-5.
It should fix a panic reported with BRIDGE_IPF and pf on current-users
(I'm not sure this panic is reproductible with ipf).

comments ?

PS: the patch is diff -ub for ease of read, but bridge_forward() is of course
properly indented in my local tree.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--
Index: if_bridge.c
===================================================================
RCS file: /cvsroot/src/sys/net/if_bridge.c,v
retrieving revision 1.62
diff -u -p -u -b -r1.62 if_bridge.c
--- if_bridge.c 15 Jun 2008 16:37:21 -0000      1.62
+++ if_bridge.c 2 Apr 2009 09:12:37 -0000
@@ -97,6 +97,7 @@ __KERNEL_RCSID(0, "$NetBSD: if_bridge.c,
 #include <sys/proc.h>
 #include <sys/pool.h>
 #include <sys/kauth.h>
+#include <sys/cpu.h>
 
 #if NBPFILTER > 0
 #include <net/bpf.h>
@@ -185,7 +186,7 @@ static int  bridge_init(struct ifnet *);
 static void    bridge_stop(struct ifnet *, int);
 static void    bridge_start(struct ifnet *);
 
-static void    bridge_forward(struct bridge_softc *, struct mbuf *m);
+static void    bridge_forward(void *);
 
 static void    bridge_timer(void *);
 
@@ -350,6 +351,13 @@ bridge_clone_create(struct if_clone *ifc
        sc->sc_hold_time = BSTP_DEFAULT_HOLD_TIME;
        sc->sc_filter_flags = 0;
 
+       /* software interrupt to do the work */
+       sc->sc_softintr = softint_establish(SOFTINT_NET, bridge_forward, sc);
+       if (sc->sc_softintr == NULL) {
+               free(sc, M_DEVBUF);
+               return ENOMEM;
+       }
+
        /* Initialize our routing table. */
        bridge_rtable_init(sc);
 
@@ -370,6 +378,7 @@ bridge_clone_create(struct if_clone *ifc
        ifp->if_addrlen = 0;
        ifp->if_dlt = DLT_EN10MB;
        ifp->if_hdrlen = ETHER_HDR_LEN;
+       IFQ_SET_READY(&ifp->if_snd);
 
        if_attach(ifp);
 
@@ -410,6 +419,10 @@ bridge_clone_destroy(struct ifnet *ifp)
        /* Tear down the routing table. */
        bridge_rtable_fini(sc);
 
+
+
+       softint_disestablish(sc->sc_softintr);
+
        free(sc, M_DEVBUF);
 
        return (0);
@@ -1298,11 +1311,24 @@ bridge_start(struct ifnet *ifp)
  *     The forwarding function of the bridge.
  */
 static void
-bridge_forward(struct bridge_softc *sc, struct mbuf *m)
+bridge_forward(void *v)
 {
+       struct bridge_softc *sc = v;
+       struct mbuf *m;
        struct bridge_iflist *bif;
        struct ifnet *src_if, *dst_if;
        struct ether_header *eh;
+       int s;
+
+       if ((sc->sc_if.if_flags & IFF_RUNNING) == 0)
+               return;
+
+       s = splbio();
+       while (1) {
+               IFQ_POLL(&sc->sc_if.if_snd, m);
+               if (m == NULL)
+                       break;
+               IFQ_DEQUEUE(&sc->sc_if.if_snd, m);
 
        src_if = m->m_pkthdr.rcvif;
 
@@ -1316,7 +1342,7 @@ bridge_forward(struct bridge_softc *sc, 
        if (bif == NULL) {
                /* Interface is not a bridge member (anymore?) */
                m_freem(m);
-               return;
+                       continue;
        }
 
        if (bif->bif_flags & IFBIF_STP) {
@@ -1325,7 +1351,7 @@ bridge_forward(struct bridge_softc *sc, 
                case BSTP_IFSTATE_LISTENING:
                case BSTP_IFSTATE_DISABLED:
                        m_freem(m);
-                       return;
+                               continue;
                }
        }
 
@@ -1351,7 +1377,7 @@ bridge_forward(struct bridge_softc *sc, 
        if ((bif->bif_flags & IFBIF_STP) != 0 &&
            bif->bif_state == BSTP_IFSTATE_LEARNING) {
                m_freem(m);
-               return;
+                       continue;
        }
 
        /*
@@ -1367,7 +1393,7 @@ bridge_forward(struct bridge_softc *sc, 
                dst_if = bridge_rtlookup(sc, eh->ether_dhost);
                if (src_if == dst_if) {
                        m_freem(m);
-                       return;
+                               continue;
                }
        } else {
                /* ...forward it to all interfaces. */
@@ -1380,15 +1406,15 @@ bridge_forward(struct bridge_softc *sc, 
            m->m_pkthdr.rcvif, PFIL_IN) != 0) {
                if (m != NULL)
                        m_freem(m);
-               return;
+                       continue;
        }
        if (m == NULL)
-               return;
+                       continue;
 #endif /* PFIL_HOOKS */
 
        if (dst_if == NULL) {
                bridge_broadcast(sc, src_if, m);
-               return;
+                       continue;
        }
 
        /*
@@ -1397,13 +1423,13 @@ bridge_forward(struct bridge_softc *sc, 
         */
        if ((dst_if->if_flags & IFF_RUNNING) == 0) {
                m_freem(m);
-               return;
+                       continue;
        }
        bif = bridge_lookup_member_if(sc, dst_if);
        if (bif == NULL) {
                /* Not a member of the bridge (anymore?) */
                m_freem(m);
-               return;
+                       continue;
        }
 
        if (bif->bif_flags & IFBIF_STP) {
@@ -1411,11 +1437,13 @@ bridge_forward(struct bridge_softc *sc, 
                case BSTP_IFSTATE_DISABLED:
                case BSTP_IFSTATE_BLOCKING:
                        m_freem(m);
-                       return;
+                               continue;
                }
        }
 
        bridge_enqueue(sc, dst_if, m, 1);
+       }
+       splx(s);
 }
 
 /*
@@ -1423,6 +1451,7 @@ bridge_forward(struct bridge_softc *sc, 
  *
  *     Receive input from a member interface.  Queue the packet for
  *     bridging if it is not for us.
+ *     should be called at splbio()
  */
 struct mbuf *
 bridge_input(struct ifnet *ifp, struct mbuf *m)
@@ -1464,12 +1493,17 @@ bridge_input(struct ifnet *ifp, struct m
                 * for bridge processing; return the original packet for
                 * local processing.
                 */
+               if (IF_QFULL(&sc->sc_if.if_snd)) {
+                       IF_DROP(&sc->sc_if.if_snd);
+                       return (m);
+               }
                mc = m_dup(m, 0, M_COPYALL, M_NOWAIT);
                if (mc == NULL)
                        return (m);
 
                /* Perform the bridge forwarding function with the copy. */
-               bridge_forward(sc, mc);
+               IF_ENQUEUE(&sc->sc_if.if_snd, mc);
+               softint_schedule(sc->sc_softintr);
 
                /* Return the original packet for local processing. */
                return (m);
@@ -1517,7 +1551,13 @@ bridge_input(struct ifnet *ifp, struct m
        }
 
        /* Perform the bridge forwarding function. */
-       bridge_forward(sc, m);
+       if (IF_QFULL(&sc->sc_if.if_snd)) {
+               IF_DROP(&sc->sc_if.if_snd);
+               m_freem(m);
+               return (NULL);
+       }
+       IF_ENQUEUE(&sc->sc_if.if_snd, m);
+       softint_schedule(sc->sc_softintr);
 
        return (NULL);
 }
@@ -2003,6 +2043,7 @@ bridge_ipf(void *arg, struct mbuf **mp, 
        /*
         * Check basic packet sanity and run IPF through pfil.
         */
+       KASSERT(!cpu_intr_p());
        switch (ether_type)
        {
        case ETHERTYPE_IP :
Index: if_bridgevar.h
===================================================================
RCS file: /cvsroot/src/sys/net/if_bridgevar.h,v
retrieving revision 1.11
diff -u -p -u -b -r1.11 if_bridgevar.h
--- if_bridgevar.h      9 Jul 2007 21:11:00 -0000       1.11
+++ if_bridgevar.h      2 Apr 2009 09:12:37 -0000
@@ -300,6 +300,7 @@ struct bridge_softc {
        LIST_HEAD(, bridge_rtnode) sc_rtlist;   /* list version of above */
        uint32_t                sc_rthash_key;  /* key for hash */
        uint32_t                sc_filter_flags; /* ipf and flags */
+       void                    *sc_softintr;
 };
 
 extern const uint8_t bstp_etheraddr[];


Home | Main Index | Thread Index | Old Index