[PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

Signed-off-by: Nick Carter <redacted>

This Kconfig option is used to enable a bridge to forward 802.1x
(EAPOL) Port Access Entity (PAE) frames.  One use of this would be to
enable 802.1x authentication between a PAE supplicant running inside a
virtual machine, with the EAPOL frames bridged out to an external PAE
authenticator.

If BRIDGE_PAE_FORWARD is not set the behaviour of bridge.ko is unchanged.

If BRIDGE_PAE_FORWARD is set then by default the only new behaviour is
that unicast EAPOL frames attempting to traverse the bridge will be
dropped.  This makes the bridge standards compliant by preventing
crosstalk (IEEE Std 802.1X-2001 C.3.3).

Writing a 1 to the new sysfs attribute ../bridge/pae_forward will
enable the forwarding of EAPOL frames, both unicast and link local
multicast (01-80-C2-00-00-03).

diff --git a/net/bridge/Kconfig b/net/bridge/Kconfig
index 6dee7bf..c47a49e 100644
--- a/net/bridge/Kconfig
+++ b/net/bridge/Kconfig

@@ -46,3 +46,22 @@ config BRIDGE_IGMP_SNOOPING
 	  Say N to exclude this support and reduce the binary size.

 	  If unsure, say Y.
+
+config BRIDGE_PAE_FORWARD
+	bool "PAE Forwarding"
+	depends on BRIDGE
+	default n
+	---help---
+	  If you say Y here, then the Ethernet bridge will be able to forward
+	  802.1x (EAPOL) Port Access Entity (PAE) frames.  One use of this would
+	  be to enable 802.1x authentication between a PAE supplicant running
+	  inside a virtual machine, with the EAPOL frames bridged out to an
+	  external PAE authenticator.
+
+	  On a running kernel with this support, enable PAE forwarding by
+	  writing a '1' to the bridge devices pae_forward attribute.
+	  e.g. echo 1 > /sys/devices/virtual/net/br73/bridge/pae_forward
+
+	  Say N to exclude this support.
+
+	  If unsure, say N.

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..b493474 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -214,6 +214,9 @@ static struct net_device *new_bridge_dev(struct

net *net, const char *name)
 	br->topology_change = 0;
 	br->topology_change_detected = 0;
 	br->ageing_time = 300 * HZ;
+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+	br->pae_forward = BR_PAE_DEFAULT;
+#endif

 	br_netfilter_rtable_init(br);

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 90e985b..183c40f 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -43,6 +43,24 @@ static int br_pass_frame_up(struct sk_buff *skb)
 		       netif_receive_skb);
 }

+static inline bool br_pae_forward(struct net_bridge *br, __be16 proto)
+{
+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+	return br->pae_forward == BR_PAE_FORWARD && proto == htons(ETH_P_PAE);
+#else
+	return false;
+#endif
+}
+
+static inline bool br_pae_drop(struct net_bridge *br, __be16 proto)
+{
+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+	return br->pae_forward == BR_PAE_DEFAULT && proto == htons(ETH_P_PAE);
+#else
+	return false;
+#endif
+}
+
 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {

@@ -98,6 +116,10 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	}

 	if (skb) {
+		/* Prevent Crosstalk (IEEE Std 802.1X-2001 C.3.3) */
+		if (unlikely(br_pae_drop(br, skb->protocol)))
+			goto drop;
+
 		if (dst)
 			br_forward(dst->dst, skb, skb2);
 		else

@@ -166,6 +188,10 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)
 		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
 			goto forward;

+		/* Check if PAE frame should be forwarded */
+		if (br_pae_forward(p->br, skb->protocol))
+			goto forward;
+
 		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
 			    NULL, br_handle_local_finish))
 			return NULL;	/* frame consumed by filter */

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..a523032 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -244,6 +244,13 @@ struct net_bridge
 	struct timer_list		multicast_query_timer;
 #endif

+#ifdef CONFIG_BRIDGE_PAE_FORWARD	
+	enum {
+		BR_PAE_DEFAULT,		/* 802.1x frames consumed by bridge */
+		BR_PAE_FORWARD,		/* 802.1x frames forwarded by bridge */
+	} pae_forward;
+#endif
+
 	struct timer_list		hello_timer;
 	struct timer_list		tcn_timer;
 	struct timer_list		topology_change_timer;

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 5c1e555..c5ffd97 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c

@@ -679,6 +679,33 @@ static DEVICE_ATTR(nf_call_arptables, S_IRUGO | S_IWUSR,
 		   show_nf_call_arptables, store_nf_call_arptables);
 #endif

