Patchwork bonding: only use primary address for ARP

login
register
mail settings
Submitter Henrik Saavedra Persson
Date Nov. 24, 2011, 9:37 a.m.
Message ID <20111124093715.GA25365@as68121.uab.ericsson.se>
Download mbox | patch
Permalink /patch/127462/
State Accepted
Delegated to: David Miller
Headers show

Comments

Henrik Saavedra Persson - Nov. 24, 2011, 9:37 a.m.
Only use the primary address of the bond device
for master_ip. This will prevent changing the ARP source
address in Active-Backup mode whenever a secondry address
is added to the bond device.

Signed-off-by: Henrik Saavedra Persson <henrik.e.persson@ericsson.com>
---
 drivers/net/bonding/bond_main.c |   33 ++++++---------------------------
 1 files changed, 6 insertions(+), 27 deletions(-)
David Miller - Nov. 29, 2011, 11:33 p.m.
From: Henrik Saavedra Persson <henrik.e.persson@ericsson.com>
Date: Thu, 24 Nov 2011 10:37:15 +0100

> Only use the primary address of the bond device
> for master_ip. This will prevent changing the ARP source
> address in Active-Backup mode whenever a secondry address
> is added to the bond device.
> 
> Signed-off-by: Henrik Saavedra Persson <henrik.e.persson@ericsson.com>

Bonding hackers, please review this.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gospodarek - Nov. 30, 2011, 10:07 p.m.
On Thu, Nov 24, 2011 at 10:37:15AM +0100, Henrik Saavedra Persson wrote:
> Only use the primary address of the bond device
> for master_ip. This will prevent changing the ARP source
> address in Active-Backup mode whenever a secondry address
> is added to the bond device.
> 
> Signed-off-by: Henrik Saavedra Persson <henrik.e.persson@ericsson.com>
> ---
>  drivers/net/bonding/bond_main.c |   33 ++++++---------------------------
>  1 files changed, 6 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 6191e63..ec293b3 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2551,30 +2551,6 @@ out:
>  	}
>  }
>  
> -static __be32 bond_glean_dev_ip(struct net_device *dev)
> -{
> -	struct in_device *idev;
> -	struct in_ifaddr *ifa;
> -	__be32 addr = 0;
> -
> -	if (!dev)
> -		return 0;
> -
> -	rcu_read_lock();
> -	idev = __in_dev_get_rcu(dev);
> -	if (!idev)
> -		goto out;
> -
> -	ifa = idev->ifa_list;
> -	if (!ifa)
> -		goto out;
> -
> -	addr = ifa->ifa_local;
> -out:
> -	rcu_read_unlock();
> -	return addr;
> -}
> -
>  static int bond_has_this_ip(struct bonding *bond, __be32 ip)
>  {
>  	struct vlan_entry *vlan;
> @@ -3316,6 +3292,10 @@ static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
>  	struct bonding *bond;
>  	struct vlan_entry *vlan;
>  
> +	/* we only care about primary address */
> +	if(ifa->ifa_flags & IFA_F_SECONDARY)
> +		return NOTIFY_DONE;
> +

This change is good.  We need to stop overwriting bond->master_ip when
secondary addresses are added.

>  	list_for_each_entry(bond, &bn->dev_list, bond_list) {
>  		if (bond->dev == event_dev) {
>  			switch (event) {
> @@ -3323,7 +3303,7 @@ static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
>  				bond->master_ip = ifa->ifa_local;
>  				return NOTIFY_OK;
>  			case NETDEV_DOWN:
> -				bond->master_ip = bond_glean_dev_ip(bond->dev);
> +				bond->master_ip = 0;
>  				return NOTIFY_OK;
>  			default:
>  				return NOTIFY_DONE;
> @@ -3339,8 +3319,7 @@ static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
>  					vlan->vlan_ip = ifa->ifa_local;
>  					return NOTIFY_OK;
>  				case NETDEV_DOWN:
> -					vlan->vlan_ip =
> -						bond_glean_dev_ip(vlan_dev);
> +					vlan->vlan_ip = 0;
>  					return NOTIFY_OK;
>  				default:
>  					return NOTIFY_DONE;

Inspection indicates this is safe too.  The only code that appears to
check against master_up and vlan_ip appears to be in ARP monitoring code
and it does not make much sense to use ARP monitoring when your host does
not have an IP address.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Dec. 1, 2011, 4 a.m.
From: Andy Gospodarek <andy@greyhouse.net>
Date: Wed, 30 Nov 2011 17:07:10 -0500

> On Thu, Nov 24, 2011 at 10:37:15AM +0100, Henrik Saavedra Persson wrote:
>> Only use the primary address of the bond device
>> for master_ip. This will prevent changing the ARP source
>> address in Active-Backup mode whenever a secondry address
>> is added to the bond device.
>> 
>> Signed-off-by: Henrik Saavedra Persson <henrik.e.persson@ericsson.com>
 ...
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>

Applied, thanks for reviewing Andy.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6191e63..ec293b3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2551,30 +2551,6 @@  out:
 	}
 }
 
-static __be32 bond_glean_dev_ip(struct net_device *dev)
-{
-	struct in_device *idev;
-	struct in_ifaddr *ifa;
-	__be32 addr = 0;
-
-	if (!dev)
-		return 0;
-
-	rcu_read_lock();
-	idev = __in_dev_get_rcu(dev);
-	if (!idev)
-		goto out;
-
-	ifa = idev->ifa_list;
-	if (!ifa)
-		goto out;
-
-	addr = ifa->ifa_local;
-out:
-	rcu_read_unlock();
-	return addr;
-}
-
 static int bond_has_this_ip(struct bonding *bond, __be32 ip)
 {
 	struct vlan_entry *vlan;
@@ -3316,6 +3292,10 @@  static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
 	struct bonding *bond;
 	struct vlan_entry *vlan;
 
+	/* we only care about primary address */
+	if(ifa->ifa_flags & IFA_F_SECONDARY)
+		return NOTIFY_DONE;
+
 	list_for_each_entry(bond, &bn->dev_list, bond_list) {
 		if (bond->dev == event_dev) {
 			switch (event) {
@@ -3323,7 +3303,7 @@  static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
 				bond->master_ip = ifa->ifa_local;
 				return NOTIFY_OK;
 			case NETDEV_DOWN:
-				bond->master_ip = bond_glean_dev_ip(bond->dev);
+				bond->master_ip = 0;
 				return NOTIFY_OK;
 			default:
 				return NOTIFY_DONE;
@@ -3339,8 +3319,7 @@  static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
 					vlan->vlan_ip = ifa->ifa_local;
 					return NOTIFY_OK;
 				case NETDEV_DOWN:
-					vlan->vlan_ip =
-						bond_glean_dev_ip(vlan_dev);
+					vlan->vlan_ip = 0;
 					return NOTIFY_OK;
 				default:
 					return NOTIFY_DONE;