Patchwork bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

login
register
mail settings
Submitter Nick Carter
Date June 28, 2011, 6:34 p.m.
Message ID <BANLkTikDqjErqBmfwrN6SJPgPjmmMfJw7g@mail.gmail.com>
Download mbox | patch
Permalink /patch/102443/
State RFC
Delegated to: David Miller
Headers show

Comments

Nick Carter - June 28, 2011, 6:34 p.m.
On 28 June 2011 17:00, David Lamparter <equinox@diac24.net> wrote:
> On Tue, Jun 28, 2011 at 08:10:15AM -0700, Stephen Hemminger wrote:
>> On Tue, 28 Jun 2011 17:02:57 +0200
>> David Lamparter <equinox@diac24.net> wrote:
>> > >   if (skb) {
>> > > +         /* Prevent Crosstalk where a Supplicant on one Port attempts to
>> > > +          * interfere with authentications occurring on another Port.
>> > > +          * (IEEE Std 802.1X-2001 C.3.3)
>> > > +          */
>> > > +         if (unlikely(!br->pae_forward &&
>> > > +             skb->protocol == htons(ETH_P_PAE)))
>> >
>> > No, please don't.
>> >
>> > Linux bridging has two "grand" modes: dumb and STP enabled.
>> >
>> > If we're running a dumb bridge, we behave like an ethernet hub without
>> > any intelligence, and in that case we should absolutely forward 802.1X
>> > frames. We may have (e.g. VM) client(s) that want to authenticate with a
>> > physical switch.
>> > (For the spec, this counts as "repeater", not "bridge"/"switch")
>> >
>> > If we're running with STP enabled, then 802.1X traffic should already be
>> > caught by the general ethernet link-local multicast drop (which applies
>> > to 01:80:c2:/24 and therefore catches 802.1X too.)
>>
>> The problem is that STP is not enabled by default, and most people don't
>> know how to enable it.
>
> Yes, the default is a dumb hub (IMHO correctly so). And a dumb hub will
> forward 802.1X packets (IMHO also correctly so).
>
> Why should we specifically add a knob for EAPOL? Next we're adding one
> for STP itself, then one for LLDP, then one for Cisco's deprecated
> crap (CDP, DTP, ...) etc.
>
> If you want a dumb hub that drops EAPOL, use ebtables.
>
> -David
>
>
If we are not going to have an EAPOL knob, but we are going to act as
a repeater when STP is off then we still need these diffs to forward
the PAE group address.
(In fact we cant just act as a repeater because of the recent ethernet
bonding regression)

the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lamparter - June 28, 2011, 6:58 p.m.
On Tue, Jun 28, 2011 at 07:34:57PM +0100, Nick Carter wrote:
> > Yes, the default is a dumb hub (IMHO correctly so). And a dumb hub will
> > forward 802.1X packets (IMHO also correctly so).
> >
> > Why should we specifically add a knob for EAPOL? Next we're adding one
> > for STP itself, then one for LLDP, then one for Cisco's deprecated
> > crap (CDP, DTP, ...) etc.
> >
> > If you want a dumb hub that drops EAPOL, use ebtables.
> >
> > -David
> >
> >
> If we are not going to have an EAPOL knob, but we are going to act as
> a repeater when STP is off then we still need these diffs to forward
> the PAE group address.
> (In fact we cant just act as a repeater because of the recent ethernet
> bonding regression)
> 
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 90e985b..267f581 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -163,7 +163,8 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)
>                         goto drop;
> 
>                 /* If STP is turned off, then forward */
> -               if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
> +               if (p->br->stp_enabled == BR_NO_STP &&
> +                       (dest[5] == 0 || skb->protocol == htons(ETH_P_PAE)))
>                         goto forward;
> Nick

That code actually looks quite wrong to me, we should be forwarding all of
the 01:80:C2:00:00:0x groups in non-STP mode, especially :0E and :0D.
(LLDP and GVRP/MVRP)

Pause frames are the one exception that makes the rule, but as the
comment a few lines above states, "Pause frames shouldn't be passed up by
driver anyway".

Btw, what might make sense is a general knob for forwarding those
link-local groups, split off from the STP switch so the STP switch
controls only the :00 (STP) group. That way you can decide separately
whether you want to be LLDP/GVRP/802.1X/... transparent and whether you
want to run STP.
Not sure if it's needed, it can always be done with ebtables...


