kern/109815: wrong interface identifier at pfil_hooks for vlans
+ if_bridge
Roman Kurakin
rik at inse.ru
Tue Mar 6 17:50:55 UTC 2007
Ok, since no one want to provide an explanation I'll to do it myself.
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.
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.
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).
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.
We also should have the ability to filter packet according to its
physical origin on
this host eq case2. But this is other stories.
Since no one claim responsibility for fixing this, I'll do it myself.
I'll post the final
patch at the end of this week and will wait one more week for the last
chance to
stop me. After that it will be committed.
rik
Eygene Ryabinkin wrote:
> Good day, the freebsd-net readers.
>
> Perhaps someone can look at the kern/109815 and provide some
> feedback. Citing my PR:
>
>
>>> When if_bridge is used to gather multiple vlan interfaces that have
>>> the same physical parent (I will call such vlan interfaces the 'vlan
>>> group') the interface identifier that will be passed to the
>>> pfil_hooks will be sometimes wrong. For all packets coming from the
>>> 'vlan group' and destined to some local interface the incoming interface
>>> passed to the pfil_hooks will be the last interface in that group
>>> regardless of the actual incoming interface. The 'last interface' is
>>> the interface that was added to the if_bridge at the very last 'addm'
>>> command.
>>>
>>> The problem is lying in the fact that MAC addresses of the 'vlan group'
>>> are just the same and they are equal to the MAC address of the parent
>>> interface. And the check for the unicat packet that is destined for
>>> 'us' in the if_bridge.c:bridge_input() is walking by the list of the
>>> if_bridge-attached interfaces and compares the MAC addresses to the
>>> packet's one. Once match is found the interface in the packet header
>>> will be rewritten to the found list entry's one. Apparently such
>>> code flow will select the last added interface from the 'vlan group'
>>> because FreeBSD list macros are adding list members to the beginning
>>> of the linked list.
>>>
>>> BPF will receive the right interface, because it is tapped before
>>> bridge_input (in if_ethersubr.c).
>>>
>
> Roman Kurakin suggested that the patch can break some things:
>
> Sun, Mar 04, 2007 at 01:08:40AM +0300, Roman Kurakin wrote:
>
>> The idea behind current code could be that in case of bridge the
>> interface for which this packet in?nded is much more important
>> than the physical interface it is arrived from.
>> If this is the case, than it would be much better do the same logic
>> for ifp and in case it is not that interface to check the list.
>> This will fix the problem in case of vlans and will leave the old
>> behavior for the other cases.
>>
>> Any comments?
>>
>
> I've slightly elaborated his idea and commented on it:
> Sun, Mar 04, 2007 at 09:22:03AM +0300, Eygene Ryabinkin wrote:
>
>> So, you're saying the following: let us have two interfaces in the
>> bridge ifA and ifB with the MAC-A and MAC-B. In the current situation
>> packet coming from the physical interface ifA but destined to the
>> MAC-B, then the interface seen by the packet filter will be ifB and
>> not the ifA. Right?
>>
>> In principle, this situation can be used for something. But then we
>> should at least teah BPF to behave this way, because as you remember
>> from spending three hours before the console with me and trying to
>> diagnose the problem with tcpdump we were amazed to see the discrepance
>> between bpf and pfil. So it should be at least documented.
>>
>> But as I understand, my patch will let the described situation to
>> work as usual -- packets will still be delivered to the ifB, but
>> packet filter will see the physical incoming interface. The patch
>> should not break the delivery of the packet since all I do is the
>> change of the rcvif. Or I am wrong?
>>
>
> I traced the current if_bridge.c behaviour to the NetBSD's if_bridge.c
> 1.9. This was the first version in that the firewall hooks were
> introduced. And the assumtion that the MAC identifies the physical
> interfaces was used in this first version.
>
> And a question: can anyone say if my patch will break some known
> good behaviour and if the current behaviour of if_bridge is based
> on some logic I am currently failing to understand.
>
> Thank you!
>
More information about the freebsd-net
mailing list