[Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Bernhard Thaler]

BR_GROUPFWD_RESTRICTED bitmask restricts users from setting values to
/sys/class/net/brX/bridge/group_fwd_mask that allow forwarding of
some IEEE 802.1D Table 7-10 Reserved addresses:
	(MAC Control) 802.3		01-80-C2-00-00-01
	(Link Aggregation) 802.3	01-80-C2-00-00-02
	802.1AB LLDP			01-80-C2-00-00-0E
BR_GROUPFWD_RESTRICTED may have been set as an extra protection against
forwarding these control frames as forwarding 802.1X PAE (01-80-C2-00-00-03)
in 802.1X setups satisfies most common use-cases.
Other situations, such as placing a software based bridge as a "TAP" between two
devices may require to forward e.g. LLDP frames while debugging network problems
or actively changing/filtering traffic with ebtables.

This patch allows to set e.g.:
	echo 65535 > /sys/class/net/brX/bridge/group_fwd_mask
which sets no restrictions on the forwardable reserved addresses.

- the default value 0 will still comply with 802.1D and not forward any
  reserved addresses
- values such as 8 for forwarding 802.1X related frames will behave the
  same way as with BR_GROUPFWD_RESTRICTED currently in place, so backward
  compatibility to current scripts using group_fwd_masks shoudl be possible

Administrators and network engineers however will be able to arbitrarily
forward any reserved addresses without BR_GROUPFWD_RESTRICTED. This will
be non-standard compliant behavior, but forwarding of any reserved address
right from the beginning is. Users should be aware of this anyway and
know what/why they are doing when setting values such as 65535, 32768, 16384,
4, 2 for group_fwd_mask

This patch was tested on a bridge with two interfaces created with bridge-utils.

Signed-off-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
---
 net/bridge/br_input.c    |    8 ++++++--
 net/bridge/br_private.h  |    2 --
 net/bridge/br_sysfs_br.c |    3 ---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 1f1de71..e44fe38 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -262,8 +262,12 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 				goto forward;
 			break;
 
-		case 0x01:	/* IEEE MAC (Pause) */
-			goto drop;
+		case 0x01:      /* IEEE MAC (Pause) */
+			fwd_mask |= p->br->group_fwd_mask;
+			if (fwd_mask & (1u << dest[5]))
+				goto forward;
+			else
+				goto drop;
 
 		default:
 			/* Allow selective forwarding for most other protocols */
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index aea3d13..9b548754 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -33,8 +33,6 @@
 
 /* Control of forwarding link local multicast */
 #define BR_GROUPFWD_DEFAULT	0
-/* Don't allow forwarding control protocols like STP and LLDP */
-#define BR_GROUPFWD_RESTRICTED	0x4007u
 /* The Nearest Customer Bridge Group Address, 01-80-C2-00-00-[00,0B,0C,0D,0F] */
 #define BR_GROUPFWD_8021AD	0xB801u
 
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 4c97fc5..7f04d8b 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -171,9 +171,6 @@ static ssize_t group_fwd_mask_store(struct device *d,
 	if (endp == buf)
 		return -EINVAL;
 
-	if (val & BR_GROUPFWD_RESTRICTED)
-		return -EINVAL;
-
 	br->group_fwd_mask = val;
 
 	return len;
-- 
1.7.10.4

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Stephen Hemminger]

On Tue,  6 Jan 2015 01:56:15 +0100
Bernhard Thaler <bernhard.thaler@wvnet.at> wrote:

