Wed, Jul 17, 2019 at 04:35:22PM -0300, Martin Pieuchot wrote: > The diff below does a bit of plumbing to avoid code duplication: > > - rename the original port member descriptor from `bif' to `bif0' > - check for bad source MAC (loop prevention) early > - check for wrongly crafted packet before dereferencing `eh' > > Ok?
OK for me, but I have some "plumbing" corrections to the patched version. Attached. Thank you! -- rea
>From 11b378673f126fb6f1d2d71bf3dc54f9cb66f4ca Mon Sep 17 00:00:00 2001 From: Eygene Ryabinkin <[email protected]> Date: Sun, 21 Jul 2019 13:24:22 +0300 Subject: [PATCH] if_bridge: refactoring - Don't trust "bif" to be NULL just before the if-our-packet test: too much code above, future modifications can break this assumption. - Don't check for 'bif->ifp == ifp' inside if-our-packet test: it means to optimize out already checked bridge_ourether(bif0->ifp, ...) in the "if" branch, but it a. creates inter-relation between this check and the initial code that fills bif0; b. conditionals inside loop mean more work for the CPU branch predictors thus I don't feel that this optimization really worth it. - Put processing for "unicast and not for us" case together [1] and employ the fact that final processing for both multicast and unicast packets going through the bridge is just the same: pass the packet through. [1] ... also freeing processing of "unicast and for us" case from an extra indentation level. Signed-off-by: Eygene Ryabinkin <[email protected]> --- sys/net/if_bridge.c | 44 +++++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c index 25f4aa1485d..09b3b2ebca3 100644 --- a/sys/net/if_bridge.c +++ b/sys/net/if_bridge.c @@ -1111,7 +1111,7 @@ bridge_process(struct ifnet *ifp, struct mbuf *m) { struct ifnet *brifp; struct bridge_softc *sc; - struct bridge_iflist *bif = NULL, *bif0 = NULL; + struct bridge_iflist *bif, *bif0 = NULL; struct ether_header *eh; struct mbuf *mc; #if NBPFILTER > 0 @@ -1191,46 +1191,35 @@ bridge_process(struct ifnet *ifp, struct mbuf *m) goto reenqueue; bridge_ifinput(ifp, mc); - - bridgeintr_frame(brifp, ifp, m); - if_put(brifp); - return; + goto pass_through; } /* * Unicast, make sure it's not for us. */ + bif = NULL; if (bridge_ourether(bif0->ifp, eh->ether_dhost)) { bif = bif0; } else { SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) { - if (bif->ifp == ifp) - continue; if (bridge_ourether(bif->ifp, eh->ether_dhost)) break; } } - if (bif != NULL) { - if (bif0->bif_flags & IFBIF_LEARNING) - bridge_rtupdate(sc, - (struct ether_addr *)&eh->ether_shost, - ifp, 0, IFBAF_DYNAMIC, m); - if (bridge_filterrule(&bif0->bif_brlin, eh, m) == - BRL_ACTION_BLOCK) { - goto bad; - } + if (bif == NULL) + goto pass_through; - /* Count for the bridge */ - brifp->if_ipackets++; - brifp->if_ibytes += m->m_pkthdr.len; + if (bif0->bif_flags & IFBIF_LEARNING) + bridge_rtupdate(sc, (struct ether_addr *)&eh->ether_shost, + ifp, 0, IFBAF_DYNAMIC, m); + if (bridge_filterrule(&bif0->bif_brlin, eh, m) == BRL_ACTION_BLOCK) + goto bad; - ifp = bif->ifp; - goto reenqueue; - } + /* Count for the bridge */ + brifp->if_ipackets++; + brifp->if_ibytes += m->m_pkthdr.len; - bridgeintr_frame(brifp, ifp, m); - if_put(brifp); - return; + ifp = bif->ifp; reenqueue: bridge_ifinput(ifp, m); @@ -1238,6 +1227,11 @@ reenqueue: bad: m_freem(m); if_put(brifp); + return; + +pass_through: + bridgeintr_frame(brifp, ifp, m); + if_put(brifp); } /* -- 2.22.0