-David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Carter - June 28, 2011, 8 p.m.
On 28 June 2011 19:58, David Lamparter <equinox@diac24.net> wrote:
> On Tue, Jun 28, 2011 at 07:34:57PM +0100, Nick Carter wrote:
>> > Yes, the default is a dumb hub (IMHO correctly so). And a dumb hub will
>> > forward 802.1X packets (IMHO also correctly so).
>> >
>> > Why should we specifically add a knob for EAPOL? Next we're adding one
>> > for STP itself, then one for LLDP, then one for Cisco's deprecated
>> > crap (CDP, DTP, ...) etc.
>> >
>> > If you want a dumb hub that drops EAPOL, use ebtables.
>> >
>> > -David
>> >
>> >
>> If we are not going to have an EAPOL knob, but we are going to act as
>> a repeater when STP is off then we still need these diffs to forward
>> the PAE group address.
>> (In fact we cant just act as a repeater because of the recent ethernet
>> bonding regression)
>>
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 90e985b..267f581 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -163,7 +163,8 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)
>>                         goto drop;
>>
>>                 /* If STP is turned off, then forward */
>> -               if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
>> +               if (p->br->stp_enabled == BR_NO_STP &&
>> +                       (dest[5] == 0 || skb->protocol == htons(ETH_P_PAE)))
>>                         goto forward;
>> Nick
>
> That code actually looks quite wrong to me, we should be forwarding all of
> the 01:80:C2:00:00:0x groups in non-STP mode, especially :0E and :0D.
> (LLDP and GVRP/MVRP)
>
> Pause frames are the one exception that makes the rule, but as the
> comment a few lines above states, "Pause frames shouldn't be passed up by
> driver anyway".
>
> Btw, what might make sense is a general knob for forwarding those
> link-local groups, split off from the STP switch so the STP switch
> controls only the :00 (STP) group. That way you can decide separately
> whether you want to be LLDP/GVRP/802.1X/... transparent and whether you
> want to run STP.

Sounds good to me.  So we go for :03, :0D, and :0E.  We cant touch :02 see:
 commit f01cb5fbea1c1613621f9f32f385e12c1a29dde0
 Revert "bridge: Forward reserved group addresses if !STP"

> Not sure if it's needed, it can always be done with ebtables...
What would be the ebtables rules to achieve the forwarding of :03 ?  I
asked this question on the netfilter list and the only response I got
said ebtables was a filter and could not do this. :03 is hitting
NF_BR_LOCAL_IN.  How would you 'reinject' it so it is forwarded ?

Thanks
Nick

>
>
> -David
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lamparter - June 28, 2011, 8:22 p.m.
On Tue, Jun 28, 2011 at 09:00:16PM +0100, Nick Carter wrote:
> >>                 /* If STP is turned off, then forward */
> >> -               if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
> >> +               if (p->br->stp_enabled == BR_NO_STP &&
> >> +                       (dest[5] == 0 || skb->protocol == htons(ETH_P_PAE)))
> >>                         goto forward;
> >> Nick
> >
> > That code actually looks quite wrong to me, we should be forwarding all of
> > the 01:80:C2:00:00:0x groups in non-STP mode, especially :0E and :0D.
> > (LLDP and GVRP/MVRP)
> >
> > Pause frames are the one exception that makes the rule, but as the
> > comment a few lines above states, "Pause frames shouldn't be passed up by
> > driver anyway".
> >
> > Btw, what might make sense is a general knob for forwarding those
> > link-local groups, split off from the STP switch so the STP switch
> > controls only the :00 (STP) group. That way you can decide separately
> > whether you want to be LLDP/GVRP/802.1X/... transparent and whether you
> > want to run STP.
> 
> Sounds good to me.  So we go for :03, :0D, and :0E.  We cant touch :02 see:
>  commit f01cb5fbea1c1613621f9f32f385e12c1a29dde0
>  Revert "bridge: Forward reserved group addresses if !STP"
> 
> > Not sure if it's needed, it can always be done with ebtables...
> What would be the ebtables rules to achieve the forwarding of :03 ?  I
> asked this question on the netfilter list and the only response I got
> said ebtables was a filter and could not do this. :03 is hitting
> NF_BR_LOCAL_IN.  How would you 'reinject' it so it is forwarded ?

