On 17/06/19(Mon) 16:15, Eygene Ryabinkin wrote:
> Sun, Jun 09, 2019 at 11:44:10AM -0300, Martin Pieuchot wrote:
> > On 07/06/19(Fri) 20:50, Eygene Ryabinkin wrote:
> > > The attached patch fixes incoming interface for pf(4) processing
> > > in the case of bridging of multiple VLAN interfaces which have
> > > the same parent iface and unicast packets destined to the bridge
> > > member: we can't rely solely on the destination MAC in determining
> > > proper incoming interface.
> > >
> > > Can it be reviewed and possibly applied? It was verified in my
> > > production environment.
> >
> > Are you suggesting that the order in which bridge members are checked
> > matters?
>
> Yes.
>
> > In that case would it makes sense to apply the so-called unicast
> > check to `ifp', the incoming interface, first?
>
> Right. Had rewritten the patch to do this explicitely: +1 goto,
> -1 (I hope) confusion point about ifp setting.
>
> The bridge with updated patch works as expected already for some hours
> in production: will continue to monitor it.
Diff is correct. The problem with using a goto here is that `bif->ifp'
is only valid inside the loop. At least that how it will become once
the SMR_LIST is converted to a no-locked variant. That's what I have
in my tree.
So I'd suggest we move the bridge_ifinput() call inside the loop and
duplicate the logic for `ifp0'.
In order to reduce the amount of duplicated logic we could start by
moving the filter above. This means that bridge_rtupdate() won't be
executed on blocked packet, which should be fine.
Are you ok with the diff below?
Index: net/if_bridge.c
===================================================================
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.335
diff -u -p -r1.335 if_bridge.c
--- net/if_bridge.c 9 Jun 2019 17:42:16 -0000 1.335
+++ net/if_bridge.c 10 Jul 2019 17:19:24 -0000
@@ -1014,10 +1014,6 @@ bridgeintr_frame(struct ifnet *brifp, st
return;
}
- if (bridge_filterrule(&bif->bif_brlin, &eh, m) == BRL_ACTION_BLOCK) {
- m_freem(m);
- return;
- }
m = bridge_ip(&sc->sc_if, BRIDGE_IN, src_if, &eh, m);
if (m == NULL)
return;
@@ -1197,6 +1193,9 @@ bridge_process(struct ifnet *ifp, struct
return;
}
+ if (bridge_filterrule(&bif->bif_brlin, eh, m) == BRL_ACTION_BLOCK)
+ goto bad;
+
/*
* Unicast, make sure it's not for us.
*/
@@ -1207,11 +1206,6 @@ bridge_process(struct ifnet *ifp, struct
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;
- }
-
/* Count for the bridge */
brifp->if_ipackets++;
brifp->if_ibytes += m->m_pkthdr.len;
