Re: [PATCH] bridge: mask forwarding of IEEE 802 local multicast groups

Re: [PATCH] bridge: mask forwarding of IEEE 802 local multicast groups

[Ed Swierk]

Stephen Hemminger wrote:
> No.
> Don't like adding yet another hack user visible API which will have
> to be maintained for too long. But on the other hand I don't have
> a better solution at my finger tips. If better idea doesn't come
> along, then we can go with yours.

+1 for applying Nick Carter's patch adding a group_fwd_mask setting to
the bridge driver
(http://www.spinics.net/lists/netdev/msg169607.html).

I'm using elaborate combinations of bridges, tunnels and vlans to make
remote switches many hops away appear as though they're locally
connected to a virtual machine. In this application the bridges are
just used to forward traffic between a pair of interfaces. I need the
bridge to behave as much like a dumb 2-port hub as possible,
forwarding LLDP frames that 802.1d-compliant bridges normally drop.

I solemnly swear I will not pester the netdev list when I melt down my
LAN by inadvertently forwarding STP frames.

--Ed

Re: [PATCH] bridge: mask forwarding of IEEE 802 local multicast groups

[Stephen Hemminger]

On Fri, 12 Aug 2011 15:37:54 -0700
Ed Swierk <eswierk@bigswitch.com> wrote:

> I'm using elaborate combinations of bridges, tunnels and vlans to make
> remote switches many hops away appear as though they're locally
> connected to a virtual machine. In this application the bridges are
> just used to forward traffic between a pair of interfaces. I need the
> bridge to behave as much like a dumb 2-port hub as possible,
> forwarding LLDP frames that 802.1d-compliant bridges normally drop.

If you turn off STP that is what it does right now. It forwards STP
and 802.1X frames. The patch was only related to turning on STP
and still expecting 802.1X frames to be forwarded (in violation of
the spec).

Re: [PATCH] bridge: mask forwarding of IEEE 802 local multicast groups

[Ed Swierk]

On Fri, Aug 12, 2011 at 3:45 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> If you turn off STP that is what it does right now. It forwards STP
> and 802.1X frames. The patch was only related to turning on STP
> and still expecting 802.1X frames to be forwarded (in violation of
> the spec).

As far as I can tell, the only case currently where the bridge
forwards a packet destined for a link-local multicast address is

  if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)

dest[5] != 0 for an LLDP packet, whose destination address is
01-80-C2-00-00-0E per IEEE 802.1AB-2005.

--Ed

[RFC] bridge: allow passing link-local multicast

[Stephen Hemminger]

Several users have wanted to forward 802.1x EAP multicast
packets through a bridge. And there has been a couple of
attempts at allowing some form of this in the past.

If a bridge does not have spanning tree turned on, then it should
act like a pure hub and forward all traffic. This makes it fully
transparent, and if there is another bridge using spanning tree
the STP packets will still work for detecting loops in the network.

If bridge has STP enabled, then the default behavior is to
process all link-local multicasts locally. The expectation is
that if 802.1x or other protocol using link-local multicasts
that a service (or proxy) for that protocol will be used.

Optionally, a sysctl value can be set to allow non STP packets
to still be forwarded.  I chose sysctl for this because it is
where such modifications exist when doing IP or netfilter.
There are other filtering/configuration options that are needed
and this is a better way to enable them.

Thanks to David Lamparter, and others for bringing this up.
Users who need this facility should provide feedback, is this
a usable solution?

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Patch against net-next

 Documentation/networking/ip-sysctl.txt |    4 ++
 net/bridge/Makefile                    |    2 -
 net/bridge/br.c                        |   12 ++++++
 net/bridge/br_input.c                  |   30 ++++++++++++++++-
 net/bridge/br_private.h                |    5 ++
 net/bridge/br_sysctl.c                 |   57 +++++++++++++++++++++++++++++++++
 6 files changed, 107 insertions(+), 3 deletions(-)

--- a/Documentation/networking/ip-sysctl.txt	2011-08-15 10:58:36.451532115 -0700
+++ b/Documentation/networking/ip-sysctl.txt	2011-08-15 11:39:57.719438766 -0700
@@ -1289,6 +1289,10 @@ bridge-nf-filter-pppoe-tagged - BOOLEAN
 	0 : disable this.
 	Default: 1
 
