From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Yasevich Subject: Re: [PATCH net v2 4/9] bridge: Change local fdb entries whenever mac address of bridge device changes Date: Tue, 17 Dec 2013 13:45:49 -0500 Message-ID: <52B09BDD.3090405@redhat.com> References: <1387281821-21342-1-git-send-email-makita.toshiaki@lab.ntt.co.jp> <1387281821-21342-5-git-send-email-makita.toshiaki@lab.ntt.co.jp> <52B07A56.9070005@redhat.com> Reply-To: vyasevic@redhat.com Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit To: Toshiaki Makita , "David S . Miller" , Stephen Hemminger , netdev@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:63554 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755711Ab3LQSpz (ORCPT ); Tue, 17 Dec 2013 13:45:55 -0500 In-Reply-To: <52B07A56.9070005@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On 12/17/2013 11:22 AM, Vlad Yasevich wrote: > On 12/17/2013 07:03 AM, Toshiaki Makita wrote: >> Vlan code may need fdb change when changing mac address of bridge device >> even if it is caused by the mac address changing of a bridge port. >> >> Example configuration: >> ip link set eth0 address 12:34:56:78:90:ab >> ip link set eth1 address aa:bb:cc:dd:ee:ff >> brctl addif br0 eth0 >> brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab >> bridge vlan add dev br0 vid 10 self >> bridge vlan add dev eth0 vid 10 >> We will have fdb entry such that f->dst == NULL, f->vlan_id == 10 and >> f->addr == 12:34:56:78:90:ab at this time. >> Next, change the mac address of eth0 to greater value. >> ip link set eth0 address ee:ff:12:34:56:78 >> Then, mac address of br0 will be recalculated and set to aa:bb:cc:dd:ee:ff. >> However, an entry aa:bb:cc:dd:ee:ff will not be created and we will be not >> able to communicate using br0 on vlan 10. >> >> Address this issue by deleting and adding local entries whenever >> changing the mac address of the bridge device. >> >> If there already exists an entry that has the same address, for example, >> in case that br_fdb_changeaddr() has already inserted it, >> br_fdb_change_mac_address() will simply fail to insert it and no >> duplicated entry will be made, as it was. >> >> This approach also needs br_add_if() to call br_fdb_insert() before >> br_stp_recalculate_bridge_id() so that we don't create an entry whose >> dst == NULL in this function to preserve previous behavior. > > Aren't we still creating this entry even with this change? > > Looking at br_fdb_change_mac_address(), it will always add > an fdb with NULL port. > Never mind. br_fdb_insert() here will return 0 since a local entry already exists... Acked-by: Vlad Yasevich -vlad > May be we can update br_fdb_change_mac_address() to not add > a new entry if we have a local fdb already for the address. > > -vlad > >> >> Note that this is a slight change in behavior where the bridge device can >> receive the traffic to the new address before calling >> br_stp_recalculate_bridge_id() in br_add_if(). >> However, it is not a problem because we have already the address on the >> new port and such a way to insert new one before recalculating bridge id >> is taken in br_device_event() as well. >> >> Signed-off-by: Toshiaki Makita >> --- >> net/bridge/br_device.c | 1 - >> net/bridge/br_if.c | 6 +++--- >> net/bridge/br_stp_if.c | 2 ++ >> 3 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c >> index d967773..1cbdbf1 100644 >> --- a/net/bridge/br_device.c >> +++ b/net/bridge/br_device.c >> @@ -187,7 +187,6 @@ static int br_set_mac_address(struct net_device *dev, void *p) >> >> spin_lock_bh(&br->lock); >> if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) { >> - br_fdb_change_mac_address(br, addr->sa_data); >> /* Mac address will be changed in br_stp_change_bridge_id(). */ >> br_stp_change_bridge_id(br, addr->sa_data); >> } >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index 4bf02ad..779d91c 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -389,6 +389,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) >> if (br->dev->needed_headroom < dev->needed_headroom) >> br->dev->needed_headroom = dev->needed_headroom; >> >> + if (br_fdb_insert(br, p, dev->dev_addr, 0)) >> + netdev_err(dev, "failed insert local address bridge forwarding table\n"); >> + >> spin_lock_bh(&br->lock); >> changed_addr = br_stp_recalculate_bridge_id(br); >> >> @@ -404,9 +407,6 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) >> >> dev_set_mtu(br->dev, br_min_mtu(br)); >> >> - if (br_fdb_insert(br, p, dev->dev_addr, 0)) >> - netdev_err(dev, "failed insert local address bridge forwarding table\n"); >> - >> kobject_uevent(&p->kobj, KOBJ_ADD); >> >> return 0; >> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c >> index 656a6f3..189ba1e 100644 >> --- a/net/bridge/br_stp_if.c >> +++ b/net/bridge/br_stp_if.c >> @@ -194,6 +194,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr) >> >> wasroot = br_is_root_bridge(br); >> >> + br_fdb_change_mac_address(br, addr); >> + >> memcpy(oldaddr, br->bridge_id.addr, ETH_ALEN); >> memcpy(br->bridge_id.addr, addr, ETH_ALEN); >> memcpy(br->dev->dev_addr, addr, ETH_ALEN); >> >