> BR_GROUPFWD_RESTRICTED bitmask restricts users from setting values to
> /sys/class/net/brX/bridge/group_fwd_mask that allow forwarding of
> some IEEE 802.1D Table 7-10 Reserved addresses:
> 	(MAC Control) 802.3		01-80-C2-00-00-01
> 	(Link Aggregation) 802.3	01-80-C2-00-00-02
> 	802.1AB LLDP			01-80-C2-00-00-0E
> BR_GROUPFWD_RESTRICTED may have been set as an extra protection against
> forwarding these control frames as forwarding 802.1X PAE (01-80-C2-00-00-03)
> in 802.1X setups satisfies most common use-cases.
> Other situations, such as placing a software based bridge as a "TAP" between two
> devices may require to forward e.g. LLDP frames while debugging network problems
> or actively changing/filtering traffic with ebtables.
> 
> This patch allows to set e.g.:
> 	echo 65535 > /sys/class/net/brX/bridge/group_fwd_mask
> which sets no restrictions on the forwardable reserved addresses.
> 
> - the default value 0 will still comply with 802.1D and not forward any
>   reserved addresses
> - values such as 8 for forwarding 802.1X related frames will behave the
>   same way as with BR_GROUPFWD_RESTRICTED currently in place, so backward
>   compatibility to current scripts using group_fwd_masks shoudl be possible
> 
> Administrators and network engineers however will be able to arbitrarily
> forward any reserved addresses without BR_GROUPFWD_RESTRICTED. This will
> be non-standard compliant behavior, but forwarding of any reserved address
> right from the beginning is. Users should be aware of this anyway and
> know what/why they are doing when setting values such as 65535, 32768, 16384,
> 4, 2 for group_fwd_mask
> 
> This patch was tested on a bridge with two interfaces created with bridge-utils.
> 
> Signed-off-by: Bernhard Thaler <bernhard.thaler@wvnet.at>

I am ok with forwarding LLDP because some people need it.
But allowing forwarding STP or PAUSE frames is bad.

We don't let people do things that break networks. Other examples
already exist like set all 0 ethernet addresses, or the restrictions
on allowing net 127 in IP addresses.

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Richard Weinberger]

Florian,

Am Montag, 1. Oktober 2018, 18:24:25 CEST schrieb Florian Fainelli:
> If all you are doing is forwarding anything, one thing I experimented
> with before is the following:
> 
> # tc qdisc add dev eth1 handle ffff: ingress
> # tc qdisc add dev eth3 handle ffff: ingress
> # tc filter add dev eth3 parent ffff: u32 \
> > match u32 0 0 \
> > action mirred egress redirect dev eth1
> # tc filter add dev eth1 parent ffff: u32 \
> > match u32 0 0 \
> > action mirred egress redirect dev eth3
> # ifconfig eth3 promisc
> # ifconfig eth1 promisc
> 
> and this works just fine actually, bypassing the bridge layer entirely.

Yeah, mirred is a powerful knife. :-)

In my case it is too low level since I utilize the netfilter functionality of
the bridge layer.

Thanks,
//richard

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Ido Schimmel]

On Mon, Oct 01, 2018 at 08:16:22PM +0200, Richard Weinberger wrote:
> Florian,
> 
> Am Montag, 1. Oktober 2018, 18:24:25 CEST schrieb Florian Fainelli:
> > If all you are doing is forwarding anything, one thing I experimented
> > with before is the following:
> > 
> > # tc qdisc add dev eth1 handle ffff: ingress
> > # tc qdisc add dev eth3 handle ffff: ingress
> > # tc filter add dev eth3 parent ffff: u32 \
> > > match u32 0 0 \
> > > action mirred egress redirect dev eth1
> > # tc filter add dev eth1 parent ffff: u32 \
> > > match u32 0 0 \
> > > action mirred egress redirect dev eth3
> > # ifconfig eth3 promisc
> > # ifconfig eth1 promisc
> > 
> > and this works just fine actually, bypassing the bridge layer entirely.
> 
> Yeah, mirred is a powerful knife. :-)
> 
> In my case it is too low level since I utilize the netfilter functionality of
> the bridge layer.

You can use mirred only for the specific packets you care about and let
the rest continue to the bridge.

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Richard Weinberger]

