Fix bridge MAC change notification.

Fix bridge MAC change notification.

[Andrei Warkentin]

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

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

[Andrei Warkentin]

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 <shemminger@vyatta.com>
Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 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

Always send NETDEV_CHANGEADDR up

[Andrei Warkentin]

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

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

[Andrei Warkentin]

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 <shemminger@vyatta.com>
Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 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

Always send NETDEV_CHANGEADDR up

[Andrei Warkentin]

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

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

[Andrei Warkentin]

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 <shemminger@vyatta.com>
Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 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

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

[Stephen Hemminger]

On Thu,  4 Aug 2011 21:17:05 -0500
Andrei Warkentin <andreiw@motorola.com> wrote:

Half ok, half not.

> 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.

> @@ -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;
>  }
>  
Good, I forgot that case.

> 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:

This is also fine.

> iff --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.

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

[Andrei Warkentin]

Hi Stephen,

On Thu, Aug 4, 2011 at 11:36 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> On Thu,  4 Aug 2011 21:17:05 -0500
> Andrei Warkentin <andreiw@motorola.com> wrote:
>
> Half ok, half not.
>
>> 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.
>
>> 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

Always send NETDEV_CHANGEADDR up

[Andrei Warkentin]

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

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

[Andrei Warkentin]

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 <shemminger@vyatta.com>
Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
---
 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

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

[Stephen Hemminger]

On Fri,  5 Aug 2011 16:04:10 -0500
Andrei Warkentin <andreiw@motorola.com> 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 <shemminger@vyatta.com>
> Signed-off-by: Andrei Warkentin <andreiw@motorola.com>

Acked-by: Stephen Hemminger <shemminger@vyatta.com>

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

[Andrei Warkentin]

On Mon, Aug 8, 2011 at 3:12 PM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> On Fri,  5 Aug 2011 16:04:10 -0500
> Andrei Warkentin <andreiw@motorola.com> 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 <shemminger@vyatta.com>
>> Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
>
> Acked-by: Stephen Hemminger <shemminger@vyatta.com>
>

Thank you for the Ack.

A

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

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 8 Aug 2011 13:12:30 -0700

> On Fri,  5 Aug 2011 16:04:10 -0500
> Andrei Warkentin <andreiw@motorola.com> 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 <shemminger@vyatta.com>
>> Signed-off-by: Andrei Warkentin <andreiw@motorola.com>
> 
> Acked-by: Stephen Hemminger <shemminger@vyatta.com>

Applied, thanks.