[PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

Signed-off-by: Nick Carter <ncarter100@gmail.com>

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
 };

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Stephen Hemminger]

On Thu, 23 Jun 2011 22:39:52 +0100
Nick Carter <ncarter100@gmail.com> wrote:

> Signed-off-by: Nick Carter <ncarter100@gmail.com>
> 
> 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.

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

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
<shemminger@linux-foundation.org> wrote:
> On Thu, 23 Jun 2011 22:39:52 +0100
> Nick Carter <ncarter100@gmail.com> wrote:
>
>> Signed-off-by: Nick Carter <ncarter100@gmail.com>
>>
>> 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.
>

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Stephen Hemminger]

On Fri, 24 Jun 2011 19:29:41 +0100
Nick Carter <ncarter100@gmail.com> 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.

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

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

>  		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,

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

On 24 June 2011 20:08, Stephen Hemminger
<shemminger@linux-foundation.org> wrote:
> On Fri, 24 Jun 2011 19:29:41 +0100
> Nick Carter <ncarter100@gmail.com> 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.
>
>> 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.
>
>>       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.
>
>>  /* 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.

>>               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,
>
>

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

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 <ncarter100@gmail.com> wrote:
> On 24 June 2011 20:08, Stephen Hemminger
> <shemminger@linux-foundation.org> wrote:
>> On Fri, 24 Jun 2011 19:29:41 +0100
>> Nick Carter <ncarter100@gmail.com> 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.
>>
>>> 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.
>>
>>>       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.
>>
>>>  /* 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.
>
>>>               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,
>>
>>
>

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[David Lamparter]

On Sat, Jun 25, 2011 at 12:33:05AM +0100, Nick Carter wrote:
> @@ -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.)


-David

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Stephen Hemminger]

On Tue, 28 Jun 2011 17:02:57 +0200
David Lamparter <equinox@diac24.net> wrote:

> On Sat, Jun 25, 2011 at 12:33:05AM +0100, Nick Carter wrote:
> > @@ -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.

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[David Lamparter]

On Tue, Jun 28, 2011 at 08:10:15AM -0700, Stephen Hemminger wrote:
> On Tue, 28 Jun 2011 17:02:57 +0200
> David Lamparter <equinox@diac24.net> wrote:
> > >  	if (skb) {
> > > +		/* Prevent Crosstalk where a Supplicant on one Port attempts to
> > > +		 * interfere with authentications occurring on another Port.
> > > +		 * (IEEE Std 802.1X-2001 C.3.3)
> > > +		 */
> > > +		if (unlikely(!br->pae_forward &&
> > > +		    skb->protocol == htons(ETH_P_PAE)))
> > 
> > No, please don't.
> > 
> > Linux bridging has two "grand" modes: dumb and STP enabled.
> > 
> > If we're running a dumb bridge, we behave like an ethernet hub without
> > any intelligence, and in that case we should absolutely forward 802.1X
> > frames. We may have (e.g. VM) client(s) that want to authenticate with a
> > physical switch.
> > (For the spec, this counts as "repeater", not "bridge"/"switch")
> > 
> > If we're running with STP enabled, then 802.1X traffic should already be
> > caught by the general ethernet link-local multicast drop (which applies
> > to 01:80:c2:/24 and therefore catches 802.1X too.)
> 
> The problem is that STP is not enabled by default, and most people don't
> know how to enable it.

Yes, the default is a dumb hub (IMHO correctly so). And a dumb hub will
forward 802.1X packets (IMHO also correctly so).

Why should we specifically add a knob for EAPOL? Next we're adding one
for STP itself, then one for LLDP, then one for Cisco's deprecated
crap (CDP, DTP, ...) etc.

If you want a dumb hub that drops EAPOL, use ebtables.

-David

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

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

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

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[David Lamparter]

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

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

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

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


-David

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

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

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

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

Thanks
Nick

>
>
> -David
>
>

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[David Lamparter]

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

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

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

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


-David

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

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

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[David Lamparter]

On Tue, Jun 28, 2011 at 09:54:01PM +0100, Nick Carter wrote:
> > 'reinject' isn't possible when it hits that code path - which is pretty
> > much why I'm saying we should be forwarding everything in the non-STP
> > case.
> I'm not sure I like this turn off STP and suddenly start forwarding
> random groups.  There is no connection between wanting STP on / off
> and forwarding pae on / off.

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

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

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

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

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

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

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

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

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

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

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

-David

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

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

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[David Lamparter]

On Tue, Jun 28, 2011 at 10:22:53PM +0100, Nick Carter wrote:
> > I beg to differ, there very much is. You never ever ever want to be
> > running STP with 802.1X packets passing through... some client will shut
> > down your upstream port, your STP will break and you will die in a fire.
> >
> > The general idea, though, is that a STP-enabled switch is an intelligent
> > switch. And an intelligent switch can speak all those pesky small
> > side-dish protocols.
[...]
> >> > (Some quick googling reveals that hardware switch chips special-drop
> >> > 01:80:c2:00:00:01 [802.3x/pause] and :02 [802.3ad/lacp] and nothing
> >> > else - for the dumb ones anyway. It also seems like the match for pause
> >> > frames usually works on the address, not on the protocol field like we
> >> > do it...)
> >> 'Enterprise' switches drop :03 [802.1x]
> >
> > They also speak STP, see above about never STP+1X :)
> But if you turn off STP they wont start forwarding 802.1x.

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

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

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


-David

[PATCH 1/2] bridge: ignore pause & bonding frames

[David Lamparter]

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 <equinox@diac24.net>
---
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

[PATCH 2/2] bridge: pass through 802.1X & co. in 'dumb' mode

[David Lamparter]

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 <equinox@diac24.net>
---
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

[PATCH v2] bridge: ignore pause & bonding frames

[David Lamparter]

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 <equinox@diac24.net>
---
[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

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Nick Carter]

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

> Instead, you now have to (1) disable the port switching to make
> sure your upstreams stay on and (2) deal with 802.1X clients being
> re"routed" by STP to different authenticators that don't know them.
Well if the authenticators are pointed at a remote ACS then they will
know them.  And again even though non-standard, 802.1X 'mac move'
functionality exists.
Nick
>
>
> -David
>
>

Re: [PATCH 2/2] bridge: pass through 802.1X & co. in 'dumb' mode

[Nick Carter]

On 28 June 2011 23:03, David Lamparter <equinox@diac24.net> 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
>
> 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 <equinox@diac24.net>
> ---
> 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
>
>

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Stephen Hemminger]

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.

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[David Lamparter]

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

Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD

[Michał Mirosław]

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

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

Best Regards,
Michał Mirosław

bridge vs. bonding/pause frames (was: Forward EAPOL...)

[David Lamparter]

On Fri, Jul 01, 2011 at 04:58:56PM +0200, Michał Mirosław wrote:
[...]
> > We _MUST_NOT_ pass bonding frames in any case, but we
> > currently do that if STP is off. (cf. my earlier patch 1/2)
> 
> If you use linux box as a (invisible) L2 network tap, then you want to
> pass everything in the hub mode (including LACP/whatever).

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

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

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


-David

Re: bridge vs. bonding/pause frames (was: Forward EAPOL...)

[Michał Mirosław]

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

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

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

Best Regards,
Michał Mirosław

Re: bridge vs. bonding/pause frames (was: Forward EAPOL...)

[Nick Carter]

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

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

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

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

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

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

>
> Best Regards,
> Michał Mirosław
>