'reinject' isn't possible when it hits that code path - which is pretty
much why I'm saying we should be forwarding everything in the non-STP
case.

I have to read up on the bonding interactions, but to my understanding
the only reasonable usage case is to have the bond below the bridge like
 eth0 \
      |- bond0 - br0
 eth1 /
then the bonding should receive (and consume) the packets before they
reach the bridge.

(Some quick googling reveals that hardware switch chips special-drop
01:80:c2:00:00:01 [802.3x/pause] and :02 [802.3ad/lacp] and nothing
else - for the dumb ones anyway. It also seems like the match for pause
frames usually works on the address, not on the protocol field like we
do it...)


-David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Carter - June 28, 2011, 8:54 p.m.
On 28 June 2011 21:22, David Lamparter <equinox@diac24.net> wrote:
> On Tue, Jun 28, 2011 at 09:00:16PM +0100, Nick Carter wrote:
>> >>                 /* If STP is turned off, then forward */
>> >> -               if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
>> >> +               if (p->br->stp_enabled == BR_NO_STP &&
>> >> +                       (dest[5] == 0 || skb->protocol == htons(ETH_P_PAE)))
>> >>                         goto forward;
>> >> Nick
>> >
>> > That code actually looks quite wrong to me, we should be forwarding all of
>> > the 01:80:C2:00:00:0x groups in non-STP mode, especially :0E and :0D.
>> > (LLDP and GVRP/MVRP)
>> >
>> > Pause frames are the one exception that makes the rule, but as the
>> > comment a few lines above states, "Pause frames shouldn't be passed up by
>> > driver anyway".
>> >
>> > Btw, what might make sense is a general knob for forwarding those
>> > link-local groups, split off from the STP switch so the STP switch
>> > controls only the :00 (STP) group. That way you can decide separately
>> > whether you want to be LLDP/GVRP/802.1X/... transparent and whether you
>> > want to run STP.
>>
>> Sounds good to me.  So we go for :03, :0D, and :0E.  We cant touch :02 see:
>>  commit f01cb5fbea1c1613621f9f32f385e12c1a29dde0
>>  Revert "bridge: Forward reserved group addresses if !STP"
>>
>> > Not sure if it's needed, it can always be done with ebtables...
>> What would be the ebtables rules to achieve the forwarding of :03 ?  I
>> asked this question on the netfilter list and the only response I got
>> said ebtables was a filter and could not do this. :03 is hitting
>> NF_BR_LOCAL_IN.  How would you 'reinject' it so it is forwarded ?
>
> 'reinject' isn't possible when it hits that code path - which is pretty
> much why I'm saying we should be forwarding everything in the non-STP
> case.
I'm not sure I like this turn off STP and suddenly start forwarding
random groups.  There is no connection between wanting STP on / off
and forwarding pae on / off.  There is no dependencies between the
protocols.
Also on reflection I think a knob per mac group would be better.  We
are only interested in 3 and if I enable pae forwarding so I can
connect virtual machine supplicants, i don't then want to turn on LDP
forwarding which will needlessly hit my virtual machines.
So how about sysfs
../bridge/pae_forwarding
../bridge/ldp_forwarding
../bridge/mvrp_forwarding
>
> I have to read up on the bonding interactions, but to my understanding
> the only reasonable usage case is to have the bond below the bridge like
>  eth0 \
>      |- bond0 - br0
>  eth1 /
> then the bonding should receive (and consume) the packets before they
> reach the bridge.
>
> (Some quick googling reveals that hardware switch chips special-drop
> 01:80:c2:00:00:01 [802.3x/pause] and :02 [802.3ad/lacp] and nothing
> else - for the dumb ones anyway. It also seems like the match for pause
> frames usually works on the address, not on the protocol field like we
> do it...)
'Enterprise' switches drop :03 [802.1x]
Nick
>
>
> -David
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lamparter - June 28, 2011, 9:04 p.m.
On Tue, Jun 28, 2011 at 09:54:01PM +0100, Nick Carter wrote:
> > 'reinject' isn't possible when it hits that code path - which is pretty
> > much why I'm saying we should be forwarding everything in the non-STP
> > case.
> I'm not sure I like this turn off STP and suddenly start forwarding
> random groups.  There is no connection between wanting STP on / off
> and forwarding pae on / off.