+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+static ssize_t show_pae_forward(struct device *d, struct

device_attribute *attr,
+				char *buf)
+{
+	struct net_bridge *br = to_bridge(d);
+	return sprintf(buf, "%d\n", br->pae_forward);
+}
+
+static int set_pae_forward(struct net_bridge *br, unsigned long val)
+{
+	if (val > BR_PAE_FORWARD)
+		return -EINVAL;
+
+	br->pae_forward = val;
+	return 0;
+}
+
+static ssize_t store_pae_forward(struct device *d,
+				 struct device_attribute *attr, const char *buf,
+				 size_t len)
+{
+	return store_bridge_parm(d, buf, len, set_pae_forward);
+}
+static DEVICE_ATTR(pae_forward, S_IRUGO | S_IWUSR, show_pae_forward,
+		   store_pae_forward);
+#endif
+
 static struct attribute *bridge_attrs[] = {
 	&dev_attr_forward_delay.attr,
 	&dev_attr_hello_time.attr,

@@ -717,6 +744,9 @@ static struct attribute *bridge_attrs[] = {
 	&dev_attr_nf_call_ip6tables.attr,
 	&dev_attr_nf_call_arptables.attr,
 #endif
+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+	&dev_attr_pae_forward.attr,
+#endif
 	NULL
 };

On Thu, 23 Jun 2011 22:39:52 +0100
Nick Carter [off-list ref] wrote:

Don't make it a config option, users and distros won't get it right.
The bridge already makes special case for multicast, why not add
some smarts and always do it.

New diffs below with the Kconfig option removed as requested.

Now all users and distro's will get the correct 802.1x bridge
behaviour by default.  That is EAPOL frames attempting to traverse the
bridge will be dropped (IEEE Std 802.1X-2001 C.3.3).

Users or distro's who want the non-standard behaviour of forwarding
EAPOL frames, can use a simple runtime configuration change to the
sysfs bridge/pae_forward attribute.

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..91c1b71 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -214,6 +214,7 @@ static struct net_device *new_bridge_dev(struct

net *net, const char *name)
 	br->topology_change = 0;
 	br->topology_change_detected = 0;
 	br->ageing_time = 300 * HZ;
+	br->pae_forward = BR_PAE_DEFAULT;

 	br_netfilter_rtable_init(br);

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 90e985b..edeb92d 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -43,6 +43,16 @@ static int br_pass_frame_up(struct sk_buff *skb)
 		       netif_receive_skb);
 }

+static inline bool br_pae_forward(struct net_bridge *br, __be16 proto)
+{
+	return br->pae_forward == BR_PAE_FORWARD && proto == htons(ETH_P_PAE);
+}
+
+static inline bool br_pae_drop(struct net_bridge *br, __be16 proto)
+{
+	return br->pae_forward == BR_PAE_DEFAULT && proto == htons(ETH_P_PAE);
+}
+
 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {

@@ -98,6 +108,10 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	}

 	if (skb) {
+		/* Prevent Crosstalk (IEEE Std 802.1X-2001 C.3.3) */
+		if (unlikely(br_pae_drop(br, skb->protocol)))
+			goto drop;
+
 		if (dst)
 			br_forward(dst->dst, skb, skb2);
 		else

@@ -166,6 +180,10 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)
 		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
 			goto forward;

+		/* Check if PAE frame should be forwarded */
+		if (br_pae_forward(p->br, skb->protocol))
+			goto forward;
+
 		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
 			    NULL, br_handle_local_finish))
 			return NULL;	/* frame consumed by filter */

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..683c057 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -244,6 +244,11 @@ struct net_bridge
 	struct timer_list		multicast_query_timer;
 #endif

+	enum {
+		BR_PAE_DEFAULT,		/* 802.1x frames consumed by bridge */
+		BR_PAE_FORWARD,		/* 802.1x frames forwarded by bridge */
+	} pae_forward;
+
 	struct timer_list		hello_timer;
 	struct timer_list		tcn_timer;
 	struct timer_list		topology_change_timer;

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 5c1e555..9bdbc84 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c

@@ -679,6 +679,31 @@ static DEVICE_ATTR(nf_call_arptables, S_IRUGO | S_IWUSR,
 		   show_nf_call_arptables, store_nf_call_arptables);
 #endif

