Linux Ethernet Bridging - Mar 2014 - [Bridge] [PATCH 1/3] bridge: preserve random init MAC address

From: "Luis R. Rodriguez" <mcgrof at 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 at networkplumber.org>
Cc: bridge at lists.linux-foundation.org
Cc: netdev at vger.kernel.org
Cc: linux-kernel at vger.kernel.org
Cc: xen-devel at lists.xenproject.org
Cc: kvm at vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof at 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;
 }
-- 
1.8.5.3

(2014/03/13 12:15), Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof at 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 at networkplumber.org>
> Cc: bridge at lists.linux-foundation.org
> Cc: netdev at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: xen-devel at lists.xenproject.org
> Cc: kvm at vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof at 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

On Wed, 12 Mar 2014 20:15:25 -0700
"Luis R. Rodriguez" <mcgrof at do-not-panic.com> wrote:
> 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.
What is supposed to happen is that the recalculate chooses
the lowest MAC address of the slaves. If there are no slaves
it might as well just calculate a new random value. There is
not great merit in preserving the original defunct address.

Or something like this that just keeps the old value.
The bridge is in a meaningless state when there are no ports,
and when the first port is added back it will be used as the
new bridge id.

--- a/net/bridge/br_stp_if.c	2014-02-12 08:21:56.733857356 -0800
+++ b/net/bridge/br_stp_if.c	2014-03-18 20:09:09.334388826 -0700
@@ -235,6 +235,9 @@ bool br_stp_recalculate_bridge_id(struct
 			addr = p->dev->dev_addr;
 
 	}
+
+	if (addr == br_mac_zero)
+		return false;  /* keep original address */
 
 	if (ether_addr_equal(br->bridge_id.addr, addr))
 		return false;	/* no change */

On Tue, Mar 18, 2014 at 08:10:56PM -0700, Stephen Hemminger
wrote:
> On Wed, 12 Mar 2014 20:15:25 -0700
> "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
> 
> > 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.
> 
> What is supposed to happen is that the recalculate chooses
> the lowest MAC address of the slaves. If there are no slaves
> it might as well just calculate a new random value. There is
> not great merit in preserving the original defunct address.
> 
> Or something like this
> --- a/net/bridge/br_stp_if.c	2014-02-12 08:21:56.733857356 -0800
> +++ b/net/bridge/br_stp_if.c	2014-03-18 20:09:09.334388826 -0700
> @@ -235,6 +235,9 @@ bool br_stp_recalculate_bridge_id(struct
>  			addr = p->dev->dev_addr;
>  
>  	}
> +
> +	if (addr == br_mac_zero)
> +		return false;  /* keep original address */
>  
>  	if (ether_addr_equal(br->bridge_id.addr, addr))
>  		return false;	/* no change */
> 
> that just keeps the old value.
The old value could be a port which got root blocked, I think
it can be confusing to see that happen. Either way feel free to
make the call, I'll provide more details below on perhaps one reason
to keep the original MAC address.
> The bridge is in a meaningless state when there are no ports,
Some virtualization topologies may want a backend with no link (or
perhaps one which is dynamic, with the option to have none) to the
internet but just a bridge so guests sharing the bridge can talk to 
each other. In this case bridging can be used only to link the
batch of guests.

In this case the bridge simply acts as a switch, but also can be used as the
interface for DHCP, for example. In such a case the guests will be doing
ARP to get to the DHCP server. There's a flurry of ways one can try to get
all this meshed together including enablign an ARP proxy but I'm looking
at ways to optimize this further -- but I'd like to address the current
usage cases first.
> and when the first port is added back it will be used as the
> new bridge id.
Sure. Let me know how you think we should proceed with this patch based
on the above.

  Luis