Fix bridge MAC change notification.

This is my first time on netdev. I am writing about the ensuring
that upper network layers are aware of bridge MAC changes due
to re-configuration or port addition/removal.

It seems this topic has cropped up before, with one patch by 
Stephen Hemminger already in 3.0, and a fix for it (sent 7/22)
(http://marc.info/?l=linux-netdev&m=131135705613958&w=2) pending.

The existing patches don't cover the br_del_if case, the case of 
a changing MAC address on a port associated with the bridge, and
the case of manually setting the MAC address, which the following
patch remedies.

Table of Contents:
[PATCH] Bridge: Always send NETDEV_CHANGEADDR up on br MAC change.

Thank you,
A

Minor fix in br_notify, such that notification only is sent if 
bridge MAC address is updated.

ToC:
[PATCHv2] Bridge: Always send NETDEV_CHANGEADDR up on br MAC change.

A

This ensures the neighbor entries associated with the bridge
dev are flushed, also invalidating the associated cached L2 headers.

This means we br_add_if/br_del_if ports to implement hand-over and
not wind up with bridge packets going out with stale MAC.

This means we can also change MAC of port device and also not wind
up with bridge packets going out with stale MAC.

This builds on Stephen Hemminger's patch, also handling the br_del_if
case, port MAC change case, and bridge MAC manual assignment case.

Change-Id: I6039ba021006f854e0e7e83dd1c4261c500aeab7
Cc: Stephen Hemminger <redacted>
Signed-off-by: Andrei Warkentin <redacted>
---
 net/bridge/br_device.c |    1 +
 net/bridge/br_if.c     |    6 +++++-
 net/bridge/br_notify.c |    6 +++++-
 net/bridge/br_stp_if.c |    2 +-
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index cf09fe5..ef18070 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c

@@ -162,6 +162,7 @@ static int br_set_mac_address(struct net_device *dev, void *p)
 	br->flags |= BR_SET_MAC_ADDR;
 	spin_unlock_bh(&br->lock);
 
+	call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
 	return 0;
 }
 

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 204e542..36ad887 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -481,6 +481,7 @@ put_back:
 int br_del_if(struct net_bridge *br, struct net_device *dev)
 {
 	struct net_bridge_port *p;
+	bool changed_addr;
 
 	if (!br_port_exists(dev))
 		return -EINVAL;

@@ -492,10 +493,13 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
-	br_stp_recalculate_bridge_id(br);
+	changed_addr = br_stp_recalculate_bridge_id(br);
 	br_features_recompute(br);
 	spin_unlock_bh(&br->lock);
 
+	if (changed_addr)
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
+
 	return 0;
 }
 

diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index 404d4e1..651eac3 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c

@@ -34,6 +34,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	struct net_device *dev = ptr;
 	struct net_bridge_port *p = br_port_get(dev);
 	struct net_bridge *br;
+	bool changed_addr;
 	int err;
 
 	/* not a port of a bridge */

@@ -51,8 +52,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	case NETDEV_CHANGEADDR:
 		spin_lock_bh(&br->lock);
 		br_fdb_changeaddr(p, dev->dev_addr);
-		br_stp_recalculate_bridge_id(br);
+		changed_addr = br_stp_recalculate_bridge_id(br);
 		spin_unlock_bh(&br->lock);
+
+		if (changed_addr)
+			call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
 		break;
 
 	case NETDEV_CHANGE:

diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index c0990ba..4528e9a 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c

@@ -213,7 +213,7 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)
 
 	/* user has chosen a value so keep it */
 	if (br->flags & BR_SET_MAC_ADDR)
