Re: [PATCH net v2 4/9] bridge: Change local fdb entries whenever mac address of bridge device changes

[Vlad Yasevich <vyasevic@redhat.com>]

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.

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 <makita.toshiaki@lab.ntt.co.jp>
> ---
>  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);
>