+static ssize_t show_pae_forward(struct device *d, struct

device_attribute *attr,
+				char *buf)
+{
+	struct net_bridge *br = to_bridge(d);
+	return sprintf(buf, "%d\n", br->pae_forward);
+}
+
+static int set_pae_forward(struct net_bridge *br, unsigned long val)
+{
+	if (val > BR_PAE_FORWARD)
+		return -EINVAL;
+
+	br->pae_forward = val;
+	return 0;
+}
+
+static ssize_t store_pae_forward(struct device *d,
+				 struct device_attribute *attr, const char *buf,
+				 size_t len)
+{
+	return store_bridge_parm(d, buf, len, set_pae_forward);
+}
+static DEVICE_ATTR(pae_forward, S_IRUGO | S_IWUSR, show_pae_forward,
+		   store_pae_forward);
+
 static struct attribute *bridge_attrs[] = {
 	&dev_attr_forward_delay.attr,
 	&dev_attr_hello_time.attr,

@@ -698,6 +723,7 @@ static struct attribute *bridge_attrs[] = {
 	&dev_attr_gc_timer.attr,
 	&dev_attr_group_addr.attr,
 	&dev_attr_flush.attr,
+	&dev_attr_pae_forward.attr,
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	&dev_attr_multicast_router.attr,
 	&dev_attr_multicast_snooping.attr,

On 23 June 2011 23:29, Stephen Hemminger
[off-list ref] wrote:

On Thu, 23 Jun 2011 22:39:52 +0100
Nick Carter [off-list ref] wrote:

quoted

Signed-off-by: Nick Carter <redacted>

This Kconfig option is used to enable a bridge to forward 802.1x
(EAPOL) Port Access Entity (PAE) frames.  One use of this would be to
enable 802.1x authentication between a PAE supplicant running inside a
virtual machine, with the EAPOL frames bridged out to an external PAE
authenticator.

If BRIDGE_PAE_FORWARD is not set the behaviour of bridge.ko is unchanged.

If BRIDGE_PAE_FORWARD is set then by default the only new behaviour is
that unicast EAPOL frames attempting to traverse the bridge will be
dropped.  This makes the bridge standards compliant by preventing
crosstalk (IEEE Std 802.1X-2001 C.3.3).

Writing a 1 to the new sysfs attribute ../bridge/pae_forward will
enable the forwarding of EAPOL frames, both unicast and link local
multicast (01-80-C2-00-00-03).

diff --git a/net/bridge/Kconfig b/net/bridge/Kconfig
index 6dee7bf..c47a49e 100644
--- a/net/bridge/Kconfig
+++ b/net/bridge/Kconfig

@@ -46,3 +46,22 @@ config BRIDGE_IGMP_SNOOPING

        Say N to exclude this support and reduce the binary size.

        If unsure, say Y.
+
+config BRIDGE_PAE_FORWARD
+     bool "PAE Forwarding"
+     depends on BRIDGE
+     default n
+     ---help---
+       If you say Y here, then the Ethernet bridge will be able to forward
+       802.1x (EAPOL) Port Access Entity (PAE) frames.  One use of this would
+       be to enable 802.1x authentication between a PAE supplicant running
+       inside a virtual machine, with the EAPOL frames bridged out to an
+       external PAE authenticator.
+
+       On a running kernel with this support, enable PAE forwarding by
+       writing a '1' to the bridge devices pae_forward attribute.
+       e.g. echo 1 > /sys/devices/virtual/net/br73/bridge/pae_forward
+
+       Say N to exclude this support.
+
+       If unsure, say N.

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..b493474 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -214,6 +214,9 @@ static struct net_device *new_bridge_dev(struct

net *net, const char *name)
      br->topology_change = 0;
      br->topology_change_detected = 0;
      br->ageing_time = 300 * HZ;
+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+     br->pae_forward = BR_PAE_DEFAULT;
+#endif

      br_netfilter_rtable_init(br);

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 90e985b..183c40f 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -43,6 +43,24 @@ static int br_pass_frame_up(struct sk_buff *skb)

                     netif_receive_skb);
 }

+static inline bool br_pae_forward(struct net_bridge *br, __be16 proto)
+{
+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+     return br->pae_forward == BR_PAE_FORWARD && proto == htons(ETH_P_PAE);
+#else
+     return false;
+#endif
+}
+
+static inline bool br_pae_drop(struct net_bridge *br, __be16 proto)
+{
+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+     return br->pae_forward == BR_PAE_DEFAULT && proto == htons(ETH_P_PAE);
+#else
+     return false;
+#endif
+}
+
 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {

@@ -98,6 +116,10 @@ int br_handle_frame_finish(struct sk_buff *skb)

      }

      if (skb) {
+             /* Prevent Crosstalk (IEEE Std 802.1X-2001 C.3.3) */
+             if (unlikely(br_pae_drop(br, skb->protocol)))
+                     goto drop;
+
              if (dst)
                      br_forward(dst->dst, skb, skb2);
              else

@@ -166,6 +188,10 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)

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

+             /* Check if PAE frame should be forwarded */
+             if (br_pae_forward(p->br, skb->protocol))
+                     goto forward;
+
              if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
                          NULL, br_handle_local_finish))
                      return NULL;    /* frame consumed by filter */

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..a523032 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -244,6 +244,13 @@ struct net_bridge

      struct timer_list               multicast_query_timer;
 #endif

+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+     enum {
+             BR_PAE_DEFAULT,         /* 802.1x frames consumed by bridge */
+             BR_PAE_FORWARD,         /* 802.1x frames forwarded by bridge */
+     } pae_forward;
+#endif
+
      struct timer_list               hello_timer;
      struct timer_list               tcn_timer;
      struct timer_list               topology_change_timer;

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 5c1e555..c5ffd97 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c

@@ -679,6 +679,33 @@ static DEVICE_ATTR(nf_call_arptables, S_IRUGO | S_IWUSR,

                 show_nf_call_arptables, store_nf_call_arptables);
 #endif

