tech-net archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: fix for bridge called from hard irq
FYI I commited this patch to HEAD and will request a pullup to netbsd-5.
On Thu, Apr 02, 2009 at 11:24:13AM +0200, Manuel Bouyer wrote:
> 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[];
--
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
NetBSD: 26 ans d'experience feront toujours la difference
--
Home |
Main Index |
Thread Index |
Old Index