Date: Wed, 7 Mar 2007 09:36:07 +0300 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Roman Kurakin <rik@inse.ru> Cc: rik@FreeBSD.org, andre@FreeBSD.org, freebsd-net@freebsd.org, glebius@FreeBSD.org, thompsa@FreeBSD.org, bms@FreeBSD.org Subject: Re: kern/109815: wrong interface identifier at pfil_hooks for vlans + if_bridge Message-ID: <20070307063606.GU57456@codelabs.ru> In-Reply-To: <45EDA348.3030309@inse.ru> References: <E1HNbWw-000LoF-Bo@pobox.codelabs.ru> <45E9F1E8.2000802@inse.ru> <20070304062203.GL80319@codelabs.ru> <E1HNbWw-000LoF-Bo@pobox.codelabs.ru> <45E9F1E8.2000802@inse.ru> <20070304160613.GN80319@codelabs.ru> <45EDA348.3030309@inse.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
Roman, good day!
> Lets assume that we have two interfaces that are joined to bridge.
> I'll call them as A and B. Both has a distinct MACs.
>
> Now we have two cases for behaviour of filtering. The first case that
> we will behave like real hub between the physical interfaces and its
> virtual logical representation. The packet will arise on logical interface
> for which it is intended for (according to dst MAC). That is how it
> is implemented now. So now we can filter packets treating dst interface
> as incoming, but not the interfaces it comes through physically.
Yes, I've finally got the idea, thanks!
> The second variant to do visa versa, e q filter packets according interfaces
> they physically come in, and do not take in to account the dst MAC.
>
> Both variant have bad sides and good sides. Both are not wrong from the
> filtering point of view. So I do not want to discuss which one is correct.
I can add only one thing: I am already did the patch to implement
filtering on the physical incoming interface as well as on the logical
one. It is working quite well for the week on a rather busy filtering
bridge that is serving around 60 hosts. The additional behaviour is
controlled by a new sysctl, so it can be easily switched on and off.
> Now about the problem. Lets assume that we also have a C interfaces,
> but with VLANs or anything that will bring the same situation. I'll call
> VLANs C1,C2...CN.
>
> Now we can't implement the first case without a problems, cause all Cx
> will have the same MAC, MAC of the C interface. In current implementation
> we will take the first interface on the list, which would be the last added
> to the bridge. This problem could be fixed. Be patient, I'll describe all
> cases, but not at once. Read till the end of the letter.
>
> If I understand right the Eygene's patch solves problem by introducing the
> second behaviour. It is not quite good cause it will change the old known
> behaviour. Here is the patch itself:
>
> --- if_bridge.c.orig Fri Mar 2 18:23:56 2007
> +++ if_bridge.c Sat Mar 3 05:04:38 2007
> @@ -2122,7 +2122,11 @@
> LIST_FOREACH(bif2, &sc->sc_iflist, bif_next) {
> if (bif2->bif_ifp->if_type == IFT_GIF)
> continue;
> - /* It is destined for us. */
> + /* It is destined for us. We should not rely on the
> + * found interface's MAC as the interface identifier,
> + * because vlanX interfaces are sharing their MAC
> + * with the parent. Moreover, we do know the interface
> + * the packet is coming from. So we're using it. */
> if (memcmp(IF_LLADDR(bif2->bif_ifp), eh->ether_dhost,
> ETHER_ADDR_LEN) == 0
> #ifdef DEV_CARP
> @@ -2133,7 +2137,7 @@
> if (bif->bif_flags & IFBIF_LEARNING)
> (void) bridge_rtupdate(sc,
> eh->ether_shost, bif, 0, IFBAF_DYNAMIC);
> - m->m_pkthdr.rcvif = bif2->bif_ifp;
> + m->m_pkthdr.rcvif = ifp;
> BRIDGE_UNLOCK(sc);
> return (m);
> }
>
>
> I suggest to fix this problem in the other way, by checking if the physical
> interface is the dst interface by MAC. Eq if we got packet from Ci,
> it will be market as received from Ci, not from Cj. Yes it will add
> double checking for this interface it is not the dst with some
> probability, but will optimize the case the dst is the current
> one cause we will not check the list. This will keep the old behaviour
> eq case 1 and will do the same trick for cases like VLANs.
> Here my variant of the patch:
>
> + /* Give a chance for ifp at first priority. This will help in case we
> + * the packet comes through the interface with VLAN's and the same
> + * MACs on several interfaces in a bridge. Also will save some circles
> + * in case dst interface is the physical input interface (eq ifp).
> + */
> + if (ifp->if_type == IFT_GIF
> + && (memcmp(IF_LLADDR(bif2->bif_ifp), eh->ether_dhost,
> + ETHER_ADDR_LEN) == 0
> +#ifdef DEV_CARP
> + || (bif2->bif_ifp->if_carp + &&
> carp_forus(bif2->bif_ifp->if_carp, eh->ether_dhost))
> +#endif
> + )) {
> + if (bif->bif_flags & IFBIF_LEARNING)
> + (void) bridge_rtupdate(sc,
> + eh->ether_shost, bif, 0, IFBAF_DYNAMIC);
> + m->m_pkthdr.rcvif = ifp;
> + BRIDGE_UNLOCK(sc);
> + return (m);
> + }
> LIST_FOREACH(bif2, &sc->sc_iflist, bif_next) {
> if (bif2->bif_ifp->if_type == IFT_GIF)
> continue;
> /* It is destined for us. */
> if (memcmp(IF_LLADDR(bif2->bif_ifp), eh->ether_dhost,
> ETHER_ADDR_LEN) == 0
> #ifdef DEV_CARP
> || (bif2->bif_ifp->if_carp &&
> carp_forus(bif2->bif_ifp->if_carp, eh->ether_dhost))
> #endif
> ) {
> if (bif->bif_flags & IFBIF_LEARNING)
> (void) bridge_rtupdate(sc,
> eh->ether_shost, bif, 0, IFBAF_DYNAMIC);
> m->m_pkthdr.rcvif = bif2->bif_ifp;
> BRIDGE_UNLOCK(sc);
> return (m);
> }
>
>
> As you may note this will fix the problem only for case the packet comes from
> dst VLAN. In case this packet comes from the other interface (or not dst VLAN)
> we will also get a random VLAN (not quite random, but this may not really
> help).
Yes, this patch will do the trick. I propose to make the inner 'if' with
its body the inline function to avoid duplicating the code. I am not
100% sure about inline function, maybe macro will suit better. And I
propose to document the current behaviour of the if_bridge and filtering
in its man page for the others to avoid struggling with this ussie.
I will try to implement this modifications and will post it there.
>
> The last case could be solved by trying to pass the packet through
> the filter as more as we have an interfaces with the same MAC in
> one bridge till the first one according to the rules will say this
> is my packet or no one will take it.
May be it will be good, but we will need an additional sysctl for this.
By the way, I am already a bit scared with the number of statements
like 'if (sysctl_is_set) { do_that }' in the if_bridge. May be it
will be better to try to eliminate some 'if' statements by providing
dynamic wrappers to the functions that implement the bridge behaviour
based on the value of the current sysctl set (and assign the pointer
to those functions to the pointer that will be called once bridge
wants to do the job). Or implement the 'case' statement that probably
will do the trick. The L2 bridge should be fast as hell, isn't it?
I will try to illustrate my ideas by code, but a bit later. Or we
should not trade the code cleanness for the speed? Sure, as I am not
implemented this now I can not say about the speed gains, sorry.
And another idea about if_bridge (sorry, but today I feel myself about
spitting my ideas to everyone): maybe it is good to implement the
per-bridge set of sysctls that are governing the filtering behaviour?
For example, one bridge wants to the the full member and bridge filtering,
but another one wants no filtering at all. It is possible now, but the
second bridge will waste CPU calling the pfil_hooks that will just let
the packet to flow through.
Shutting up, thanks for your patience!
--
Eygene
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070307063606.GU57456>