+bridge-forward-link-local - BOOLEAN
+	1 : pass link-local multicasts through bridge in STP mode
+	0 : disable this.
+	Default: 0
 
 proc/sys/net/sctp/* Variables:
 
--- a/net/bridge/Makefile	2011-08-15 10:30:25.203595742 -0700
+++ b/net/bridge/Makefile	2011-08-15 11:22:38.139477877 -0700
@@ -9,7 +9,7 @@ bridge-y	:= br.o br_device.o br_fdb.o br
 			br_stp_if.o br_stp_timer.o br_netlink.o
 
 bridge-$(CONFIG_SYSFS) += br_sysfs_if.o br_sysfs_br.o
-
+bridge-$(CONFIG_SYSCTL) += br_sysctl.o
 bridge-$(CONFIG_BRIDGE_NETFILTER) += br_netfilter.o
 
 bridge-$(CONFIG_BRIDGE_IGMP_SNOOPING) += br_multicast.o
--- a/net/bridge/br.c	2011-08-15 10:30:48.755594855 -0700
+++ b/net/bridge/br.c	2011-08-15 10:33:07.215589647 -0700
@@ -60,6 +60,12 @@ static int __init br_init(void)
 	if (err)
 		goto err_out4;
 
+#ifdef CONFIG_SYSCTL
+	err = br_sysctl_init();
+	if (err)
+		goto err_out5;
+#endif
+
 	brioctl_set(br_ioctl_deviceless_stub);
 
 #if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
@@ -67,6 +73,9 @@ static int __init br_init(void)
 #endif
 
 	return 0;
+
+err_out5:
+	br_netlink_fini();
 err_out4:
 	unregister_netdevice_notifier(&br_device_notifier);
 err_out3:
@@ -84,6 +93,9 @@ static void __exit br_deinit(void)
 {
 	stp_proto_unregister(&br_stp_proto);
 
+#ifdef CONFIG_SYSCTL
+	br_sysctl_fini();
+#endif
 	br_netlink_fini();
 	unregister_netdevice_notifier(&br_device_notifier);
 	brioctl_set(NULL);
--- a/net/bridge/br_input.c	2011-08-15 10:40:21.435573311 -0700
+++ b/net/bridge/br_input.c	2011-08-15 11:39:57.719438766 -0700
@@ -16,11 +16,18 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/netfilter_bridge.h>
+#include <linux/llc.h>
+#include <net/llc.h>
+#include <net/llc_pdu.h>
+
 #include "br_private.h"
 
 /* Bridge group multicast address 802.1d (pg 51). */
 const u8 br_group_address[ETH_ALEN] = { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 };
 
+/* Should link-local packets be forwarded (in STP mode) */
+int br_forward_link_local;
+
 /* Hook for brouter */
 br_should_route_hook_t __rcu *br_should_route_hook __read_mostly;
 EXPORT_SYMBOL(br_should_route_hook);
@@ -138,6 +145,17 @@ static inline int is_link_local(const un
 	return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | ((a[2] ^ b[2]) & m)) == 0;
 }
 
+/* Identify Spanning Tree packets based on header */
+static bool is_stp_bpdu(struct sk_buff *skb)
+{
+	struct llc_pdu_un *pdu = llc_pdu_un_hdr(skb);
+
+	return skb->protocol == htons(ETH_P_802_2) &&
+		pdu->ctrl_1 == LLC_PDU_TYPE_U &&
+		pdu->dsap == LLC_SAP_BSPAN &&
+		pdu->ssap == LLC_SAP_BSPAN;
+}
+
 /*
  * Return NULL if skb is handled
  * note: already called with rcu_read_lock
@@ -166,8 +184,16 @@ rx_handler_result_t br_handle_frame(stru
 		if (skb->protocol == htons(ETH_P_PAUSE))
 			goto drop;
 
-		/* If STP is turned off, then forward */
-		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
+		/* If STP is turned off, then in hub mode */
+		if (p->br->stp_enabled == BR_NO_STP)
+			goto forward;
+
+		/*
+		 * If STP is on
+		 * then Always handle STP packets locally,
+		 *      other packets can be forwarded if sysctl is enabled.
+		 */
+		if (!is_stp_bpdu(skb) && br_forward_link_local)
 			goto forward;
 
 		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