+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+static ssize_t show_pae_forward(struct device *d, struct
device_attribute *attr,
+                             char *buf)
+{
+     struct net_bridge *br = to_bridge(d);
+     return sprintf(buf, "%d\n", br->pae_forward);
+}
+
+static int set_pae_forward(struct net_bridge *br, unsigned long val)
+{
+     if (val > BR_PAE_FORWARD)
+             return -EINVAL;
+
+     br->pae_forward = val;
+     return 0;
+}
+
+static ssize_t store_pae_forward(struct device *d,
+                              struct device_attribute *attr, const char *buf,
+                              size_t len)
+{
+     return store_bridge_parm(d, buf, len, set_pae_forward);
+}
+static DEVICE_ATTR(pae_forward, S_IRUGO | S_IWUSR, show_pae_forward,
+                store_pae_forward);
+#endif
+
 static struct attribute *bridge_attrs[] = {
      &dev_attr_forward_delay.attr,
      &dev_attr_hello_time.attr,

@@ -717,6 +744,9 @@ static struct attribute *bridge_attrs[] = {

      &dev_attr_nf_call_ip6tables.attr,
      &dev_attr_nf_call_arptables.attr,
 #endif
+#ifdef CONFIG_BRIDGE_PAE_FORWARD
+     &dev_attr_pae_forward.attr,
+#endif
      NULL
 };

Don't make it a config option, users and distros won't get it right.
The bridge already makes special case for multicast, why not add
some smarts and always do it.

On Fri, 24 Jun 2011 19:29:41 +0100
Nick Carter [off-list ref] wrote:

New diffs below with the Kconfig option removed as requested.

Now all users and distro's will get the correct 802.1x bridge
behaviour by default.  That is EAPOL frames attempting to traverse the
bridge will be dropped (IEEE Std 802.1X-2001 C.3.3).

Users or distro's who want the non-standard behaviour of forwarding
EAPOL frames, can use a simple runtime configuration change to the
sysfs bridge/pae_forward attribute.

This is much better, thanks.
See the comments for how to make the code more compact and tighter.

It is just a boolean, why the verbose enum values?
 

Since only used one place, the extra wrappers aren't helping.

Referencing standard is good, but perhaps explaining what that means.
Since these are multicast frames, will it ever reach this point.
This point is reached for unicast frames that are not local.
And won't this change existing behavior since before this 802.1x unicast
frames would be forwarded.

On 24 June 2011 20:08, Stephen Hemminger
[off-list ref] wrote:

On Fri, 24 Jun 2011 19:29:41 +0100
Nick Carter [off-list ref] wrote:

quoted

New diffs below with the Kconfig option removed as requested.

Now all users and distro's will get the correct 802.1x bridge
behaviour by default.  That is EAPOL frames attempting to traverse the
bridge will be dropped (IEEE Std 802.1X-2001 C.3.3).

Users or distro's who want the non-standard behaviour of forwarding
EAPOL frames, can use a simple runtime configuration change to the
sysfs bridge/pae_forward attribute.

This is much better, thanks.
See the comments for how to make the code more compact and tighter.

quoted

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..91c1b71 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -214,6 +214,7 @@ static struct net_device *new_bridge_dev(struct

net *net, const char *name)
      br->topology_change = 0;
      br->topology_change_detected = 0;
      br->ageing_time = 300 * HZ;
+     br->pae_forward = BR_PAE_DEFAULT;

It is just a boolean, why the verbose enum values?

In case we want BR_PAE_<foo> in the future, not that I can think of a
3rd option now.  So happy to change to a boolean.

quoted

      br_netfilter_rtable_init(br);

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 90e985b..edeb92d 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -43,6 +43,16 @@ static int br_pass_frame_up(struct sk_buff *skb)

                     netif_receive_skb);
 }

+static inline bool br_pae_forward(struct net_bridge *br, __be16 proto)
+{
+     return br->pae_forward == BR_PAE_FORWARD && proto == htons(ETH_P_PAE);
+}
+
+static inline bool br_pae_drop(struct net_bridge *br, __be16 proto)
+{
+     return br->pae_forward == BR_PAE_DEFAULT && proto == htons(ETH_P_PAE);
+}

Since only used one place, the extra wrappers aren't helping.

I thought they helped readability, but certainly for performance we
should only be doing each check once in a single place.  Again happy
to change.

quoted

 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {

@@ -98,6 +108,10 @@ int br_handle_frame_finish(struct sk_buff *skb)

      }

      if (skb) {
+             /* Prevent Crosstalk (IEEE Std 802.1X-2001 C.3.3) */
+             if (unlikely(br_pae_drop(br, skb->protocol)))
+                     goto drop;
+

Referencing standard is good, but perhaps explaining what that means.

ok

Since these are multicast frames, will it ever reach this point.
This point is reached for unicast frames that are not local.

yes, think of it as a bug fix rather than part of new functionality

And won't this change existing behavior since before this 802.1x unicast
frames would be forwarded.

Yes, that was my original motivation for making it a Kconfig setting,
so there would be no chance of regressions.  But keep in mind that
802.1x handshake must start with a multicast.  Its only if that
multicast is delivered that the reply can be unicast.  So any one
relying on the existing behaviour of forwarding unicast 802.1x must be
doing something very strange and non-standard.  I can't imagine what.
If there is a valid use case then they now have the simple workaround
of enabling pae forwarding.

quoted

              if (dst)
                      br_forward(dst->dst, skb, skb2);
              else

@@ -166,6 +180,10 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)

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

