Message ID | 1394680527-28970-2-git-send-email-mcgrof@do-not-panic.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
(2014/03/13 12:15), Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > As it is now if you add create a bridge it gets started > with a random MAC address and if you then add a net_device > as a slave but later kick it out you end up with a zero > MAC address. Instead preserve the original random MAC > address and use it. > > If you manually set the bridge address that will always > be respected. This change only takes effect if at the time > of computing the new root port we determine we have found > no candidates. > > Cc: Stephen Hemminger <stephen@networkplumber.org> > Cc: bridge@lists.linux-foundation.org > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: xen-devel@lists.xenproject.org > Cc: kvm@vger.kernel.org > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> > --- > net/bridge/br_device.c | 1 + > net/bridge/br_private.h | 1 + > net/bridge/br_stp_if.c | 3 +++ > 3 files changed, 5 insertions(+) > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > index b063050..5f13eac 100644 > --- a/net/bridge/br_device.c > +++ b/net/bridge/br_device.c > @@ -368,6 +368,7 @@ void br_dev_setup(struct net_device *dev) > br->bridge_id.prio[1] = 0x00; > > ether_addr_copy(br->group_addr, eth_reserved_addr_base); > + ether_addr_copy(br->random_init_addr, dev->dev_addr); > > br->stp_enabled = BR_NO_STP; > br->group_fwd_mask = BR_GROUPFWD_DEFAULT; > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index e1ca1dc..32a06da 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -240,6 +240,7 @@ struct net_bridge > unsigned long bridge_hello_time; > unsigned long bridge_forward_delay; > > + u8 random_init_addr[ETH_ALEN]; > u8 group_addr[ETH_ALEN]; > u16 root_port; > > diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c > index 189ba1e..4c9ad45 100644 > --- a/net/bridge/br_stp_if.c > +++ b/net/bridge/br_stp_if.c > @@ -239,6 +239,9 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br) > if (ether_addr_equal(br->bridge_id.addr, addr)) > return false; /* no change */ > > + if (ether_addr_equal(addr, br_mac_zero)) > + addr = br->random_init_addr; > + > br_stp_change_bridge_id(br, addr); > return true; > } nit, If the last detached port happens to have the same addr as random_init_addr, this seems to call br_stp_change_bridge_id() even though bridge_id is not changed. Shouldn't the assignment of random_init_addr be done before the check of "no change"? Toshiaki Makita -- 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 Tue, Mar 18, 2014 at 5:42 PM, Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > nit, > If the last detached port happens to have the same addr as > random_init_addr, this seems to call br_stp_change_bridge_id() even > though bridge_id is not changed. Ah good point. > Shouldn't the assignment of random_init_addr be done before the check of > "no change"? Good question, should we even allow two ports to have the same MAC address or should we complain and refuse to add it? If so that should mean we should also have to monitor any manual address changes or events for address changes on the ports. Stephen? Luis -- 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
(2014/03/19 9:50), Luis R. Rodriguez wrote: > On Tue, Mar 18, 2014 at 5:42 PM, Toshiaki Makita > <makita.toshiaki@lab.ntt.co.jp> wrote: >> nit, >> If the last detached port happens to have the same addr as >> random_init_addr, this seems to call br_stp_change_bridge_id() even >> though bridge_id is not changed. > > Ah good point. > >> Shouldn't the assignment of random_init_addr be done before the check of >> "no change"? > > Good question, should we even allow two ports to have the same MAC > address or should we complain and refuse to add it? If so that should > mean we should also have to monitor any manual address changes or > events for address changes on the ports. This was recently discussed by Stephen and me. I'm thinking it should be allowed. http://marc.info/?l=linux-netdev&m=139182743919257&w=2 Toshiaki Makita > > Stephen? > > Luis -- 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 Tue, Mar 18, 2014 at 6:04 PM, Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > (2014/03/19 9:50), Luis R. Rodriguez wrote: >> On Tue, Mar 18, 2014 at 5:42 PM, Toshiaki Makita >> <makita.toshiaki@lab.ntt.co.jp> wrote: >>> nit, >>> If the last detached port happens to have the same addr as >>> random_init_addr, this seems to call br_stp_change_bridge_id() even >>> though bridge_id is not changed. >> >> Ah good point. >> >>> Shouldn't the assignment of random_init_addr be done before the check of >>> "no change"? >> >> Good question, should we even allow two ports to have the same MAC >> address or should we complain and refuse to add it? If so that should >> mean we should also have to monitor any manual address changes or >> events for address changes on the ports. > > This was recently discussed by Stephen and me. > I'm thinking it should be allowed. > > http://marc.info/?l=linux-netdev&m=139182743919257&w=2 Great now that that's sorted out though I still think calling br_stp_change_bridge_id() is right just as calling the update features as the device is different. It could however be confusing when this situation is run and folks might report odd bugs unless we could tell them apart clearly. Thoughts? Luis -- 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 Tue, 2014-03-18 at 18:10 -0700, Luis R. Rodriguez wrote: > On Tue, Mar 18, 2014 at 6:04 PM, Toshiaki Makita > <makita.toshiaki@lab.ntt.co.jp> wrote: > > (2014/03/19 9:50), Luis R. Rodriguez wrote: > >> On Tue, Mar 18, 2014 at 5:42 PM, Toshiaki Makita > >> <makita.toshiaki@lab.ntt.co.jp> wrote: > >>> nit, > >>> If the last detached port happens to have the same addr as > >>> random_init_addr, this seems to call br_stp_change_bridge_id() even > >>> though bridge_id is not changed. > >> > >> Ah good point. > >> > >>> Shouldn't the assignment of random_init_addr be done before the check of > >>> "no change"? > >> > >> Good question, should we even allow two ports to have the same MAC > >> address or should we complain and refuse to add it? If so that should > >> mean we should also have to monitor any manual address changes or > >> events for address changes on the ports. > > > > This was recently discussed by Stephen and me. > > I'm thinking it should be allowed. > > > > http://marc.info/?l=linux-netdev&m=139182743919257&w=2 > > Great now that that's sorted out though I still think calling > br_stp_change_bridge_id() is right just as calling the update features > as the device is different. It could however be confusing when this > situation is run and folks might report odd bugs unless we could tell > them apart clearly. Thoughts? br_stp_change_bridge_id() is currently called only if bridge_id.addr should be changed. If the addr should not be changed but some updates are needed, br_stp_recalculate_bridge_id() doesn't seem to fit into it. Toshiaki Makita -- 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_device.c b/net/bridge/br_device.c index b063050..5f13eac 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -368,6 +368,7 @@ void br_dev_setup(struct net_device *dev) br->bridge_id.prio[1] = 0x00; ether_addr_copy(br->group_addr, eth_reserved_addr_base); + ether_addr_copy(br->random_init_addr, dev->dev_addr); br->stp_enabled = BR_NO_STP; br->group_fwd_mask = BR_GROUPFWD_DEFAULT; diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index e1ca1dc..32a06da 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -240,6 +240,7 @@ struct net_bridge unsigned long bridge_hello_time; unsigned long bridge_forward_delay; + u8 random_init_addr[ETH_ALEN]; u8 group_addr[ETH_ALEN]; u16 root_port; diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c index 189ba1e..4c9ad45 100644 --- a/net/bridge/br_stp_if.c +++ b/net/bridge/br_stp_if.c @@ -239,6 +239,9 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br) if (ether_addr_equal(br->bridge_id.addr, addr)) return false; /* no change */ + if (ether_addr_equal(addr, br_mac_zero)) + addr = br->random_init_addr; + br_stp_change_bridge_id(br, addr); return true; }