Am Montag, 1. Oktober 2018, 20:25:26 CEST schrieb Ido Schimmel:
> On Mon, Oct 01, 2018 at 08:16:22PM +0200, Richard Weinberger wrote:
> > Florian,
> > 
> > Am Montag, 1. Oktober 2018, 18:24:25 CEST schrieb Florian Fainelli:
> > > If all you are doing is forwarding anything, one thing I experimented
> > > with before is the following:
> > > 
> > > # tc qdisc add dev eth1 handle ffff: ingress
> > > # tc qdisc add dev eth3 handle ffff: ingress
> > > # tc filter add dev eth3 parent ffff: u32 \
> > > > match u32 0 0 \
> > > > action mirred egress redirect dev eth1
> > > # tc filter add dev eth1 parent ffff: u32 \
> > > > match u32 0 0 \
> > > > action mirred egress redirect dev eth3
> > > # ifconfig eth3 promisc
> > > # ifconfig eth1 promisc
> > > 
> > > and this works just fine actually, bypassing the bridge layer entirely.
> > 
> > Yeah, mirred is a powerful knife. :-)
> > 
> > In my case it is too low level since I utilize the netfilter functionality of
> > the bridge layer.
> 
> You can use mirred only for the specific packets you care about and let
> the rest continue to the bridge.

This is my plan b, having a u32 classifier that transports STP directly
to the other interface.
But IMHO this all is a bit hacky and a "forward anything" bridge mode
sounds more natural to me.

Thanks,
//richard 

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Ido Schimmel]

On Mon, Oct 01, 2018 at 08:32:12PM +0200, Richard Weinberger wrote:
> Am Montag, 1. Oktober 2018, 20:25:26 CEST schrieb Ido Schimmel:
> > On Mon, Oct 01, 2018 at 08:16:22PM +0200, Richard Weinberger wrote:
> > > Florian,
> > > 
> > > Am Montag, 1. Oktober 2018, 18:24:25 CEST schrieb Florian Fainelli:
> > > > If all you are doing is forwarding anything, one thing I experimented
> > > > with before is the following:
> > > > 
> > > > # tc qdisc add dev eth1 handle ffff: ingress
> > > > # tc qdisc add dev eth3 handle ffff: ingress
> > > > # tc filter add dev eth3 parent ffff: u32 \
> > > > > match u32 0 0 \
> > > > > action mirred egress redirect dev eth1
> > > > # tc filter add dev eth1 parent ffff: u32 \
> > > > > match u32 0 0 \
> > > > > action mirred egress redirect dev eth3
> > > > # ifconfig eth3 promisc
> > > > # ifconfig eth1 promisc
> > > > 
> > > > and this works just fine actually, bypassing the bridge layer entirely.
> > > 
> > > Yeah, mirred is a powerful knife. :-)
> > > 
> > > In my case it is too low level since I utilize the netfilter functionality of
> > > the bridge layer.
> > 
> > You can use mirred only for the specific packets you care about and let
> > the rest continue to the bridge.
> 
> This is my plan b, having a u32 classifier that transports STP directly
> to the other interface.
> But IMHO this all is a bit hacky and a "forward anything" bridge mode
> sounds more natural to me.

But "forwarding STP and PAUSE if the number of slaves is restricted to
2" is a hack. The Linux bridge (like other networking equipment) needs
to conform to standards and to the best of my knowledge what you're
requesting is explicitly forbidden by IEEE standards.

Also, if what you need is "forward anything", then Florian's suggestion
should work for you.

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Richard Weinberger]

Am Montag, 1. Oktober 2018, 20:48:21 CEST schrieb Ido Schimmel:
> > This is my plan b, having a u32 classifier that transports STP directly
> > to the other interface.
> > But IMHO this all is a bit hacky and a "forward anything" bridge mode
> > sounds more natural to me.
> 
> But "forwarding STP and PAUSE if the number of slaves is restricted to
> 2" is a hack. The Linux bridge (like other networking equipment) needs
> to conform to standards and to the best of my knowledge what you're
> requesting is explicitly forbidden by IEEE standards.
> 
> Also, if what you need is "forward anything", then Florian's suggestion
> should work for you.

Agreed, both variants are hacks. Depending on the point of view one might seem
less hacky than the other. :-)

