| Message ID | 20090814144133.34ac9d94@nehalam |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
> Subject: [RFC] bridge: prevent hairpin and STP problems? > > Do we need to add this to block Spanning Tree from being enabled > with hairpin mode? I am not sure what the exact usage of hairpin > mode and if it is possible to create loops and get STP confusion. > > For comment only, do not apply as is. Your patch disables STP on the whole bridge if one or more ports are set to hairpin mode. However, I don't really see that this is necessary. A hairpin mode port should not reflect BPDUs, because otherwise the connected port would think it has detected a loop. The hairpin mode port should still be able to generate BPDUs though, and in any case the bridge should still be able to run STP. The hairpin patch we submitted reflects packets on the forwarding / data path whereas BPDUs are processed with a separate hook, so we should not be reflecting BPDUs back out of a hairpin mode port. Anna -- 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
On Mon, 17 Aug 2009 21:16:04 +0000 "Fischer, Anna" <anna.fischer@hp.com> wrote: > > Subject: [RFC] bridge: prevent hairpin and STP problems? > > > > Do we need to add this to block Spanning Tree from being enabled > > with hairpin mode? I am not sure what the exact usage of hairpin > > mode and if it is possible to create loops and get STP confusion. > > > > For comment only, do not apply as is. > > Your patch disables STP on the whole bridge if one or more ports > are set to hairpin mode. > > However, I don't really see that this is necessary. > > A hairpin mode port should not reflect BPDUs, because otherwise the > connected port would think it has detected a loop. The hairpin mode > port should still be able to generate BPDUs though, and in any case > the bridge should still be able to run STP. > > The hairpin patch we submitted reflects packets on the forwarding / > data path whereas BPDUs are processed with a separate hook, so we > should not be reflecting BPDUs back out of a hairpin mode port. So if user is using hairpin properly, the STP would work. In fact it would be a good thing since it would detect looping configurations. -- 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
--- a/net/bridge/br_ioctl.c 2009-08-14 14:28:19.917690805 -0700 +++ b/net/bridge/br_ioctl.c 2009-08-14 14:29:54.078271361 -0700 @@ -259,8 +259,7 @@ static int old_dev_ioctl(struct net_devi if (!capable(CAP_NET_ADMIN)) return -EPERM; - br_stp_set_enabled(br, args[1]); - return 0; + return br_stp_set_enabled(br, args[1]); case BRCTL_SET_BRIDGE_PRIORITY: if (!capable(CAP_NET_ADMIN)) --- a/net/bridge/br_stp_if.c 2009-08-14 14:24:30.022315573 -0700 +++ b/net/bridge/br_stp_if.c 2009-08-14 14:35:25.819566113 -0700 @@ -160,17 +160,26 @@ static void br_stp_stop(struct net_bridg br->stp_enabled = BR_NO_STP; } -void br_stp_set_enabled(struct net_bridge *br, unsigned long val) +int br_stp_set_enabled(struct net_bridge *br, unsigned long val) { ASSERT_RTNL(); if (val) { + struct net_bridge_port *p; + list_for_each_entry_rcu(p, &br->port_list, list) { + if (p->flags & BR_HAIRPIN_MODE) + return -EINVAL; + } + + if (br->stp_enabled == BR_NO_STP) br_stp_start(br); } else { if (br->stp_enabled != BR_NO_STP) br_stp_stop(br); } + + return 0; } /* called under bridge lock */ --- a/net/bridge/br_sysfs_br.c 2009-08-14 14:24:36.874256194 -0700 +++ b/net/bridge/br_sysfs_br.c 2009-08-14 14:33:26.025441102 -0700 @@ -164,6 +164,7 @@ static ssize_t store_stp_state(struct de struct net_bridge *br = to_bridge(d); char *endp; unsigned long val; + int ret; if (!capable(CAP_NET_ADMIN)) return -EPERM; @@ -174,10 +175,11 @@ static ssize_t store_stp_state(struct de if (!rtnl_trylock()) return restart_syscall(); - br_stp_set_enabled(br, val); + + ret = br_stp_set_enabled(br, val); rtnl_unlock(); - return len; + return (ret == 0) ? len : ret; } static DEVICE_ATTR(stp_state, S_IRUGO | S_IWUSR, show_stp_state, store_stp_state); --- a/net/bridge/br_sysfs_if.c 2009-08-14 14:24:36.888356879 -0700 +++ b/net/bridge/br_sysfs_if.c 2009-08-14 14:34:55.339272738 -0700 @@ -150,10 +150,13 @@ static ssize_t show_hairpin_mode(struct } static ssize_t store_hairpin_mode(struct net_bridge_port *p, unsigned long v) { - if (v) + if (!v) + p->flags &= ~BR_HAIRPIN_MODE; + else if (p->br->stp_enabled == BR_NO_STP) p->flags |= BR_HAIRPIN_MODE; else - p->flags &= ~BR_HAIRPIN_MODE; + return -EINVAL; + return 0; } static BRPORT_ATTR(hairpin_mode, S_IRUGO | S_IWUSR, --- a/net/bridge/br_private.h 2009-08-14 14:34:05.263278817 -0700 +++ b/net/bridge/br_private.h 2009-08-14 14:34:15.717297908 -0700 @@ -218,7 +218,7 @@ extern void br_become_designated_port(st /* br_stp_if.c */ extern void br_stp_enable_bridge(struct net_bridge *br); extern void br_stp_disable_bridge(struct net_bridge *br); -extern void br_stp_set_enabled(struct net_bridge *br, unsigned long val); +extern int br_stp_set_enabled(struct net_bridge *br, unsigned long val); extern void br_stp_enable_port(struct net_bridge_port *p); extern void br_stp_disable_port(struct net_bridge_port *p); extern void br_stp_recalculate_bridge_id(struct net_bridge *br);