--- a/net/bridge/br_private.h	2011-08-15 10:38:35.587577293 -0700
+++ b/net/bridge/br_private.h	2011-08-15 10:57:36.983534352 -0700
@@ -284,6 +284,7 @@ struct br_input_skb_cb {
 	pr_debug("%s: " format,  (br)->dev->name, ##args)
 
 extern struct notifier_block br_device_notifier;
+extern int br_forward_link_local;
 extern const u8 br_group_address[ETH_ALEN];
 
 /* called under bridge lock */
@@ -546,6 +547,10 @@ extern int br_sysfs_renameif(struct net_
 extern int br_sysfs_addbr(struct net_device *dev);
 extern void br_sysfs_delbr(struct net_device *dev);
 
+/* br_sysctl.c */
+extern int br_sysctl_init(void);
+extern void br_sysctl_fini(void);
+
 #else
 
 #define br_sysfs_addif(p)	(0)
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/net/bridge/br_sysctl.c	2011-08-15 11:41:00.819436393 -0700
@@ -0,0 +1,57 @@
+/*
+ *	Sysctl settings for bridge
+ *
+ *	Authors:
+ *	Stephen Hemminger		<shemminger@osdl.org>
+ *
+ *	This program is free software; you can redistribute it and/or
+ *	modify it under the terms of the GNU General Public License
+ *	as published by the Free Software Foundation; either version
+ *	2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/ip.h>
+#include <linux/netdevice.h>
+#include <linux/skbuff.h>
+#include <linux/if_arp.h>
+#include <linux/if_ether.h>
+#include <linux/if_vlan.h>
+#include <linux/if_pppox.h>
+#include <linux/sysctl.h>
+
+#include "br_private.h"
+
+static struct ctl_table bridge_table[] = {
+	{
+		.procname	= "bridge-forward-link-local",
+		.data		= &br_forward_link_local,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
+};
+
+static struct ctl_path bridge_ctl_path[] = {
+	{ .procname = "net", },
+	{ .procname = "bridge", },
+	{ },
+};
+
+static struct ctl_table_header *br_sysctl;
+
+int __init br_sysctl_init(void)
+{
+	br_sysctl = register_sysctl_paths(bridge_ctl_path, bridge_table);
+	if (br_sysctl == NULL)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void __exit br_sysctl_fini(void)
+{
+	unregister_net_sysctl_table(br_sysctl);
+}

Re: [RFC] bridge: allow passing link-local multicast

[Nick Carter]

On 15 August 2011 23:05, Stephen Hemminger <shemminger@vyatta.com> wrote:
> Several users have wanted to forward 802.1x EAP multicast
> packets through a bridge. And there has been a couple of
> attempts at allowing some form of this in the past.
>
> If a bridge does not have spanning tree turned on, then it should
> act like a pure hub and forward all traffic. This makes it fully
> transparent, and if there is another bridge using spanning tree
> the STP packets will still work for detecting loops in the network.
>
> If bridge has STP enabled, then the default behavior is to
> process all link-local multicasts locally. The expectation is
> that if 802.1x or other protocol using link-local multicasts
> that a service (or proxy) for that protocol will be used.
>
> Optionally, a sysctl value can be set to allow non STP packets
> to still be forwarded.  I chose sysctl for this because it is
> where such modifications exist when doing IP or netfilter.
> There are other filtering/configuration options that are needed
> and this is a better way to enable them.
>
> Thanks to David Lamparter, and others for bringing this up.
> Users who need this facility should provide feedback, is this
> a usable solution?
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
> ---
> Patch against net-next
>
>  Documentation/networking/ip-sysctl.txt |    4 ++
>  net/bridge/Makefile                    |    2 -
>  net/bridge/br.c                        |   12 ++++++
>  net/bridge/br_input.c                  |   30 ++++++++++++++++-
>  net/bridge/br_private.h                |    5 ++
>  net/bridge/br_sysctl.c                 |   57 +++++++++++++++++++++++++++++++++
>  6 files changed, 107 insertions(+), 3 deletions(-)
>
> --- a/Documentation/networking/ip-sysctl.txt    2011-08-15 10:58:36.451532115 -0700
> +++ b/Documentation/networking/ip-sysctl.txt    2011-08-15 11:39:57.719438766 -0700
> @@ -1289,6 +1289,10 @@ bridge-nf-filter-pppoe-tagged - BOOLEAN
>        0 : disable this.
>        Default: 1
>
> +bridge-forward-link-local - BOOLEAN
> +       1 : pass link-local multicasts through bridge in STP mode
> +       0 : disable this.
> +       Default: 0
>
>  proc/sys/net/sctp/* Variables:
>
> --- a/net/bridge/Makefile       2011-08-15 10:30:25.203595742 -0700
> +++ b/net/bridge/Makefile       2011-08-15 11:22:38.139477877 -0700
> @@ -9,7 +9,7 @@ bridge-y        := br.o br_device.o br_fdb.o br
>                        br_stp_if.o br_stp_timer.o br_netlink.o
>
>  bridge-$(CONFIG_SYSFS) += br_sysfs_if.o br_sysfs_br.o
> -
> +bridge-$(CONFIG_SYSCTL) += br_sysctl.o
>  bridge-$(CONFIG_BRIDGE_NETFILTER) += br_netfilter.o
>
>  bridge-$(CONFIG_BRIDGE_IGMP_SNOOPING) += br_multicast.o
> --- a/net/bridge/br.c   2011-08-15 10:30:48.755594855 -0700
> +++ b/net/bridge/br.c   2011-08-15 10:33:07.215589647 -0700
> @@ -60,6 +60,12 @@ static int __init br_init(void)
>        if (err)
>                goto err_out4;
>
> +#ifdef CONFIG_SYSCTL
> +       err = br_sysctl_init();
> +       if (err)
> +               goto err_out5;
> +#endif
> +
>        brioctl_set(br_ioctl_deviceless_stub);
>
>  #if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
> @@ -67,6 +73,9 @@ static int __init br_init(void)
>  #endif
>
>        return 0;
> +
> +err_out5:
> +       br_netlink_fini();
>  err_out4:
>        unregister_netdevice_notifier(&br_device_notifier);
>  err_out3:
> @@ -84,6 +93,9 @@ static void __exit br_deinit(void)
>  {
>        stp_proto_unregister(&br_stp_proto);
>
> +#ifdef CONFIG_SYSCTL
> +       br_sysctl_fini();
> +#endif
>        br_netlink_fini();
>        unregister_netdevice_notifier(&br_device_notifier);
>        brioctl_set(NULL);
> --- a/net/bridge/br_input.c     2011-08-15 10:40:21.435573311 -0700
> +++ b/net/bridge/br_input.c     2011-08-15 11:39:57.719438766 -0700
> @@ -16,11 +16,18 @@
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
>  #include <linux/netfilter_bridge.h>
> +#include <linux/llc.h>
> +#include <net/llc.h>
> +#include <net/llc_pdu.h>
> +
>  #include "br_private.h"
>
>  /* Bridge group multicast address 802.1d (pg 51). */
>  const u8 br_group_address[ETH_ALEN] = { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 };
>
> +/* Should link-local packets be forwarded (in STP mode) */
> +int br_forward_link_local;
> +
>  /* Hook for brouter */
>  br_should_route_hook_t __rcu *br_should_route_hook __read_mostly;
>  EXPORT_SYMBOL(br_should_route_hook);
> @@ -138,6 +145,17 @@ static inline int is_link_local(const un
>        return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | ((a[2] ^ b[2]) & m)) == 0;
>  }
>
> +/* Identify Spanning Tree packets based on header */
Why can't we use the 802.1D specified STP group address to identify ?
The existing code uses that address.
I know you said on another thread that there are people using other addresses.
Who are these people ?
Are they following any standard ?
What address / address range are they using ?

Thanks,
Nick
> +static bool is_stp_bpdu(struct sk_buff *skb)
> +{
> +       struct llc_pdu_un *pdu = llc_pdu_un_hdr(skb);
> +
> +       return skb->protocol == htons(ETH_P_802_2) &&
> +               pdu->ctrl_1 == LLC_PDU_TYPE_U &&
> +               pdu->dsap == LLC_SAP_BSPAN &&
> +               pdu->ssap == LLC_SAP_BSPAN;
> +}
> +
>  /*
>  * Return NULL if skb is handled
>  * note: already called with rcu_read_lock
> @@ -166,8 +184,16 @@ rx_handler_result_t br_handle_frame(stru
>                if (skb->protocol == htons(ETH_P_PAUSE))
>                        goto drop;
>
> -               /* If STP is turned off, then forward */
> -               if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
> +               /* If STP is turned off, then in hub mode */
> +               if (p->br->stp_enabled == BR_NO_STP)
> +                       goto forward;
> +
> +               /*
> +                * If STP is on
> +                * then Always handle STP packets locally,
> +                *      other packets can be forwarded if sysctl is enabled.
> +                */
> +               if (!is_stp_bpdu(skb) && br_forward_link_local)
>                        goto forward;
>
>                if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
> --- a/net/bridge/br_private.h   2011-08-15 10:38:35.587577293 -0700
> +++ b/net/bridge/br_private.h   2011-08-15 10:57:36.983534352 -0700
> @@ -284,6 +284,7 @@ struct br_input_skb_cb {
>        pr_debug("%s: " format,  (br)->dev->name, ##args)
>
>  extern struct notifier_block br_device_notifier;
> +extern int br_forward_link_local;
>  extern const u8 br_group_address[ETH_ALEN];
>
>  /* called under bridge lock */
> @@ -546,6 +547,10 @@ extern int br_sysfs_renameif(struct net_
>  extern int br_sysfs_addbr(struct net_device *dev);
>  extern void br_sysfs_delbr(struct net_device *dev);
>
> +/* br_sysctl.c */
> +extern int br_sysctl_init(void);
> +extern void br_sysctl_fini(void);
> +
>  #else
>
>  #define br_sysfs_addif(p)      (0)
> --- /dev/null   1970-01-01 00:00:00.000000000 +0000
> +++ b/net/bridge/br_sysctl.c    2011-08-15 11:41:00.819436393 -0700
> @@ -0,0 +1,57 @@
> +/*
> + *     Sysctl settings for bridge
> + *
> + *     Authors:
> + *     Stephen Hemminger               <shemminger@osdl.org>
> + *
> + *     This program is free software; you can redistribute it and/or
> + *     modify it under the terms of the GNU General Public License
> + *     as published by the Free Software Foundation; either version
> + *     2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/ip.h>
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>
> +#include <linux/if_arp.h>
> +#include <linux/if_ether.h>
> +#include <linux/if_vlan.h>
> +#include <linux/if_pppox.h>
> +#include <linux/sysctl.h>
> +
> +#include "br_private.h"
> +
> +static struct ctl_table bridge_table[] = {
> +       {
> +               .procname       = "bridge-forward-link-local",
> +               .data           = &br_forward_link_local,
> +               .maxlen         = sizeof(int),
> +               .mode           = 0644,
> +               .proc_handler   = proc_dointvec
> +       },
> +};
> +
> +static struct ctl_path bridge_ctl_path[] = {
> +       { .procname = "net", },
> +       { .procname = "bridge", },
> +       { },
> +};
> +
> +static struct ctl_table_header *br_sysctl;
> +
> +int __init br_sysctl_init(void)
> +{
> +       br_sysctl = register_sysctl_paths(bridge_ctl_path, bridge_table);
> +       if (br_sysctl == NULL)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +
> +void __exit br_sysctl_fini(void)
> +{
> +       unregister_net_sysctl_table(br_sysctl);
> +}
>

Re: [RFC] bridge: allow passing link-local multicast

[Stephen Hemminger]

On Thu, 18 Aug 2011 16:06:19 +0100
Nick Carter <ncarter100@gmail.com> wrote:

> Why can't we use the 802.1D specified STP group address to identify ?
> The existing code uses that address.
> I know you said on another thread that there are people using other addresses.
> Who are these people ?
> Are they following any standard ?
> What address / address range are they using ?

The group address can be reprogrammed, and it is settable on other
routing equipment. People do it to create spanning tree domains.

Re: [RFC] bridge: allow passing link-local multicast

[Nick Carter]

On 18 August 2011 16:10, Stephen Hemminger <shemminger@vyatta.com> wrote:
> On Thu, 18 Aug 2011 16:06:19 +0100
> Nick Carter <ncarter100@gmail.com> wrote:
>
>> Why can't we use the 802.1D specified STP group address to identify ?
>> The existing code uses that address.
>> I know you said on another thread that there are people using other addresses.
>> Who are these people ?
>> Are they following any standard ?
>> What address / address range are they using ?
>
> The group address can be reprogrammed, and it is settable on other
> routing equipment. People do it to create spanning tree domains.
>
But before the new
+               if (!is_stp_bpdu(skb) && br_forward_link_local)
check, we have already checked
	if (unlikely(is_link_local(dest))) {
So the frame must have a link local destination.  If the reprogrammed
group address is outside of the link local range then the new code in
this patch will never be hit.  If the reprogrammed group address is in
the link local range then i'd suggest my previous group_fwd_mask patch
is cleaner and more flexible.

Nick

Re: [RFC] bridge: allow passing link-local multicast

[Stephen Hemminger]

On Thu, 18 Aug 2011 16:52:45 +0100
Nick Carter <ncarter100@gmail.com> wrote:

> On 18 August 2011 16:10, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > On Thu, 18 Aug 2011 16:06:19 +0100
> > Nick Carter <ncarter100@gmail.com> wrote:
> >
> >> Why can't we use the 802.1D specified STP group address to identify ?
> >> The existing code uses that address.
> >> I know you said on another thread that there are people using other addresses.
> >> Who are these people ?
> >> Are they following any standard ?
> >> What address / address range are they using ?
> >
> > The group address can be reprogrammed, and it is settable on other
> > routing equipment. People do it to create spanning tree domains.
> >
> But before the new
> +               if (!is_stp_bpdu(skb) && br_forward_link_local)
> check, we have already checked
> 	if (unlikely(is_link_local(dest))) {
> So the frame must have a link local destination.  If the reprogrammed
> group address is outside of the link local range then the new code in
> this patch will never be hit.  If the reprogrammed group address is in
> the link local range then i'd suggest my previous group_fwd_mask patch
> is cleaner and more flexible.

The problem is that the group_fwd_mask is specific to the address
not the protocol.

Re: [RFC] bridge: allow passing link-local multicast

[David Lamparter]

On Thu, Aug 18, 2011 at 09:39:41AM -0700, Stephen Hemminger wrote:
> On Thu, 18 Aug 2011 16:52:45 +0100
> Nick Carter <ncarter100@gmail.com> wrote:
> 
> > On 18 August 2011 16:10, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > > On Thu, 18 Aug 2011 16:06:19 +0100
> > > Nick Carter <ncarter100@gmail.com> wrote:
> > >
> > >> Why can't we use the 802.1D specified STP group address to identify ?
> > >> The existing code uses that address.
> > >> I know you said on another thread that there are people using other addresses.
> > >> Who are these people ?
> > >> Are they following any standard ?
> > >> What address / address range are they using ?
> > >
> > > The group address can be reprogrammed, and it is settable on other
> > > routing equipment. People do it to create spanning tree domains.

I just checked my hardware switches. Neither my Cisco devices nor my HP
ProCurves can reprogram the spanning tree address.

Can you provide an example? Even Linux can't do this, can it?

> > But before the new
> > +               if (!is_stp_bpdu(skb) && br_forward_link_local)
> > check, we have already checked
> > 	if (unlikely(is_link_local(dest))) {
> > So the frame must have a link local destination.  If the reprogrammed
> > group address is outside of the link local range then the new code in
> > this patch will never be hit.  If the reprogrammed group address is in
> > the link local range then i'd suggest my previous group_fwd_mask patch
> > is cleaner and more flexible.
> 
> The problem is that the group_fwd_mask is specific to the address
> not the protocol.

Until corrected, I would consider either match a valid option, and I
would say Nick's patch is much cleaner than yours.

Also, you're arguing against yourself, saying that matching the address
is a problem, but the code you're proposing does exactly that - it first
matches the address (is_link_local()), then the protocol. (wtf?)


-David

[RFC] bridge: add netfilter hook for forwarding 802.1D group addresses

[Stephen Hemminger]

The IEEE standard expects that link local multicast packets will not
be forwarded by a bridge. But there are cases like 802.1X which may
require that packets be forwarded. For maximum flexibilty implement
this via netfilter.

The netfilter chain is slightly different from other chains in that
if packet is ACCEPTED by the chain, it means it should be forwarded.
And if the packet verdict result is DROP, the packet is processed
as a local packet. The default result for this chain is DROP and
therefore users who do not install any rules will get the same
result as before; ie. packets are only processed on the local host
and not forwarded.

Spanning Tree Packets are treated specially and do not
go through the new chain.

This code is conceptual design concept only. It compiles but
hasn't been tested.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 include/linux/netfilter_bridge.h      |    5 ++++-
 net/bridge/br_input.c                 |   15 ++++++++++++---
 net/bridge/netfilter/ebtable_filter.c |   18 ++++++++++++++++--
 3 files changed, 32 insertions(+), 6 deletions(-)

--- a/include/linux/netfilter_bridge.h	2011-08-19 13:11:51.972125670 -0700
+++ b/include/linux/netfilter_bridge.h	2011-08-19 13:13:36.452130443 -0700
@@ -22,7 +22,10 @@
 #define NF_BR_POST_ROUTING	4
 /* Not really a hook, but used for the ebtables broute table */
 #define NF_BR_BROUTING		5
-#define NF_BR_NUMHOOKS		6
+/* Packets to link local multicast addresses (01-80-C2-00-00-XX) */
+#define NF_BR_LINK_LOCAL_IN	6
+
+#define NF_BR_NUMHOOKS		7
 
 #ifdef __KERNEL__
 
--- a/net/bridge/br_input.c	2011-08-18 16:12:02.576672548 -0700
+++ b/net/bridge/br_input.c	2011-08-19 13:28:13.696170518 -0700
@@ -166,10 +166,19 @@ rx_handler_result_t br_handle_frame(stru
 		if (skb->protocol == htons(ETH_P_PAUSE))
 			goto drop;
 
-		/* If STP is turned off, then forward */
-		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
-			goto forward;
+		/* If this is Spanning Tree Protocol packet */
+		if (dest[5] == 0) {
+			/* and STP is turned off, then forward */
+			if (p->br->stp_enabled == BR_NO_STP)
+				goto forward;
+		}
+		/* Hook to allow forwarding other group MAC addresses */
+		else if (p->state == BR_STATE_FORWARDING &&
+			 NF_HOOK(NFPROTO_BRIDGE, NF_BR_LINK_LOCAL_IN, skb, skb->dev,
+				 NULL, br_handle_frame_finish))
+			return RX_HANDLER_CONSUMED;	/* forwarded */
 
+		/* Packet will go only to the local host. */
 		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
 			    NULL, br_handle_local_finish)) {
 			return RX_HANDLER_CONSUMED; /* consumed by filter */
--- a/net/bridge/netfilter/ebtable_filter.c	2011-08-19 13:14:46.232133631 -0700
+++ b/net/bridge/netfilter/ebtable_filter.c	2011-08-19 13:27:33.436168679 -0700
@@ -11,8 +11,10 @@
 #include <linux/netfilter_bridge/ebtables.h>
 #include <linux/module.h>
 
-#define FILTER_VALID_HOOKS ((1 << NF_BR_LOCAL_IN) | (1 << NF_BR_FORWARD) | \
-   (1 << NF_BR_LOCAL_OUT))
+#define FILTER_VALID_HOOKS ((1 << NF_BR_LOCAL_IN) | \
+			    (1 << NF_BR_FORWARD) | \
+			    (1 << NF_BR_LOCAL_OUT) | \
+			    (1 << NF_BR_LINK_LOCAL_IN))
 
 static struct ebt_entries initial_chains[] =
 {
@@ -28,6 +30,10 @@ static struct ebt_entries initial_chains
 		.name	= "OUTPUT",
 		.policy	= EBT_ACCEPT,
 	},
+	{
+		.name	= "LINKLOCAL",
+		.policy = EBT_DROP,
+	},
 };
 
 static struct ebt_replace_kernel initial_table =
@@ -39,6 +45,7 @@ static struct ebt_replace_kernel initial
 		[NF_BR_LOCAL_IN]	= &initial_chains[0],
 		[NF_BR_FORWARD]		= &initial_chains[1],
 		[NF_BR_LOCAL_OUT]	= &initial_chains[2],
+		[NF_BR_LINK_LOCAL_IN]	= &initial_chains[3],
 	},
 	.entries	= (char *)initial_chains,
 };
