| Message ID | 1391759306-24956-3-git-send-email-makita.toshiaki@lab.ntt.co.jp |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 7 Feb 2014 16:48:19 +0900 Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > Since commit bc9a25d21ef8 ("bridge: Add vlan support for local fdb entries"), > br_fdb_changeaddr() has inserted a new local fdb entry only if it can > find old one. But if we have two ports where they have the same address > or user has deleted a local entry, there will be no entry for one of the > ports. > > Example of problematic case: > ip link set eth0 address aa:bb:cc:dd:ee:ff > ip link set eth1 address aa:bb:cc:dd:ee:ff > brctl addif br0 eth0 > brctl addif br0 eth1 # eth1 will not have a local entry due to dup. I think the second addif should fail, it doesn't seem valid to have two interfaces on same bridge with same address. Most hardware switches would disable the port in that case. -- 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 Fri, 2014-02-07 at 09:31 -0700, Stephen Hemminger wrote: > On Fri, 7 Feb 2014 16:48:19 +0900 > Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > > > Since commit bc9a25d21ef8 ("bridge: Add vlan support for local fdb entries"), > > br_fdb_changeaddr() has inserted a new local fdb entry only if it can > > find old one. But if we have two ports where they have the same address > > or user has deleted a local entry, there will be no entry for one of the > > ports. > > > > Example of problematic case: > > ip link set eth0 address aa:bb:cc:dd:ee:ff > > ip link set eth1 address aa:bb:cc:dd:ee:ff > > brctl addif br0 eth0 > > brctl addif br0 eth1 # eth1 will not have a local entry due to dup. > > I think the second addif should fail, it doesn't seem valid to have > two interfaces on same bridge with same address. Most hardware switches > would disable the port in that case. Thank you for your comment, but I don't think so for several reasons. - From other network elements on the same network, bridge ports don't appear to have a mac address, but the bridge appears to have several mac addresses that can reach to the bridge. The duplicated address is simply seen as one of those addresses. I don't think it is a problem. - This operation (add a port that has duplicated address) has allowed for several years, and it is obviously intended, as commented in fdb_insert(). 417 /* it is okay to have multiple ports with same 418 * address, just use the first one. 419 */ - Hardware switches usually have one mac address per one switch. Their ports don't have mac addresses. It is not reasonable to compare hardware switches. Thanks, 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_fdb.c b/net/bridge/br_fdb.c index ce54119..96ab1d1 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -92,8 +92,10 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f) void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr) { struct net_bridge *br = p->br; - bool no_vlan = (nbp_get_vlan_info(p) == NULL) ? true : false; + struct net_port_vlans *pv = nbp_get_vlan_info(p); + bool no_vlan = !pv; int i; + u16 vid; spin_lock_bh(&br->hash_lock); @@ -114,28 +116,37 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr) f->addr.addr) && nbp_vlan_find(op, vid)) { f->dst = op; - goto insert; + goto skip_delete; } } /* delete old one */ fdb_delete(br, f); -insert: - /* insert new address, may fail if invalid - * address or dup. - */ - fdb_insert(br, p, newaddr, vid); - +skip_delete: /* if this port has no vlan information * configured, we can safely be done at * this point. */ if (no_vlan) - goto done; + goto insert; } } } +insert: + /* insert new address, may fail if invalid address or dup. */ + fdb_insert(br, p, newaddr, 0); + + if (no_vlan) + goto done; + + /* Now add entries for every VLAN configured on the port. + * This function runs under RTNL so the bitmap will not change + * from under us. + */ + for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) + fdb_insert(br, p, newaddr, vid); + done: spin_unlock_bh(&br->hash_lock); }
Since commit bc9a25d21ef8 ("bridge: Add vlan support for local fdb entries"), br_fdb_changeaddr() has inserted a new local fdb entry only if it can find old one. But if we have two ports where they have the same address or user has deleted a local entry, there will be no entry for one of the ports. Example of problematic case: ip link set eth0 address aa:bb:cc:dd:ee:ff ip link set eth1 address aa:bb:cc:dd:ee:ff brctl addif br0 eth0 brctl addif br0 eth1 # eth1 will not have a local entry due to dup. ip link set eth1 address 12:34:56:78:90:ab Then, the new entry for the address 12:34:56:78:90:ab will not be created, and the bridge device will not be able to communicate. Insert new entries regardless of whether we can find old entries or not. Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> Acked-by: Vlad Yasevich <vyasevic@redhat.com> --- net/bridge/br_fdb.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)