I beg to differ, there very much is. You never ever ever want to be
running STP with 802.1X packets passing through... some client will shut
down your upstream port, your STP will break and you will die in a fire.

The general idea, though, is that a STP-enabled switch is an intelligent
switch. And an intelligent switch can speak all those pesky small 
side-dish protocols.

With STP disabled on the other hand, it depends on site policy. Now
policy...

> There is no dependencies between the protocols.
> Also on reflection I think a knob per mac group would be better.

.... policy can be done nice and good with ebtables. You can match the
groups you want, or the protocols, or the phase of the moon.

> We are only interested in 3 and if I enable pae forwarding so I can
> connect virtual machine supplicants, i don't then want to turn on LDP
> forwarding which will needlessly hit my virtual machines.
> So how about sysfs
> ../bridge/pae_forwarding
> ../bridge/ldp_forwarding
> ../bridge/mvrp_forwarding

It's not like either LLDP or MVRP will trash your VMs. Those protocols
send a packet once per a few seconds.

MVRP is interesting for the STP-enabled case though. I'm not aware of
any userspace *VRP implementations, and dropping *VRP without an
userspace daemon to speak it on our behalf means we're creating a *VRP
border/break.

I would however say that doing an userspace *VRP implementation is a
better solution than kernel hacks for this particular case.

> > (Some quick googling reveals that hardware switch chips special-drop
> > 01:80:c2:00:00:01 [802.3x/pause] and :02 [802.3ad/lacp] and nothing
> > else - for the dumb ones anyway. It also seems like the match for pause
> > frames usually works on the address, not on the protocol field like we
> > do it...)
> 'Enterprise' switches drop :03 [802.1x]

They also speak STP, see above about never STP+1X :)

-David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Carter - June 28, 2011, 9:22 p.m.
On 28 June 2011 22:04, David Lamparter <equinox@diac24.net> wrote:
> On Tue, Jun 28, 2011 at 09:54:01PM +0100, Nick Carter wrote:
>> > 'reinject' isn't possible when it hits that code path - which is pretty
>> > much why I'm saying we should be forwarding everything in the non-STP
>> > case.
>> I'm not sure I like this turn off STP and suddenly start forwarding
>> random groups.  There is no connection between wanting STP on / off
>> and forwarding pae on / off.
>
> I beg to differ, there very much is. You never ever ever want to be
> running STP with 802.1X packets passing through... some client will shut
> down your upstream port, your STP will break and you will die in a fire.
>
> The general idea, though, is that a STP-enabled switch is an intelligent
> switch. And an intelligent switch can speak all those pesky small
> side-dish protocols.
>
> With STP disabled on the other hand, it depends on site policy. Now
> policy...
>
>> There is no dependencies between the protocols.
>> Also on reflection I think a knob per mac group would be better.
>
> .... policy can be done nice and good with ebtables. You can match the
> groups you want, or the protocols, or the phase of the moon.
>
>> We are only interested in 3 and if I enable pae forwarding so I can
>> connect virtual machine supplicants, i don't then want to turn on LDP
>> forwarding which will needlessly hit my virtual machines.
>> So how about sysfs
>> ../bridge/pae_forwarding
>> ../bridge/ldp_forwarding
>> ../bridge/mvrp_forwarding
>
> It's not like either LLDP or MVRP will trash your VMs. Those protocols
> send a packet once per a few seconds.
>
> MVRP is interesting for the STP-enabled case though. I'm not aware of
> any userspace *VRP implementations, and dropping *VRP without an
> userspace daemon to speak it on our behalf means we're creating a *VRP
> border/break.
>
> I would however say that doing an userspace *VRP implementation is a
> better solution than kernel hacks for this particular case.
>
>> > (Some quick googling reveals that hardware switch chips special-drop
>> > 01:80:c2:00:00:01 [802.3x/pause] and :02 [802.3ad/lacp] and nothing
>> > else - for the dumb ones anyway. It also seems like the match for pause
>> > frames usually works on the address, not on the protocol field like we
>> > do it...)
>> 'Enterprise' switches drop :03 [802.1x]
>
> They also speak STP, see above about never STP+1X :)
But if you turn off STP they wont start forwarding 802.1x.
Also having STP on and forwarding 802.1x would be useful (but
non-standard) in some cheap redundant periphery switches, connecting
to a couple of 'core' switches acting as 802.1x authenticators.
Nick
>
> -David
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lamparter - June 28, 2011, 9:46 p.m.
On Tue, Jun 28, 2011 at 10:22:53PM +0100, Nick Carter wrote:
> > I beg to differ, there very much is. You never ever ever want to be
> > running STP with 802.1X packets passing through... some client will shut
> > down your upstream port, your STP will break and you will die in a fire.
> >
> > The general idea, though, is that a STP-enabled switch is an intelligent
> > switch. And an intelligent switch can speak all those pesky small
> > side-dish protocols.
[...]
> >> > (Some quick googling reveals that hardware switch chips special-drop
> >> > 01:80:c2:00:00:01 [802.3x/pause] and :02 [802.3ad/lacp] and nothing
> >> > else - for the dumb ones anyway. It also seems like the match for pause
> >> > frames usually works on the address, not on the protocol field like we
> >> > do it...)
> >> 'Enterprise' switches drop :03 [802.1x]
> >
> > They also speak STP, see above about never STP+1X :)
> But if you turn off STP they wont start forwarding 802.1x.

