From patchwork Fri Jun 24 23:33:05 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Nick Carter X-Patchwork-Id: 101925 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id A1A2EB6F7F for ; Sat, 25 Jun 2011 09:33:14 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758178Ab1FXXdJ (ORCPT ); Fri, 24 Jun 2011 19:33:09 -0400 Received: from mail-pv0-f174.google.com ([74.125.83.174]:42326 "EHLO mail-pv0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754901Ab1FXXdH convert rfc822-to-8bit (ORCPT ); Fri, 24 Jun 2011 19:33:07 -0400 Received: by pvg12 with SMTP id 12so1975067pvg.19 for ; Fri, 24 Jun 2011 16:33:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=2W6AgLL5sCkt3IgOOWgboFGHV3ErbgIPauYVx4JKMeA=; b=s1OwmQAXuWUNVcUKv6LD6AyH6YlipeNYtq2g4wglAbL67d1ztPSLO3Qjgcm+SylbKR f3FZA9sJYsXerXzw8WjYWhKN951ylySNp4M9yECEEGtu8lwZjDMKAETcEGtNW/QhBO4q DimMER0n/UGTVsMVq4GSrNJUADP3QYBgybke8= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=uil7vqhazIvIYx5fp3sFAm5E9mJKIUSVpOzwpfEogP7+FMR8G2F4atM1k2f97siRyY gT0TRFlqr9o/alZOneuFIyblUe5cWEo1YKD2Jtd0HL14PrYqBDQH9EDYiv8ktVG+dheT UOxTDUVsrNQcus1J1BtakNa96bZc0XDoNzA+k= MIME-Version: 1.0 Received: by 10.68.63.132 with SMTP id g4mr1987953pbs.349.1308958385816; Fri, 24 Jun 2011 16:33:05 -0700 (PDT) Received: by 10.68.47.161 with HTTP; Fri, 24 Jun 2011 16:33:05 -0700 (PDT) In-Reply-To: References: <20110623152929.3f94b3e7@nehalam.ftrdhcpuser.net> <20110624120859.3c43bbcb@nehalam.ftrdhcpuser.net> Date: Sat, 25 Jun 2011 00:33:05 +0100 Message-ID: Subject: Re: [PATCH] bridge: Forward EAPOL Kconfig option BRIDGE_PAE_FORWARD From: Nick Carter To: Stephen Hemminger Cc: netdev@vger.kernel.org, davem@davemloft.net Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Updated diffs addressing Stephens comments &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 wrote: > On 24 June 2011 20:08, Stephen Hemminger > wrote: >> On Fri, 24 Jun 2011 19:29:41 +0100 >> Nick Carter 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_ 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, >> >> > --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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,