-		return;
+		return false;
 
 	list_for_each_entry(p, &br->port_list, list) {
 		if (addr == br_mac_zero ||

-- 
1.7.0.4

Hi,

I apologize for spamming. I removed the meaningless Change-Id from
the patch.

ToC:
[PATCHv3] Bridge: Always send NETDEV_CHANGEADDR up on br MAC change.

A

This ensures the neighbor entries associated with the bridge
dev are flushed, also invalidating the associated cached L2 headers.

This means we br_add_if/br_del_if ports to implement hand-over and
not wind up with bridge packets going out with stale MAC.

This means we can also change MAC of port device and also not wind
up with bridge packets going out with stale MAC.

This builds on Stephen Hemminger's patch, also handling the br_del_if
case, port MAC change case, and bridge MAC manual assignment case.

Cc: Stephen Hemminger <redacted>
Signed-off-by: Andrei Warkentin <redacted>
---
 net/bridge/br_device.c |    1 +
 net/bridge/br_if.c     |    6 +++++-
 net/bridge/br_notify.c |    6 +++++-
 net/bridge/br_stp_if.c |    2 +-
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index cf09fe5..ef18070 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c

@@ -162,6 +162,7 @@ static int br_set_mac_address(struct net_device *dev, void *p)
 	br->flags |= BR_SET_MAC_ADDR;
 	spin_unlock_bh(&br->lock);
 
+	call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
 	return 0;
 }
 

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 204e542..36ad887 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -481,6 +481,7 @@ put_back:
 int br_del_if(struct net_bridge *br, struct net_device *dev)
 {
 	struct net_bridge_port *p;
+	bool changed_addr;
 
 	if (!br_port_exists(dev))
 		return -EINVAL;

@@ -492,10 +493,13 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
-	br_stp_recalculate_bridge_id(br);
+	changed_addr = br_stp_recalculate_bridge_id(br);
 	br_features_recompute(br);
 	spin_unlock_bh(&br->lock);
 
+	if (changed_addr)
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
+
 	return 0;
 }
 

diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index 404d4e1..651eac3 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c

@@ -34,6 +34,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	struct net_device *dev = ptr;
 	struct net_bridge_port *p = br_port_get(dev);
 	struct net_bridge *br;
+	bool changed_addr;
 	int err;
 
 	/* not a port of a bridge */

@@ -51,8 +52,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	case NETDEV_CHANGEADDR:
 		spin_lock_bh(&br->lock);
 		br_fdb_changeaddr(p, dev->dev_addr);
-		br_stp_recalculate_bridge_id(br);
+		changed_addr = br_stp_recalculate_bridge_id(br);
 		spin_unlock_bh(&br->lock);
+
+		if (changed_addr)
+			call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
 		break;
 
 	case NETDEV_CHANGE:

diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index c0990ba..4528e9a 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c

@@ -213,7 +213,7 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)
 
 	/* user has chosen a value so keep it */
 	if (br->flags & BR_SET_MAC_ADDR)
-		return;
+		return false;
 
 	list_for_each_entry(p, &br->port_list, list) {
 		if (addr == br_mac_zero ||

-- 
1.7.0.4

On Thu,  4 Aug 2011 21:17:05 -0500
Andrei Warkentin [off-list ref] wrote:

Half ok, half not.

This is unnecessary since already done by dev_set_mac_address.

Good, I forgot that case.

This is also fine.

This is already in net-next.

This ensures the neighbor entries associated with the bridge
dev are flushed, also invalidating the associated cached L2 headers.

This means we br_add_if/br_del_if ports to implement hand-over and
not wind up with bridge packets going out with stale MAC.

This means we can also change MAC of port device and also not wind
up with bridge packets going out with stale MAC.

This builds on Stephen Hemminger's patch, also handling the br_del_if
case, port MAC change case, and bridge MAC manual assignment case.

Change-Id: I6039ba021006f854e0e7e83dd1c4261c500aeab7
Cc: Stephen Hemminger <redacted>
Signed-off-by: Andrei Warkentin <redacted>
---
 net/bridge/br_device.c |    1 +
 net/bridge/br_if.c     |    6 +++++-
 net/bridge/br_notify.c |    2 ++
 net/bridge/br_stp_if.c |    2 +-
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index cf09fe5..ef18070 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c

@@ -162,6 +162,7 @@ static int br_set_mac_address(struct net_device *dev, void *p)
 	br->flags |= BR_SET_MAC_ADDR;
 	spin_unlock_bh(&br->lock);
 
+	call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
 	return 0;
 }
 

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 204e542..36ad887 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -481,6 +481,7 @@ put_back:
 int br_del_if(struct net_bridge *br, struct net_device *dev)
 {
 	struct net_bridge_port *p;
+	bool changed_addr;
 
 	if (!br_port_exists(dev))
 		return -EINVAL;

@@ -492,10 +493,13 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
-	br_stp_recalculate_bridge_id(br);
+	changed_addr = br_stp_recalculate_bridge_id(br);
 	br_features_recompute(br);
 	spin_unlock_bh(&br->lock);
 
+	if (changed_addr)
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
+
 	return 0;
 }
 

diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index 404d4e1..c7160b1 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c

@@ -53,6 +53,8 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 		br_fdb_changeaddr(p, dev->dev_addr);
 		br_stp_recalculate_bridge_id(br);
 		spin_unlock_bh(&br->lock);
+
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
 		break;
 
 	case NETDEV_CHANGE:

diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index c0990ba..4528e9a 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c

@@ -213,7 +213,7 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)
 
 	/* user has chosen a value so keep it */
 	if (br->flags & BR_SET_MAC_ADDR)
-		return;
+		return false;
 
 	list_for_each_entry(p, &br->port_list, list) {
 		if (addr == br_mac_zero ||

-- 
1.7.0.4

Hi Stephen,

On Thu, Aug 4, 2011 at 11:36 PM, Stephen Hemminger
[off-list ref] wrote:

On Thu,  4 Aug 2011 21:17:05 -0500
Andrei Warkentin [off-list ref] wrote:

Half ok, half not.

quoted

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index cf09fe5..ef18070 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c

@@ -162,6 +162,7 @@ static int br_set_mac_address(struct net_device *dev, void *p)

      br->flags |= BR_SET_MAC_ADDR;
      spin_unlock_bh(&br->lock);

+     call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
      return 0;
 }

This is unnecessary since already done by dev_set_mac_address.

quoted

diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index c0990ba..4528e9a 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c

@@ -213,7 +213,7 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)

      /* user has chosen a value so keep it */
      if (br->flags & BR_SET_MAC_ADDR)