Yes, hence my suggestion to have a knob for all of the link-local
ethernet groups. (Which I'm still not actually endorsing, just placing
the idea)

> Also having STP on and forwarding 802.1x would be useful (but
> non-standard) in some cheap redundant periphery switches, connecting
> to a couple of 'core' switches acting as 802.1x authenticators.

That wouldn't really make much sense since those central 802.1X
authenticators wouldn't be able switch the client-facing ports on and
off. Instead, you now have to (1) disable the port switching to make
sure your upstreams stay on and (2) deal with 802.1X clients being
re"routed" by STP to different authenticators that don't know them.


-David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Carter - June 29, 2011, 10:46 p.m.
On 28 June 2011 22:46, David Lamparter <equinox@diac24.net> wrote:
> On Tue, Jun 28, 2011 at 10:22:53PM +0100, Nick Carter wrote:
>> > I beg to differ, there very much is. You never ever ever want to be
>> > running STP with 802.1X packets passing through... some client will shut
>> > down your upstream port, your STP will break and you will die in a fire.
>> >
>> > The general idea, though, is that a STP-enabled switch is an intelligent
>> > switch. And an intelligent switch can speak all those pesky small
>> > side-dish protocols.
> [...]
>> >> > (Some quick googling reveals that hardware switch chips special-drop
>> >> > 01:80:c2:00:00:01 [802.3x/pause] and :02 [802.3ad/lacp] and nothing
>> >> > else - for the dumb ones anyway. It also seems like the match for pause
>> >> > frames usually works on the address, not on the protocol field like we
>> >> > do it...)
>> >> 'Enterprise' switches drop :03 [802.1x]
>> >
>> > They also speak STP, see above about never STP+1X :)
>> But if you turn off STP they wont start forwarding 802.1x.
>
> Yes, hence my suggestion to have a knob for all of the link-local
> ethernet groups. (Which I'm still not actually endorsing, just placing
> the idea)
>
>> Also having STP on and forwarding 802.1x would be useful (but
>> non-standard) in some cheap redundant periphery switches, connecting
>> to a couple of 'core' switches acting as 802.1x authenticators.
>
> That wouldn't really make much sense since those central 802.1X
> authenticators wouldn't be able switch the client-facing ports on and
> off.
Although its non standard, it is common for authenticators to do
802.1X per source mac rather than per port.  Also the central
authenticator ports can be routed not bridged.  So i dont think you
can rule out the "STP on plus 802.1x being forwarded" requirement.

> Instead, you now have to (1) disable the port switching to make
> sure your upstreams stay on and (2) deal with 802.1X clients being
> re"routed" by STP to different authenticators that don't know them.
Well if the authenticators are pointed at a remote ACS then they will
know them.  And again even though non-standard, 802.1X 'mac move'
functionality exists.
Nick
>
>
> -David
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Hemminger - June 29, 2011, 11:34 p.m.
The problem is that the damn 802.1 committees keep loading up protocols
on the same multicast address range. Trying to solve a design committee 
problem in the kernel is not going to make anybody happy.

I am happy with the simple solution of:
  no STP == Hub
  STP    == Bridge