@@ -95,6 +102,13 @@ static struct nf_hook_ops ebt_ops_filter
 		.hooknum	= NF_BR_LOCAL_OUT,
 		.priority	= NF_BR_PRI_FILTER_OTHER,
 	},
+	{
+		.hook		= ebt_in_hook,
+		.owner		= THIS_MODULE,
+		.pf		= NFPROTO_BRIDGE,
+		.hooknum	= NF_BR_LINK_LOCAL_IN,
+		.priority	= NF_BR_PRI_FILTER_BRIDGED,
+	},
 };
 
 static int __net_init frame_filter_net_init(struct net *net)

RE: [RFC] bridge: add netfilter hook for forwarding 802.1D group addresses

[Christian Benvenuti (benve)]

The patch description and the code are clearly saying that STP is
an exception, but I am just worried about the users.
Maybe a proper description in the iptables help is sufficient.

Users may otherwise try to use this new hook for STP too
(for example to generate logs or produce statistics/counters
or divert STP traffic to userspace, etc).

Out of curiosity, ... if this gets accepted, shouldn't you provide
NF_BR_LINK_LOCAL_OUT too?
Or maybe you should call it NF_BR_LINK_LOCAL_FWD instead of
NF_BR_LINK_LOCAL_IN?

/Chris

> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org [mailto:netfilter-devel-
> owner@vger.kernel.org] On Behalf Of Stephen Hemminger
> Sent: Friday, August 19, 2011 1:58 PM
> To: David Lamparter
> Cc: Nick Carter; Ed Swierk; netdev@vger.kernel.org; bridge@linux-
> foundation.org; netfilter-devel@vger.kernel.org
> Subject: [RFC] bridge: add netfilter hook for forwarding 802.1D group
> addresses
> 
> The IEEE standard expects that link local multicast packets will not
> be forwarded by a bridge. But there are cases like 802.1X which may
> require that packets be forwarded. For maximum flexibilty implement
> this via netfilter.
> 
> The netfilter chain is slightly different from other chains in that
> if packet is ACCEPTED by the chain, it means it should be forwarded.
> And if the packet verdict result is DROP, the packet is processed
> as a local packet. The default result for this chain is DROP and
> therefore users who do not install any rules will get the same
> result as before; ie. packets are only processed on the local host
> and not forwarded.
> 
> Spanning Tree Packets are treated specially and do not
> go through the new chain.
> 
> This code is conceptual design concept only. It compiles but
> hasn't been tested.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
>  include/linux/netfilter_bridge.h      |    5 ++++-
>  net/bridge/br_input.c                 |   15 ++++++++++++---
>  net/bridge/netfilter/ebtable_filter.c |   18 ++++++++++++++++--
>  3 files changed, 32 insertions(+), 6 deletions(-)
> 
> --- a/include/linux/netfilter_bridge.h	2011-08-19
13:11:51.972125670
> -0700
> +++ b/include/linux/netfilter_bridge.h	2011-08-19
13:13:36.452130443
> -0700
> @@ -22,7 +22,10 @@
>  #define NF_BR_POST_ROUTING	4
>  /* Not really a hook, but used for the ebtables broute table */
>  #define NF_BR_BROUTING		5
> -#define NF_BR_NUMHOOKS		6
> +/* Packets to link local multicast addresses (01-80-C2-00-00-XX) */
> +#define NF_BR_LINK_LOCAL_IN	6
> +
> +#define NF_BR_NUMHOOKS		7
> 
>  #ifdef __KERNEL__
> 
> --- a/net/bridge/br_input.c	2011-08-18 16:12:02.576672548 -0700
> +++ b/net/bridge/br_input.c	2011-08-19 13:28:13.696170518 -0700
> @@ -166,10 +166,19 @@ rx_handler_result_t br_handle_frame(stru
>  		if (skb->protocol == htons(ETH_P_PAUSE))
>  			goto drop;
> 
> -		/* If STP is turned off, then forward */
> -		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
> -			goto forward;
> +		/* If this is Spanning Tree Protocol packet */
> +		if (dest[5] == 0) {
> +			/* and STP is turned off, then forward */
> +			if (p->br->stp_enabled == BR_NO_STP)
> +				goto forward;
> +		}
> +		/* Hook to allow forwarding other group MAC addresses */
> +		else if (p->state == BR_STATE_FORWARDING &&
> +			 NF_HOOK(NFPROTO_BRIDGE, NF_BR_LINK_LOCAL_IN,
skb,
> skb->dev,
> +				 NULL, br_handle_frame_finish))
> +			return RX_HANDLER_CONSUMED;	/* forwarded */
> 
> +		/* Packet will go only to the local host. */
>  		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb,
skb->dev,
>  			    NULL, br_handle_local_finish)) {
>  			return RX_HANDLER_CONSUMED; /* consumed by
filter */
> --- a/net/bridge/netfilter/ebtable_filter.c	2011-08-19
> 13:14:46.232133631 -0700
> +++ b/net/bridge/netfilter/ebtable_filter.c	2011-08-19
> 13:27:33.436168679 -0700
> @@ -11,8 +11,10 @@
>  #include <linux/netfilter_bridge/ebtables.h>
>  #include <linux/module.h>
> 
> -#define FILTER_VALID_HOOKS ((1 << NF_BR_LOCAL_IN) | (1 <<
> NF_BR_FORWARD) | \
> -   (1 << NF_BR_LOCAL_OUT))
> +#define FILTER_VALID_HOOKS ((1 << NF_BR_LOCAL_IN) | \
> +			    (1 << NF_BR_FORWARD) | \
> +			    (1 << NF_BR_LOCAL_OUT) | \
> +			    (1 << NF_BR_LINK_LOCAL_IN))
> 
>  static struct ebt_entries initial_chains[] =
>  {
> @@ -28,6 +30,10 @@ static struct ebt_entries initial_chains
>  		.name	= "OUTPUT",
>  		.policy	= EBT_ACCEPT,
>  	},
> +	{
> +		.name	= "LINKLOCAL",
> +		.policy = EBT_DROP,
> +	},
>  };
> 
>  static struct ebt_replace_kernel initial_table =
> @@ -39,6 +45,7 @@ static struct ebt_replace_kernel initial
>  		[NF_BR_LOCAL_IN]	= &initial_chains[0],
>  		[NF_BR_FORWARD]		= &initial_chains[1],
>  		[NF_BR_LOCAL_OUT]	= &initial_chains[2],
> +		[NF_BR_LINK_LOCAL_IN]	= &initial_chains[3],
>  	},
>  	.entries	= (char *)initial_chains,
>  };
> @@ -95,6 +102,13 @@ static struct nf_hook_ops ebt_ops_filter
>  		.hooknum	= NF_BR_LOCAL_OUT,
>  		.priority	= NF_BR_PRI_FILTER_OTHER,
>  	},
> +	{
> +		.hook		= ebt_in_hook,
> +		.owner		= THIS_MODULE,
> +		.pf		= NFPROTO_BRIDGE,
> +		.hooknum	= NF_BR_LINK_LOCAL_IN,
> +		.priority	= NF_BR_PRI_FILTER_BRIDGED,
> +	},
>  };
> 
>  static int __net_init frame_filter_net_init(struct net *net)
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-
> devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] bridge: add netfilter hook for forwarding 802.1D group addresses