+             /* Check if PAE frame should be forwarded */
+             if (br_pae_forward(p->br, skb->protocol))
+                     goto forward;
+
              if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
                          NULL, br_handle_local_finish))
                      return NULL;    /* frame consumed by filter */

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..683c057 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -244,6 +244,11 @@ struct net_bridge

      struct timer_list               multicast_query_timer;
 #endif

+     enum {
+             BR_PAE_DEFAULT,         /* 802.1x frames consumed by bridge */
+             BR_PAE_FORWARD,         /* 802.1x frames forwarded by bridge */
+     } pae_forward;
+
      struct timer_list               hello_timer;
      struct timer_list               tcn_timer;
      struct timer_list               topology_change_timer;

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 5c1e555..9bdbc84 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c

@@ -679,6 +679,31 @@ static DEVICE_ATTR(nf_call_arptables, S_IRUGO | S_IWUSR,

                 show_nf_call_arptables, store_nf_call_arptables);
 #endif

+static ssize_t show_pae_forward(struct device *d, struct
device_attribute *attr,
+                             char *buf)
+{
+     struct net_bridge *br = to_bridge(d);
+     return sprintf(buf, "%d\n", br->pae_forward);
+}
+
+static int set_pae_forward(struct net_bridge *br, unsigned long val)
+{
+     if (val > BR_PAE_FORWARD)
+             return -EINVAL;
+
+     br->pae_forward = val;
+     return 0;
+}
+
+static ssize_t store_pae_forward(struct device *d,
+                              struct device_attribute *attr, const char *buf,
+                              size_t len)
+{
+     return store_bridge_parm(d, buf, len, set_pae_forward);
+}
+static DEVICE_ATTR(pae_forward, S_IRUGO | S_IWUSR, show_pae_forward,
+                store_pae_forward);
+
 static struct attribute *bridge_attrs[] = {
      &dev_attr_forward_delay.attr,
      &dev_attr_hello_time.attr,

@@ -698,6 +723,7 @@ static struct attribute *bridge_attrs[] = {

      &dev_attr_gc_timer.attr,
      &dev_attr_group_addr.attr,
      &dev_attr_flush.attr,
+     &dev_attr_pae_forward.attr,
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
      &dev_attr_multicast_router.attr,
      &dev_attr_multicast_snooping.attr,

Updated diffs addressing Stephens comments

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..a401ed4 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -214,6 +214,7 @@ static struct net_device *new_bridge_dev(struct

net *net, const char *name)
 	br->topology_change = 0;
 	br->topology_change_detected = 0;
 	br->ageing_time = 300 * HZ;
+	br->pae_forward = false;

 	br_netfilter_rtable_init(br);

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 90e985b..79b03fa 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -98,6 +98,14 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	}

 	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)))
+			goto drop;
+
 		if (dst)
 			br_forward(dst->dst, skb, skb2);
 		else

@@ -166,6 +174,10 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)
 		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
 			goto forward;

+		/* Check if PAE frame should be forwarded */
+		if (p->br->pae_forward && skb->protocol == htons(ETH_P_PAE))
+			goto forward;
+
 		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
 			    NULL, br_handle_local_finish))
 			return NULL;	/* frame consumed by filter */

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..8977d66 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -244,6 +244,8 @@ struct net_bridge
 	struct timer_list		multicast_query_timer;
 #endif

+	bool pae_forward;		/* 802.1x frames forwarded / dropped */
+
 	struct timer_list		hello_timer;
 	struct timer_list		tcn_timer;
 	struct timer_list		topology_change_timer;

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 5c1e555..de3550f 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c

@@ -679,6 +679,28 @@ static DEVICE_ATTR(nf_call_arptables, S_IRUGO | S_IWUSR,
 		   show_nf_call_arptables, store_nf_call_arptables);
 #endif