These are both well know configurations and are blessed by standards.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lamparter - July 1, 2011, 10:16 a.m.
On Wed, Jun 29, 2011 at 04:34:23PM -0700, Stephen Hemminger wrote:
> The problem is that the damn 802.1 committees keep loading up protocols
> on the same multicast address range. Trying to solve a design committee 
> problem in the kernel is not going to make anybody happy.
> 
> I am happy with the simple solution of:
>   no STP == Hub
>   STP    == Bridge
> These are both well know configurations and are blessed by standards.

I agree, that is how we should behave by default, and we'll match most
admin's expectations.

Regarding multicast groups, I would summarise like this:
1. any multicast gets forwarded by default,
 2. unless it is 01:80:c2:00:00:01 or :02 (pause/bonding)
    (this rule applies regardless of STP state)
 3. if STP is on:
  4. 01:80:c2:00:00:00 (STP) never gets forwarded
  5. 01:80:c2:00:00:03-0f don't get forwarded by default

What we can do is add a switch to disable the #5 rule. The way I see it
is that that switch would remove an exception from the rule and turn it
back to the default #1; that's acceptable for making a new knob in my
eyes.

(Adding an 802.1X knob would be an exception to the exception for me,
which is why I'm against it.)

I'll cook up a patch in a few minutes, we really need to get rule #2
right anyway. We _MUST_NOT_ pass bonding frames in any case, but we
currently do that if STP is off. (cf. my earlier patch 1/2)


-David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= - July 1, 2011, 2:58 p.m.
2011/7/1 David Lamparter <equinox@diac24.net>:
> On Wed, Jun 29, 2011 at 04:34:23PM -0700, Stephen Hemminger wrote:
>> The problem is that the damn 802.1 committees keep loading up protocols
>> on the same multicast address range. Trying to solve a design committee
>> problem in the kernel is not going to make anybody happy.
>>
>> I am happy with the simple solution of:
>>   no STP == Hub
>>   STP    == Bridge
>> These are both well know configurations and are blessed by standards.
>
> I agree, that is how we should behave by default, and we'll match most
> admin's expectations.
>
> Regarding multicast groups, I would summarise like this:
> 1. any multicast gets forwarded by default,
>  2. unless it is 01:80:c2:00:00:01 or :02 (pause/bonding)
>    (this rule applies regardless of STP state)
>  3. if STP is on:
>  4. 01:80:c2:00:00:00 (STP) never gets forwarded
>  5. 01:80:c2:00:00:03-0f don't get forwarded by default
>
> What we can do is add a switch to disable the #5 rule. The way I see it
> is that that switch would remove an exception from the rule and turn it
> back to the default #1; that's acceptable for making a new knob in my
> eyes.
>
> (Adding an 802.1X knob would be an exception to the exception for me,
> which is why I'm against it.)
>
> I'll cook up a patch in a few minutes, we really need to get rule #2
> right anyway. We _MUST_NOT_ pass bonding frames in any case, but we
> currently do that if STP is off. (cf. my earlier patch 1/2)

If you use linux box as a (invisible) L2 network tap, then you want to
pass everything in the hub mode (including LACP/whatever).

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lamparter - July 1, 2011, 3:16 p.m.
On Fri, Jul 01, 2011 at 04:58:56PM +0200, Michał Mirosław wrote:
[...]
> > We _MUST_NOT_ pass bonding frames in any case, but we
> > currently do that if STP is off. (cf. my earlier patch 1/2)
> 
> If you use linux box as a (invisible) L2 network tap, then you want to
> pass everything in the hub mode (including LACP/whatever).

We must not do that by default, this breaks bridges with bonding devices
as ports. I'm actively band-aiding that problem with ebtables on one of
my boxes currently.

How about I change "stp_forward_802local" to "forward_802local" and it
gets 3 values like:
- 0 (default) behave like a switch, if STP is on then drop all 16
  groups, if STP is off then drop :01 and :02
- 1 forward regular groups - drop :01 and :02, forward everything else
- 2 forward everything ("invisible tap mode")
optional:
- -1 drop all 16 groups even if STP is off (not needed, can be done with
  ebtables...)

btw, since the drivers should eat up pause frames, you're not a fully
invisible L2 tap anyway.


-David

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= - July 1, 2011, 5:59 p.m.
W dniu 1 lipca 2011 17:16 użytkownik David Lamparter
<equinox@diac24.net> napisał:
> On Fri, Jul 01, 2011 at 04:58:56PM +0200, Michał Mirosław wrote:
> [...]
>> > We _MUST_NOT_ pass bonding frames in any case, but we
>> > currently do that if STP is off. (cf. my earlier patch 1/2)
>>
>> If you use linux box as a (invisible) L2 network tap, then you want to
>> pass everything in the hub mode (including LACP/whatever).
>
> We must not do that by default, this breaks bridges with bonding devices
> as ports. I'm actively band-aiding that problem with ebtables on one of
> my boxes currently.
>
> How about I change "stp_forward_802local" to "forward_802local" and it
> gets 3 values like:
> - 0 (default) behave like a switch, if STP is on then drop all 16
>  groups, if STP is off then drop :01 and :02
> - 1 forward regular groups - drop :01 and :02, forward everything else
> - 2 forward everything ("invisible tap mode")
> optional:
> - -1 drop all 16 groups even if STP is off (not needed, can be done with
>  ebtables...)
>
> btw, since the drivers should eat up pause frames, you're not a fully
> invisible L2 tap anyway.

If -1 can be done with ebtables what is different for 0 and 1 cases?

Another idea: you could make this a 16-bit bitmap (bit per group) x2
(STP vs non-STP) - that would cover all uses with the same amount of
code.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nick Carter - July 1, 2011, 9:10 p.m.
2011/7/1 Michał Mirosław <mirqus@gmail.com>:
> W dniu 1 lipca 2011 17:16 użytkownik David Lamparter
> <equinox@diac24.net> napisał:
>> On Fri, Jul 01, 2011 at 04:58:56PM +0200, Michał Mirosław wrote:
>> [...]
>>> > We _MUST_NOT_ pass bonding frames in any case, but we
>>> > currently do that if STP is off. (cf. my earlier patch 1/2)
>>>
>>> If you use linux box as a (invisible) L2 network tap, then you want to
>>> pass everything in the hub mode (including LACP/whatever).
>>
>> We must not do that by default, this breaks bridges with bonding devices
>> as ports. I'm actively band-aiding that problem with ebtables on one of
>> my boxes currently.
>>
>> How about I change "stp_forward_802local" to "forward_802local" and it
>> gets 3 values like:
>> - 0 (default) behave like a switch, if STP is on then drop all 16
>>  groups, if STP is off then drop :01 and :02
>> - 1 forward regular groups - drop :01 and :02, forward everything else
>> - 2 forward everything ("invisible tap mode")
>> optional:
>> - -1 drop all 16 groups even if STP is off (not needed, can be done with
>>  ebtables...)
>>
>> btw, since the drivers should eat up pause frames, you're not a fully
>> invisible L2 tap anyway.
>
> If -1 can be done with ebtables what is different for 0 and 1 cases?
>
> Another idea: you could make this a 16-bit bitmap (bit per group) x2
> (STP vs non-STP) - that would cover all uses with the same amount of
> code.
That is exactly what I thought yesterday and I wrote and tested the
code today :)

+++ b/net/bridge/br_input.c
@@ -166,6 +166,9 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)
 		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
 			goto forward;

+		if (p->br->group_fwd_mask & (1 << dest[5]))
+			goto forward;

+++ b/net/bridge/br_private.h
@@ -244,6 +244,13 @@ struct net_bridge
 	struct timer_list		multicast_query_timer;
 #endif

+	/* Each bit used to match the LSB of the IEEE 802.1D group address
+	 * 01-80-C2-00-00-00 bit 0
+	 * ..
+	 * 01-80-C2-00-00-0F bit 15
+	 */
+	u16				group_fwd_mask;
+

I will post the full diffs to netdev now.  With this 'knob' users can
have any behaviour they require.
Nick

>
> Best Regards,
> Michał Mirosław
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 90e985b..267f581 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -163,7 +163,8 @@  struct sk_buff *br_handle_frame(struct sk_buff *skb)
                        goto drop;

                /* If STP is turned off, then forward */
-               if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
+               if (p->br->stp_enabled == BR_NO_STP &&
+                       (dest[5] == 0 || skb->protocol == htons(ETH_P_PAE)))
                        goto forward;
Nick
--
To unsubscribe from this list: send the line "unsubscribe netdev" in