[Stephen Hemminger]

On Fri, 19 Aug 2011 17:18:04 -0500
"Christian Benvenuti (benve)" <benve@cisco.com> wrote:

> The patch description and the code are clearly saying that STP is
> an exception, but I am just worried about the users.
> Maybe a proper description in the iptables help is sufficient.
> 
> Users may otherwise try to use this new hook for STP too
> (for example to generate logs or produce statistics/counters
> or divert STP traffic to userspace, etc).

STP traffic already goes to userspace. And gets processed
by the LOCAL_IN chain. So I don't think it is needed.


> Out of curiosity, ... if this gets accepted, shouldn't you provide
> NF_BR_LINK_LOCAL_OUT too?
> Or maybe you should call it NF_BR_LINK_LOCAL_FWD instead of
> NF_BR_LINK_LOCAL_IN?

Thanks, that is a better name, I'll change it in next version.

Re: [RFC] bridge: add netfilter hook for forwarding 802.1D group addresses

[Bart De Schuymer]

Op 19/08/2011 22:58, Stephen Hemminger schreef:

> Signed-off-by: Stephen Hemminger<shemminger@vyatta.com>
>
> ---
>   include/linux/netfilter_bridge.h      |    5 ++++-
>   net/bridge/br_input.c                 |   15 ++++++++++++---
>   net/bridge/netfilter/ebtable_filter.c |   18 ++++++++++++++++--
>   3 files changed, 32 insertions(+), 6 deletions(-)
>
> --- a/include/linux/netfilter_bridge.h	2011-08-19 13:11:51.972125670 -0700
> +++ b/include/linux/netfilter_bridge.h	2011-08-19 13:13:36.452130443 -0700
> @@ -22,7 +22,10 @@
>   #define NF_BR_POST_ROUTING	4
>   /* Not really a hook, but used for the ebtables broute table */
>   #define NF_BR_BROUTING		5
> -#define NF_BR_NUMHOOKS		6
> +/* Packets to link local multicast addresses (01-80-C2-00-00-XX) */
> +#define NF_BR_LINK_LOCAL_IN	6
> +
> +#define NF_BR_NUMHOOKS		7
>