+static ssize_t show_pae_forward(struct device *d, struct

device_attribute *attr,
+				char *buf)
+{
+	struct net_bridge *br = to_bridge(d);
+	return sprintf(buf, "%d\n", br->pae_forward);
+}
+
+static int set_pae_forward(struct net_bridge *br, unsigned long val)
+{
+	br->pae_forward = val ? true : false;
+	return 0;
+}
+
+static ssize_t store_pae_forward(struct device *d,
+				 struct device_attribute *attr, const char *buf,
+				 size_t len)
+{
+	return store_bridge_parm(d, buf, len, set_pae_forward);
+}
+static DEVICE_ATTR(pae_forward, S_IRUGO | S_IWUSR, show_pae_forward,
+		   store_pae_forward);
+
 static struct attribute *bridge_attrs[] = {
 	&dev_attr_forward_delay.attr,
 	&dev_attr_hello_time.attr,

@@ -698,6 +720,7 @@ static struct attribute *bridge_attrs[] = {
 	&dev_attr_gc_timer.attr,
 	&dev_attr_group_addr.attr,
 	&dev_attr_flush.attr,
+	&dev_attr_pae_forward.attr,
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	&dev_attr_multicast_router.attr,
 	&dev_attr_multicast_snooping.attr,

On 24 June 2011 22:29, Nick Carter [off-list ref] wrote:

On 24 June 2011 20:08, Stephen Hemminger
[off-list ref] wrote:

quoted

On Fri, 24 Jun 2011 19:29:41 +0100
Nick Carter [off-list ref] wrote:

quoted

New diffs below with the Kconfig option removed as requested.

Now all users and distro's will get the correct 802.1x bridge
behaviour by default.  That is EAPOL frames attempting to traverse the
bridge will be dropped (IEEE Std 802.1X-2001 C.3.3).

Users or distro's who want the non-standard behaviour of forwarding
EAPOL frames, can use a simple runtime configuration change to the
sysfs bridge/pae_forward attribute.

This is much better, thanks.
See the comments for how to make the code more compact and tighter.

quoted

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..91c1b71 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -214,6 +214,7 @@ static struct net_device *new_bridge_dev(struct

net *net, const char *name)
      br->topology_change = 0;
      br->topology_change_detected = 0;
      br->ageing_time = 300 * HZ;
+     br->pae_forward = BR_PAE_DEFAULT;

It is just a boolean, why the verbose enum values?

In case we want BR_PAE_<foo> in the future, not that I can think of a
3rd option now.  So happy to change to a boolean.

quoted

quoted

      br_netfilter_rtable_init(br);

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 90e985b..edeb92d 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -43,6 +43,16 @@ static int br_pass_frame_up(struct sk_buff *skb)

                     netif_receive_skb);
 }

+static inline bool br_pae_forward(struct net_bridge *br, __be16 proto)
+{
+     return br->pae_forward == BR_PAE_FORWARD && proto == htons(ETH_P_PAE);
+}
+
+static inline bool br_pae_drop(struct net_bridge *br, __be16 proto)
+{
+     return br->pae_forward == BR_PAE_DEFAULT && proto == htons(ETH_P_PAE);
+}

Since only used one place, the extra wrappers aren't helping.

I thought they helped readability, but certainly for performance we
should only be doing each check once in a single place.  Again happy
to change.

quoted

quoted

 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct sk_buff *skb)
 {

@@ -98,6 +108,10 @@ int br_handle_frame_finish(struct sk_buff *skb)

      }

      if (skb) {
+             /* Prevent Crosstalk (IEEE Std 802.1X-2001 C.3.3) */
+             if (unlikely(br_pae_drop(br, skb->protocol)))
+                     goto drop;
+

Referencing standard is good, but perhaps explaining what that means.

ok

quoted

Since these are multicast frames, will it ever reach this point.
This point is reached for unicast frames that are not local.

yes, think of it as a bug fix rather than part of new functionality

quoted

And won't this change existing behavior since before this 802.1x unicast
frames would be forwarded.

Yes, that was my original motivation for making it a Kconfig setting,
so there would be no chance of regressions.  But keep in mind that
802.1x handshake must start with a multicast.  Its only if that
multicast is delivered that the reply can be unicast.  So any one
relying on the existing behaviour of forwarding unicast 802.1x must be
doing something very strange and non-standard.  I can't imagine what.
If there is a valid use case then they now have the simple workaround
of enabling pae forwarding.

quoted

quoted

              if (dst)
                      br_forward(dst->dst, skb, skb2);
              else

@@ -166,6 +180,10 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)

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

+             /* Check if PAE frame should be forwarded */
+             if (br_pae_forward(p->br, skb->protocol))
+                     goto forward;
+
              if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
                          NULL, br_handle_local_finish))
                      return NULL;    /* frame consumed by filter */

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..683c057 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h

@@ -244,6 +244,11 @@ struct net_bridge

      struct timer_list               multicast_query_timer;
 #endif

+     enum {
+             BR_PAE_DEFAULT,         /* 802.1x frames consumed by bridge */
+             BR_PAE_FORWARD,         /* 802.1x frames forwarded by bridge */
+     } pae_forward;
+
      struct timer_list               hello_timer;
      struct timer_list               tcn_timer;
      struct timer_list               topology_change_timer;

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 5c1e555..9bdbc84 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c

@@ -679,6 +679,31 @@ static DEVICE_ATTR(nf_call_arptables, S_IRUGO | S_IWUSR,

                 show_nf_call_arptables, store_nf_call_arptables);
 #endif