As I said, netfilter is also part of the game. Unless I miss something, netfilter
won't see any packets if tc-mirred is used.
So the only option is having a bridge and transport STP via tc-mirred
or patching the bridge code (what we do right now).
 
Thanks,
//richard

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Ido Schimmel]

On Mon, Oct 01, 2018 at 08:54:08PM +0200, Richard Weinberger wrote:
> So the only option is having a bridge and transport STP via tc-mirred
> or patching the bridge code (what we do right now).

And I vote for the first option. I understand it involves more typing,
but I see no reason to push more complexity into the kernel - and break
standards - when you can relatively easily accomplish the same thing in
other ways.

Adding Nik and Roopa who now maintain the bridge code and should
eventually decide about this.

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Richard Weinberger]

Am Montag, 1. Oktober 2018, 21:04:33 CEST schrieb Ido Schimmel:
> On Mon, Oct 01, 2018 at 08:54:08PM +0200, Richard Weinberger wrote:
> > So the only option is having a bridge and transport STP via tc-mirred
> > or patching the bridge code (what we do right now).
> 
> And I vote for the first option. I understand it involves more typing,

hehe, it is not about typing. The setup is done by a script.
And we both know that getting a patch upstream is much more work than
typing a few lines of hacky bash scripts.
Since I want a decent solution and feedback I'm bringing this up here.

I could also just go with my in-house patch and don't tell anyone...

> but I see no reason to push more complexity into the kernel - and break
> standards - when you can relatively easily accomplish the same thing in
> other ways.

If having a bridge plus u32+mirred for STP bypass is the preferred solution,
I'm fine with it.
So far we use the kernel patch and didn't test the mirred-bypass a lot.
My fear was that mirred rules from eth0 to eth1 and back might cause a
loop or confuse the bridge...

> Adding Nik and Roopa who now maintain the bridge code and should
> eventually decide about this.

Thanks,
//richard 

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Nikolay Aleksandrov]

On 01/10/2018 22:10, Richard Weinberger wrote:
> Am Montag, 1. Oktober 2018, 21:04:33 CEST schrieb Ido Schimmel:
>> On Mon, Oct 01, 2018 at 08:54:08PM +0200, Richard Weinberger wrote:
>>> So the only option is having a bridge and transport STP via tc-mirred
>>> or patching the bridge code (what we do right now).
>>
>> And I vote for the first option. I understand it involves more typing,
> 
> hehe, it is not about typing. The setup is done by a script.
> And we both know that getting a patch upstream is much more work than
> typing a few lines of hacky bash scripts.
> Since I want a decent solution and feedback I'm bringing this up here.
> 
> I could also just go with my in-house patch and don't tell anyone...
> 
>> but I see no reason to push more complexity into the kernel - and break
>> standards - when you can relatively easily accomplish the same thing in
>> other ways.
> 
> If having a bridge plus u32+mirred for STP bypass is the preferred solution,
> I'm fine with it.
> So far we use the kernel patch and didn't test the mirred-bypass a lot.
> My fear was that mirred rules from eth0 to eth1 and back might cause a
> loop or confuse the bridge...
> 
>> Adding Nik and Roopa who now maintain the bridge code and should
>> eventually decide about this.
> 
> Thanks,
> //richard 
> 
> 
> 
> 

Richard please check commit:
commit 5af48b59f35c
Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date:   Wed Sep 27 16:12:44 2017 +0300

    net: bridge: add per-port group_fwd_mask with less restrictions
    
    We need to be able to transparently forward most link-local frames via
    tunnels (e.g. vxlan, qinq). Currently the bridge's group_fwd_mask has a
    mask which restricts the forwarding of STP and LACP, but we need to be able
    to forward these over tunnels and control that forwarding on a per-port
    basis thus add a new per-port group_fwd_mask option which only disallows
    mac pause frames to be forwarded (they're always dropped anyway).
    The patch does not change the current default situation - all of the others
    are still restricted unless configured for forwarding.
    We have successfully tested this patch with LACP and STP forwarding over
    VxLAN and qinq tunnels.
    
    Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>


Will this work for you ?
It's in the bridge since v4.15.

Thanks,
 Nik

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Richard Weinberger]