-             return;
+             return false;

      list_for_each_entry(p, &br->port_list, list) {
              if (addr == br_mac_zero ||

This is already in net-next.

Thank you for your feedback. I will clean this up and resubmit tomorrow.

A

Here is the v4 of the patch, now rebased on net-next-2.6.

ToC:
[PATCHv4] Bridge: Always send NETDEV_CHANGEADDR up on br MAC change.

Thank you,
A

This ensures the neighbor entries associated with the bridge
dev are flushed, also invalidating the associated cached L2 headers.

This means we br_add_if/br_del_if ports to implement hand-over and
not wind up with bridge packets going out with stale MAC.

This means we can also change MAC of port device and also not wind
up with bridge packets going out with stale MAC.

This builds on Stephen Hemminger's patch, also handling the br_del_if
case and the port MAC change case.

Cc: Stephen Hemminger <redacted>
Signed-off-by: Andrei Warkentin <redacted>
---
 net/bridge/br_if.c     |    6 +++++-
 net/bridge/br_notify.c |    7 ++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 3176e2e..2cdf007 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -417,6 +417,7 @@ put_back:
 int br_del_if(struct net_bridge *br, struct net_device *dev)
 {
 	struct net_bridge_port *p;
+	bool changed_addr;
 
 	p = br_port_get_rtnl(dev);
 	if (!p || p->br != br)

@@ -425,9 +426,12 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
-	br_stp_recalculate_bridge_id(br);
+	changed_addr = br_stp_recalculate_bridge_id(br);
 	spin_unlock_bh(&br->lock);
 
+	if (changed_addr)
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
+
 	netdev_update_features(br->dev);
 
 	return 0;

diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index 6545ee9..a76b621 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c

@@ -34,6 +34,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	struct net_device *dev = ptr;
 	struct net_bridge_port *p;
 	struct net_bridge *br;
+	bool changed_addr;
 	int err;
 
 	/* register of bridge completed, add sysfs entries */

@@ -57,8 +58,12 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	case NETDEV_CHANGEADDR:
 		spin_lock_bh(&br->lock);
 		br_fdb_changeaddr(p, dev->dev_addr);
-		br_stp_recalculate_bridge_id(br);
+		changed_addr = br_stp_recalculate_bridge_id(br);
 		spin_unlock_bh(&br->lock);
+
+		if (changed_addr)
+			call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
+
 		break;
 
 	case NETDEV_CHANGE:

-- 
1.7.0.4

On Fri,  5 Aug 2011 16:04:10 -0500
Andrei Warkentin [off-list ref] wrote:

This ensures the neighbor entries associated with the bridge
dev are flushed, also invalidating the associated cached L2 headers.

This means we br_add_if/br_del_if ports to implement hand-over and
not wind up with bridge packets going out with stale MAC.

This means we can also change MAC of port device and also not wind
up with bridge packets going out with stale MAC.

This builds on Stephen Hemminger's patch, also handling the br_del_if
case and the port MAC change case.

Cc: Stephen Hemminger <redacted>
Signed-off-by: Andrei Warkentin <redacted>

Acked-by: Stephen Hemminger <redacted>

On Mon, Aug 8, 2011 at 3:12 PM, Stephen Hemminger [off-list ref] wrote:

On Fri,  5 Aug 2011 16:04:10 -0500
Andrei Warkentin [off-list ref] wrote:

quoted

This ensures the neighbor entries associated with the bridge
dev are flushed, also invalidating the associated cached L2 headers.

This means we br_add_if/br_del_if ports to implement hand-over and
not wind up with bridge packets going out with stale MAC.

This means we can also change MAC of port device and also not wind
up with bridge packets going out with stale MAC.

This builds on Stephen Hemminger's patch, also handling the br_del_if
case and the port MAC change case.

Cc: Stephen Hemminger <redacted>
Signed-off-by: Andrei Warkentin <redacted>

Acked-by: Stephen Hemminger <redacted>

Thank you for the Ack.

A

From: Stephen Hemminger <redacted>
Date: Mon, 8 Aug 2011 13:12:30 -0700

On Fri,  5 Aug 2011 16:04:10 -0500
Andrei Warkentin [off-list ref] wrote:

quoted

This ensures the neighbor entries associated with the bridge
dev are flushed, also invalidating the associated cached L2 headers.

This means we br_add_if/br_del_if ports to implement hand-over and
not wind up with bridge packets going out with stale MAC.

This means we can also change MAC of port device and also not wind
up with bridge packets going out with stale MAC.

This builds on Stephen Hemminger's patch, also handling the br_del_if
case and the port MAC change case.

Cc: Stephen Hemminger <redacted>
Signed-off-by: Andrei Warkentin <redacted>

Acked-by: Stephen Hemminger <redacted>

Applied, thanks.