+static ssize_t show_pae_forward(struct device *d, struct
device_attribute *attr,
+                             char *buf)
+{
+     struct net_bridge *br = to_bridge(d);
+     return sprintf(buf, "%d\n", br->pae_forward);
+}
+
+static int set_pae_forward(struct net_bridge *br, unsigned long val)
+{
+     if (val > BR_PAE_FORWARD)
+             return -EINVAL;
+
+     br->pae_forward = val;
+     return 0;
+}
+
+static ssize_t store_pae_forward(struct device *d,
+                              struct device_attribute *attr, const char *buf,
+                              size_t len)
+{
+     return store_bridge_parm(d, buf, len, set_pae_forward);
+}
+static DEVICE_ATTR(pae_forward, S_IRUGO | S_IWUSR, show_pae_forward,
+                store_pae_forward);
+
 static struct attribute *bridge_attrs[] = {
      &dev_attr_forward_delay.attr,
      &dev_attr_hello_time.attr,

@@ -698,6 +723,7 @@ static struct attribute *bridge_attrs[] = {

      &dev_attr_gc_timer.attr,
      &dev_attr_group_addr.attr,
      &dev_attr_flush.attr,
+     &dev_attr_pae_forward.attr,
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
      &dev_attr_multicast_router.attr,
      &dev_attr_multicast_snooping.attr,

On Sat, Jun 25, 2011 at 12:33:05AM +0100, Nick Carter wrote:

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.)


-David

On Tue, 28 Jun 2011 17:02:57 +0200
David Lamparter [off-list ref] wrote:

On Sat, Jun 25, 2011 at 12:33:05AM +0100, Nick Carter wrote:

quoted

@@ -98,6 +98,14 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	}

 	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)))
+			goto drop;
+
 		if (dst)
 			br_forward(dst->dst, skb, skb2);
 		else

@@ -166,6 +174,10 @@ struct sk_buff *br_handle_frame(struct sk_buff *skb)
 		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
 			goto forward;

+		/* Check if PAE frame should be forwarded */
+		if (p->br->pae_forward && skb->protocol == htons(ETH_P_PAE))
+			goto forward;
+
 		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
 			    NULL, br_handle_local_finish))
 			return NULL;	/* frame consumed by filter */

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.

On Tue, Jun 28, 2011 at 08:10:15AM -0700, Stephen Hemminger wrote:

On Tue, 28 Jun 2011 17:02:57 +0200
David Lamparter [off-list ref] wrote:

quoted

quoted

 	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

On 28 June 2011 17:00, David Lamparter [off-list ref] wrote:

On Tue, Jun 28, 2011 at 08:10:15AM -0700, Stephen Hemminger wrote:

quoted

On Tue, 28 Jun 2011 17:02:57 +0200
David Lamparter [off-list ref] wrote:

quoted

quoted

  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)

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

On Tue, Jun 28, 2011 at 07:34:57PM +0100, Nick Carter wrote:

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

On 28 June 2011 19:58, David Lamparter [off-list ref] wrote:

On Tue, Jun 28, 2011 at 07:34:57PM +0100, Nick Carter wrote:

quoted

quoted

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

On Tue, Jun 28, 2011 at 09:00:16PM +0100, Nick Carter wrote:

quoted

quoted

                /* 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"

quoted

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

On 28 June 2011 21:22, David Lamparter [off-list ref] wrote:

On Tue, Jun 28, 2011 at 09:00:16PM +0100, Nick Carter wrote:

quoted

quoted

quoted

                /* 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"

quoted

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

On Tue, Jun 28, 2011 at 09:54:01PM +0100, Nick Carter wrote:

quoted

'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.

quoted

(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

On 28 June 2011 22:04, David Lamparter [off-list ref] wrote:

On Tue, Jun 28, 2011 at 09:54:01PM +0100, Nick Carter wrote:

quoted

quoted

'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...

quoted

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.

quoted

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.

quoted

quoted

(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

On Tue, Jun 28, 2011 at 10:22:53PM +0100, Nick Carter wrote:

quoted

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.

[...]

quoted

quoted

quoted

(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

this patch adds bonding frames to the special treatment party and has
both pause and bonding frames delivered on the underlying bridge member
device. we thus become 802.1AX section 5.2.1 compliant which quite
clearly has the link aggregation directly on top of the MAC layer, below
any bridging.

the matching is changed to mimic hardware switches, which match
802.3x/pause and 802.3ad/lacp by the hardware address (keep in mind the
existence of LLC/SNAP headers).

Signed-off-by: David Lamparter <redacted>
---
note: this should actually fix some issues i was having with bridging
screwing up my bonding. i've resolved those by "brouting" LACP frames
in ebtables... (which this patch will also result in)

compile-tested only

 net/bridge/br_input.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index f3ac1e8..c873db5 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -160,9 +160,12 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 	p = br_port_get_rcu(skb->dev);
 
 	if (unlikely(is_link_local(dest))) {
-		/* Pause frames shouldn't be passed up by driver anyway */
-		if (skb->protocol == htons(ETH_P_PAUSE))
-			goto drop;
+		/* Pause/3x frames shouldn't be passed up by driver anyway
+		 * LACP/3ad can never be allowed to cross even a dumb hub
+		 *
+		 * both are usually matched by group address */
+		if (dest[5] == 0x01 || dest[5] == 0x02)
+			return RX_HANDLER_PASS;
 
 		/* If STP is turned off, then forward */
 		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)