Nikolay,

Am Dienstag, 2. Oktober 2018, 16:59:31 CEST schrieb Nikolay Aleksandrov:
> Richard please check commit:
> commit 5af48b59f35c
> Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Date:   Wed Sep 27 16:12:44 2017 +0300
> 
>     net: bridge: add per-port group_fwd_mask with less restrictions
>     
>     We need to be able to transparently forward most link-local frames via
>     tunnels (e.g. vxlan, qinq). Currently the bridge's group_fwd_mask has a
>     mask which restricts the forwarding of STP and LACP, but we need to be able
>     to forward these over tunnels and control that forwarding on a per-port
>     basis thus add a new per-port group_fwd_mask option which only disallows
>     mac pause frames to be forwarded (they're always dropped anyway).
>     The patch does not change the current default situation - all of the others
>     are still restricted unless configured for forwarding.
>     We have successfully tested this patch with LACP and STP forwarding over
>     VxLAN and qinq tunnels.
>     
>     Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> 
> Will this work for you ?
> It's in the bridge since v4.15.

Hmm, I *think* this is exactly what I need.
To understand it correctly, I have to set per port group_fwd_mask for both slaves
of the bridge then it will forward anything (except for PAUSE frames)?

Is there a reason why this knob is not documented in Documentation/ABI/testing/sysfs-class-net?

Thanks,
//richard

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Nikolay Aleksandrov]

On 02/10/2018 18:56, Richard Weinberger wrote:
> Nikolay,
> 
> Am Dienstag, 2. Oktober 2018, 16:59:31 CEST schrieb Nikolay Aleksandrov:
>> Richard please check commit:
>> commit 5af48b59f35c
>> Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> Date:   Wed Sep 27 16:12:44 2017 +0300
>>
>>     net: bridge: add per-port group_fwd_mask with less restrictions
>>     
>>     We need to be able to transparently forward most link-local frames via
>>     tunnels (e.g. vxlan, qinq). Currently the bridge's group_fwd_mask has a
>>     mask which restricts the forwarding of STP and LACP, but we need to be able
>>     to forward these over tunnels and control that forwarding on a per-port
>>     basis thus add a new per-port group_fwd_mask option which only disallows
>>     mac pause frames to be forwarded (they're always dropped anyway).
>>     The patch does not change the current default situation - all of the others
>>     are still restricted unless configured for forwarding.
>>     We have successfully tested this patch with LACP and STP forwarding over
>>     VxLAN and qinq tunnels.
>>     
>>     Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>     Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>>
>> Will this work for you ?
>> It's in the bridge since v4.15.
> 
> Hmm, I *think* this is exactly what I need.
> To understand it correctly, I have to set per port group_fwd_mask for both slaves
> of the bridge then it will forward anything (except for PAUSE frames)?
> 

Yes, that is the expected behaviour.

> Is there a reason why this knob is not documented in Documentation/ABI/testing/sysfs-class-net?
> 

Hmm, I don't think any of the bridge knobs are documented there or any other master device for that
matter. Most of these options are documented in ip-link.8 man page from iproute2, it seems though
this one I've missed to document. I'll prepare a patch for iproute2.
 
> Thanks,
> //richard
> 
> 
> 

Re: [Bridge] [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses

[Richard Weinberger]

On Tue, Oct 2, 2018 at 6:48 PM Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> > Is there a reason why this knob is not documented in Documentation/ABI/testing/sysfs-class-net?
> >
>
> Hmm, I don't think any of the bridge knobs are documented there or any other master device for that
> matter. Most of these options are documented in ip-link.8 man page from iproute2, it seems though
> this one I've missed to document. I'll prepare a patch for iproute2.

Well, /sys/class/net/<bridge iface>/bridge/group_fwd_mask is
documented, this confused me a little.

-- 
Thanks,
//richard