You will need to make sure you don't break backwards compatibility with 
the ebtables userspace tool. ebtables.h::struct ebt_replace is a 
structure used for communication between userspace and the kernel. It 
has the member hook_entry defined like this:
struct ebt_entries __user *hook_entry[NF_BR_NUMHOOKS];

cheers,
Bart



-- 
Bart De Schuymer
www.artinalgorithms.be

Re: [RFC] bridge: add netfilter hook for forwarding 802.1D group addresses

[David Lamparter]

On Fri, Aug 19, 2011 at 01:58:10PM -0700, Stephen Hemminger wrote:
> The IEEE standard expects that link local multicast packets will not
> be forwarded by a bridge. But there are cases like 802.1X which may
> require that packets be forwarded. For maximum flexibilty implement
> this via netfilter.
> 
> The netfilter chain is slightly different from other chains in that
> if packet is ACCEPTED by the chain, it means it should be forwarded.
> And if the packet verdict result is DROP, the packet is processed
> as a local packet.

Exactly this functionality already exists by way of the BROUTING chain
in the broute table. Currently, link-local packets are hardcodedly
treated as local before they even reach that chain. Nick's patch, in
conjunction with BROUTING, provides exactly what you're trying to do.

Now, without bridge netfilter, your patch becomes rather useless while
Nick's patch still allows per-group (and therefore per-protocol)
control.

Further, Nick's patch is considerably less intrusive.

I would therefore ask for Nick's patch to be merged.


-David


P.S.: this whole issue is starting to get rather annoying