-- 
1.7.5.3

when operating without STP, we're a dumb switch and should be able to
forward ethernet management protocols like 802.1X, LLDP and GVRP.

if this is not desired, it can be enacted as local policy through
ebtables.

if we're in STP mode we basically claim to be an intelligent switch and
should implement these protocols properly (in userspace).

Signed-off-by: David Lamparter <redacted>
---
compile-tested only

 net/bridge/br_input.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index c873db5..4cee1b5 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -167,16 +167,19 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 		if (dest[5] == 0x01 || dest[5] == 0x02)
 			return RX_HANDLER_PASS;
 
-		/* If STP is turned off, then forward */
-		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)
+		/* If STP is turned off, we're a dumb switch and therefore
+		 * forward the remaining link-locals. (STP, 802.1X, LLDP,
+		 * GVRP & co.) */
+		if (p->br->stp_enabled == BR_NO_STP)
 			goto forward;
 
 		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
 			    NULL, br_handle_local_finish)) {
 			return RX_HANDLER_CONSUMED; /* consumed by filter */
 		} else {
+			/* stay on physdev for userspace implementation */
 			*pskb = skb;
-			return RX_HANDLER_PASS;	/* continue processing */
+			return RX_HANDLER_PASS;
 		}
 	}
 

-- 
1.7.5.3

On 28 June 2011 23:03, David Lamparter [off-list ref] wrote:

when operating without STP, we're a dumb switch and should be able to
forward ethernet management protocols like 802.1X, LLDP and GVRP.

I don't like the idea of tying STP on / off with the forwarding of
these other protocols.  These other protocols are not dependent on
STP.  These diffs change the default behaviour so that if someone
writes an 802.1X authenticator in userspace then all deployments will
have to turn STP on to be able to use it !!

If I was a sysadmin and I configured 'bridge_stp off' in say
/etc/interfaces, i would be very surprised and alarmed to find I had
turned *on* forwarding a load of protocols.

Also many of these addresses are reserved for future use.  Do we
really want to forward them before we know what they will be used for
?
Nick

this patch adds bonding frames to the special treatment party and has
both pause and bonding frames delivered on the underlying bridge member
device. we thus become 802.1AX section 5.2.1 compliant which quite
clearly has the link aggregation directly on top of the MAC layer, below
any bridging.

the matching is changed to mimic hardware switches, which match
802.3x/pause and 802.3ad/lacp by the hardware address (keep in mind the
existence of LLC/SNAP headers).

Signed-off-by: David Lamparter <redacted>
---
[v2:] ... and obviously i forget the "*pskb = skb;" - it's getting late.

note: this should actually fix some issues i was having with bridging
screwing up my bonding. i've resolved those by "brouting" LACP frames
in ebtables... (which this patch will also result in)

compile-tested only

 net/bridge/br_input.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index f3ac1e8..3ef2861 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -160,9 +160,14 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
 	p = br_port_get_rcu(skb->dev);
 
 	if (unlikely(is_link_local(dest))) {
-		/* Pause frames shouldn't be passed up by driver anyway */
-		if (skb->protocol == htons(ETH_P_PAUSE))
-			goto drop;
+		/* Pause/3x frames shouldn't be passed up by driver anyway
+		 * LACP/3ad can never be allowed to cross even a dumb hub
+		 *
+		 * both are usually matched by group address */
+		if (dest[5] == 0x01 || dest[5] == 0x02) {
+			*pskb = skb;
+			return RX_HANDLER_PASS;
+		}
 
 		/* If STP is turned off, then forward */
 		if (p->br->stp_enabled == BR_NO_STP && dest[5] == 0)

-- 
1.7.5.3

On 28 June 2011 22:46, David Lamparter [off-list ref] wrote:

On Tue, Jun 28, 2011 at 10:22:53PM +0100, Nick Carter wrote:

quoted

quoted

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.

[...]

quoted

quoted

quoted

quoted

(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)

quoted

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

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.

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

2011/7/1 David Lamparter [off-list ref]:

On Wed, Jun 29, 2011 at 04:34:23PM -0700, Stephen Hemminger wrote:

quoted

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

On Fri, Jul 01, 2011 at 04:58:56PM +0200, Michał Mirosław wrote:
[...]

quoted

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

W dniu 1 lipca 2011 17:16 użytkownik David Lamparter
[off-list ref] napisał:

On Fri, Jul 01, 2011 at 04:58:56PM +0200, Michał Mirosław wrote:
[...]

quoted

quoted

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

2011/7/1 Michał Mirosław [off-list ref]:

W dniu 1 lipca 2011 17:16 użytkownik David Lamparter
[off-list ref] napisał:

quoted

On Fri, Jul 01, 2011 at 04:58:56PM +0200, Michał Mirosław wrote:
[...]

quoted

quoted

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