[PATCH 0/4] bonding: race and inconsistency fixes

[PATCH 0/4] bonding: race and inconsistency fixes

[Nikolay Aleksandrov]

Hello,
 In patch 1/4 a race condition while changing the bonding mode through
 sysfs is fixed. Since no synchronization method is used it can race
 with different functions resulting in different impacts. RTNL is used
 to sync with the most important and dangerous events.
 Patch 2/4 is trivial and improves the debugging output by changing %x
 format to %pI4 for IPv4 addresses in a few pr_debug() calls.
 Patch 3/4 fixes an inconsistent arp_targets state where we have 0 entry
 between (or in the beginning) the valid entries that were obtained which
 is hard to diagnose otherwise.
 Patch 4/4 fixes multiple instances of a race condition which is because of
 calls to bond_3ad_get_active_agg_info without any locking, and since it 
 traverses the slave list this can easily result in NULL ptr dereference or
 use of freed memory.

Best regards,
 Nikolay Aleksandrov

Nikolay Aleksandrov (4):
  bonding: fix set mode race conditions
  bonding: replace %x with %pI4 for IPv4 addresses
  bonding: arp_ip_count and arp_targets can be wrong
  bonding: fix multiple 3ad mode sysfs race conditions

 drivers/net/bonding/bond_main.c  | 25 +++++++++++--------------
 drivers/net/bonding/bond_sysfs.c | 25 ++++++++++++++++++++-----
 2 files changed, 31 insertions(+), 19 deletions(-)

-- 
1.8.1.4

[PATCH 1/4] bonding: fix set mode race conditions

[Nikolay Aleksandrov]

Changing the mode without any locking can result in multiple races (e.g.
upping a bond, enslaving/releasing). Depending on which race is hit the
impact can vary from incosistent bond state to kernel crash.
Use RTNL to synchronize the mode setting with the dangerous races.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_sysfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index ea7a388..77ea237 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -316,6 +316,9 @@ static ssize_t bonding_store_mode(struct device *d,
 	int new_value, ret = count;
 	struct bonding *bond = to_bond(d);
 
+	if (!rtnl_trylock())
+		return restart_syscall();
+
 	if (bond->dev->flags & IFF_UP) {
 		pr_err("unable to update mode of %s because interface is up.\n",
 		       bond->dev->name);
@@ -352,6 +355,7 @@ static ssize_t bonding_store_mode(struct device *d,
 		bond->dev->name, bond_mode_tbl[new_value].modename,
 		new_value);
 out:
+	rtnl_unlock();
 	return ret;
 }
 static DEVICE_ATTR(mode, S_IRUGO | S_IWUSR,
-- 
1.8.1.4

[PATCH 2/4] bonding: replace %x with %pI4 for IPv4 addresses

[Nikolay Aleksandrov]

There're few pr_debug() places that can provide the IPv4 address in
dotted decimal format instead which is more helpful.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d0aade0..1a0cc13 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2555,8 +2555,8 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, __be32 dest_
 {
 	struct sk_buff *skb;
 
-	pr_debug("arp %d on slave %s: dst %x src %x vid %d\n", arp_op,
-		 slave_dev->name, dest_ip, src_ip, vlan_id);
+	pr_debug("arp %d on slave %s: dst %pI4 src %pI4 vid %d\n", arp_op,
+		 slave_dev->name, &dest_ip, &src_ip, vlan_id);
 
 	skb = arp_create(arp_op, ETH_P_ARP, dest_ip, slave_dev, src_ip,
 			 NULL, slave_dev->dev_addr, NULL);
@@ -2588,7 +2588,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		__be32 addr;
 		if (!targets[i])
 			break;
-		pr_debug("basa: target %x\n", targets[i]);
+		pr_debug("basa: target %pI4\n", &targets[i]);
 		if (!bond_vlan_used(bond)) {
 			pr_debug("basa: empty vlan: arp_send\n");
 			addr = bond_confirm_addr(bond->dev, targets[i], 0);
-- 
1.8.1.4

[PATCH 3/4] bonding: arp_ip_count and arp_targets can be wrong

[Nikolay Aleksandrov]

When getting arp_ip_targets if we encounter a bad IP, arp_ip_count still
gets increased and all the targets after the wrong one will not be probed
if arp_interval is enabled after that (unless a new IP target is added
through sysfs) because of the zero entry, in this case reading
arp_ip_target through sysfs will show valid targets even if there's a
zero entry.
Example: 1.2.3.4,4.5.6.7,blah,5.6.7.8
When retrieving the list from arp_ip_target the output would be:
1.2.3.4,4.5.6.7,5.6.7.8
but there will be a 0 entry between 4.5.6.7 and 5.6.7.8. If arp_interval
is enabled after that 5.6.7.8 will never be checked because of that.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
Question about this one: Do we have to disable arp_interval if we have
obtained at least 1 valid IP address ? 
I think this can be addressed in a net-next patch.

 drivers/net/bonding/bond_main.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1a0cc13..d6a96cb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4470,7 +4470,7 @@ int bond_parse_parm(const char *buf, const struct bond_parm_tbl *tbl)
 
 static int bond_check_params(struct bond_params *params)
 {
-	int arp_validate_value, fail_over_mac_value, primary_reselect_value;
+	int arp_validate_value, fail_over_mac_value, primary_reselect_value, i;
 
 	/*
 	 * Convert string parameters.
@@ -4650,19 +4650,18 @@ static int bond_check_params(struct bond_params *params)
 		arp_interval = BOND_LINK_ARP_INTERV;
 	}
 
-	for (arp_ip_count = 0;
-	     (arp_ip_count < BOND_MAX_ARP_TARGETS) && arp_ip_target[arp_ip_count];
-	     arp_ip_count++) {
+	for (arp_ip_count = 0, i = 0;
+	     (arp_ip_count < BOND_MAX_ARP_TARGETS) && arp_ip_target[i]; i++) {
 		/* not complete check, but should be good enough to
 		   catch mistakes */
-		__be32 ip = in_aton(arp_ip_target[arp_ip_count]);
-		if (!isdigit(arp_ip_target[arp_ip_count][0]) ||
-		    ip == 0 || ip == htonl(INADDR_BROADCAST)) {
+		__be32 ip = in_aton(arp_ip_target[i]);
+		if (!isdigit(arp_ip_target[i][0]) || ip == 0 ||
+		    ip == htonl(INADDR_BROADCAST)) {
 			pr_warning("Warning: bad arp_ip_target module parameter (%s), ARP monitoring will not be performed\n",
-				   arp_ip_target[arp_ip_count]);
+				   arp_ip_target[i]);
 			arp_interval = 0;
 		} else {
-			arp_target[arp_ip_count] = ip;
+			arp_target[arp_ip_count++] = ip;
 		}
 	}
 
@@ -4696,8 +4695,6 @@ static int bond_check_params(struct bond_params *params)
 	if (miimon) {
 		pr_info("MII link monitoring set to %d ms\n", miimon);
 	} else if (arp_interval) {
-		int i;
-
 		pr_info("ARP monitoring set to %d ms, validate %s, with %d target(s):",
 			arp_interval,
 			arp_validate_tbl[arp_validate_value].modename,
-- 
1.8.1.4

[PATCH 4/4] bonding: fix multiple 3ad mode sysfs race conditions

[Nikolay Aleksandrov]

When bond_3ad_get_active_agg_info() is used in all show_ad_ functions
it is not protected against slave manipulation and since it walks over
the slaves and uses them, this can easily result in NULL pointer
dereference or use of freed memory.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_sysfs.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 77ea237..81ef36a 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1319,6 +1319,17 @@ static ssize_t bonding_show_mii_status(struct device *d,
 }
 static DEVICE_ATTR(mii_status, S_IRUGO, bonding_show_mii_status, NULL);
 
+/* Wrapper used to hold bond->lock so no slave manipulation can occur */
+static int get_active_agg_info(struct bonding *bond, struct ad_info *ad)
+{
+	int ret;
+
+	read_lock(&bond->lock);
+	ret = bond_3ad_get_active_agg_info(bond, ad);
+	read_unlock(&bond->lock);
+
+	return ret;
+}
 
 /*
  * Show current 802.3ad aggregator ID.
@@ -1333,7 +1344,7 @@ static ssize_t bonding_show_ad_aggregator(struct device *d,
 	if (bond->params.mode == BOND_MODE_8023AD) {
 		struct ad_info ad_info;
 		count = sprintf(buf, "%d\n",
-				(bond_3ad_get_active_agg_info(bond, &ad_info))
+				(get_active_agg_info(bond, &ad_info))
 				?  0 : ad_info.aggregator_id);
 	}
 
@@ -1355,7 +1366,7 @@ static ssize_t bonding_show_ad_num_ports(struct device *d,
 	if (bond->params.mode == BOND_MODE_8023AD) {
 		struct ad_info ad_info;
 		count = sprintf(buf, "%d\n",
-				(bond_3ad_get_active_agg_info(bond, &ad_info))
+				(get_active_agg_info(bond, &ad_info))
 				?  0 : ad_info.ports);
 	}
 
@@ -1377,7 +1388,7 @@ static ssize_t bonding_show_ad_actor_key(struct device *d,
 	if (bond->params.mode == BOND_MODE_8023AD) {
 		struct ad_info ad_info;
 		count = sprintf(buf, "%d\n",
-				(bond_3ad_get_active_agg_info(bond, &ad_info))
+				(get_active_agg_info(bond, &ad_info))
 				?  0 : ad_info.actor_key);
 	}
 
@@ -1399,7 +1410,7 @@ static ssize_t bonding_show_ad_partner_key(struct device *d,
 	if (bond->params.mode == BOND_MODE_8023AD) {
 		struct ad_info ad_info;
 		count = sprintf(buf, "%d\n",
-				(bond_3ad_get_active_agg_info(bond, &ad_info))
+				(get_active_agg_info(bond, &ad_info))
 				?  0 : ad_info.partner_key);
 	}
 
@@ -1420,7 +1431,7 @@ static ssize_t bonding_show_ad_partner_mac(struct device *d,
 
 	if (bond->params.mode == BOND_MODE_8023AD) {
 		struct ad_info ad_info;
-		if (!bond_3ad_get_active_agg_info(bond, &ad_info))
+		if (!get_active_agg_info(bond, &ad_info))
 			count = sprintf(buf, "%pM\n", ad_info.partner_system);
 	}
 
-- 
1.8.1.4

Re: [PATCH 4/4] bonding: fix multiple 3ad mode sysfs race conditions

[Sergei Shtylyov]

Hello.

On 15-05-2013 16:32, Nikolay Aleksandrov wrote:

> When bond_3ad_get_active_agg_info() is used in all show_ad_ functions
> it is not protected against slave manipulation and since it walks over
> the slaves and uses them, this can easily result in NULL pointer
> dereference or use of freed memory.

> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
>   drivers/net/bonding/bond_sysfs.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)

> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index 77ea237..81ef36a 100644
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
[...]
> @@ -1333,7 +1344,7 @@ static ssize_t bonding_show_ad_aggregator(struct device *d,
>   	if (bond->params.mode == BOND_MODE_8023AD) {
>   		struct ad_info ad_info;
>   		count = sprintf(buf, "%d\n",
> -				(bond_3ad_get_active_agg_info(bond, &ad_info))
> +				(get_active_agg_info(bond, &ad_info))
>   				?  0 : ad_info.aggregator_id);
>   	}
>
> @@ -1355,7 +1366,7 @@ static ssize_t bonding_show_ad_num_ports(struct device *d,
>   	if (bond->params.mode == BOND_MODE_8023AD) {
>   		struct ad_info ad_info;
>   		count = sprintf(buf, "%d\n",
> -				(bond_3ad_get_active_agg_info(bond, &ad_info))
> +				(get_active_agg_info(bond, &ad_info))
>   				?  0 : ad_info.ports);
>   	}
>
> @@ -1377,7 +1388,7 @@ static ssize_t bonding_show_ad_actor_key(struct device *d,
>   	if (bond->params.mode == BOND_MODE_8023AD) {
>   		struct ad_info ad_info;
>   		count = sprintf(buf, "%d\n",
> -				(bond_3ad_get_active_agg_info(bond, &ad_info))
> +				(get_active_agg_info(bond, &ad_info))
>   				?  0 : ad_info.actor_key);
>   	}
>
> @@ -1399,7 +1410,7 @@ static ssize_t bonding_show_ad_partner_key(struct device *d,
>   	if (bond->params.mode == BOND_MODE_8023AD) {
>   		struct ad_info ad_info;
>   		count = sprintf(buf, "%d\n",
> -				(bond_3ad_get_active_agg_info(bond, &ad_info))
> +				(get_active_agg_info(bond, &ad_info))
>   				?  0 : ad_info.partner_key);
>   	}

    Perhaps it's time to get rid of the useless parens around function 
call in ?: operator?

WBR, Sergei

Re: [PATCH 4/4] bonding: fix multiple 3ad mode sysfs race conditions

[Nikolay Aleksandrov]

On 05/15/2013 03:53 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 15-05-2013 16:32, Nikolay Aleksandrov wrote:
> 
>> When bond_3ad_get_active_agg_info() is used in all show_ad_ functions
>> it is not protected against slave manipulation and since it walks over
>> the slaves and uses them, this can easily result in NULL pointer
>> dereference or use of freed memory.
> 
>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>> ---
>>   drivers/net/bonding/bond_sysfs.c | 21 ++++++++++++++++-----
>>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
>> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>> index 77ea237..81ef36a 100644
>> --- a/drivers/net/bonding/bond_sysfs.c
>> +++ b/drivers/net/bonding/bond_sysfs.c
> [...]
>> @@ -1333,7 +1344,7 @@ static ssize_t bonding_show_ad_aggregator(struct device *d,
>>       if (bond->params.mode == BOND_MODE_8023AD) {
>>           struct ad_info ad_info;
>>           count = sprintf(buf, "%d\n",
>> -                (bond_3ad_get_active_agg_info(bond, &ad_info))
>> +                (get_active_agg_info(bond, &ad_info))
>>                   ?  0 : ad_info.aggregator_id);
>>       }
>>
>> @@ -1355,7 +1366,7 @@ static ssize_t bonding_show_ad_num_ports(struct device *d,
>>       if (bond->params.mode == BOND_MODE_8023AD) {
>>           struct ad_info ad_info;
>>           count = sprintf(buf, "%d\n",
>> -                (bond_3ad_get_active_agg_info(bond, &ad_info))
>> +                (get_active_agg_info(bond, &ad_info))
>>                   ?  0 : ad_info.ports);
>>       }
>>
>> @@ -1377,7 +1388,7 @@ static ssize_t bonding_show_ad_actor_key(struct device *d,
>>       if (bond->params.mode == BOND_MODE_8023AD) {
>>           struct ad_info ad_info;
>>           count = sprintf(buf, "%d\n",
>> -                (bond_3ad_get_active_agg_info(bond, &ad_info))
>> +                (get_active_agg_info(bond, &ad_info))
>>                   ?  0 : ad_info.actor_key);
>>       }
>>
>> @@ -1399,7 +1410,7 @@ static ssize_t bonding_show_ad_partner_key(struct device
>> *d,
>>       if (bond->params.mode == BOND_MODE_8023AD) {
>>           struct ad_info ad_info;
>>           count = sprintf(buf, "%d\n",
>> -                (bond_3ad_get_active_agg_info(bond, &ad_info))
>> +                (get_active_agg_info(bond, &ad_info))
>>                   ?  0 : ad_info.partner_key);
>>       }
> 
>    Perhaps it's time to get rid of the useless parens around function call in ?:
> operator?
> 
> WBR, Sergei
> 
> 
Perhaps it is :-) but I think to take care of this and other styling problems in
the bonding when I submit the trivial style fix patch which I have in my queue.
I might as well fix this one here if the others require it, should I submit a v2 ?

Cheers,
 Nik

Re: [PATCH 0/4] bonding: race and inconsistency fixes

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Wed, 15 May 2013 14:32:38 +0200

> Hello,
>  In patch 1/4 a race condition while changing the bonding mode through
>  sysfs is fixed. Since no synchronization method is used it can race
>  with different functions resulting in different impacts. RTNL is used
>  to sync with the most important and dangerous events.
>  Patch 2/4 is trivial and improves the debugging output by changing %x
>  format to %pI4 for IPv4 addresses in a few pr_debug() calls.
>  Patch 3/4 fixes an inconsistent arp_targets state where we have 0 entry
>  between (or in the beginning) the valid entries that were obtained which
>  is hard to diagnose otherwise.
>  Patch 4/4 fixes multiple instances of a race condition which is because of
>  calls to bond_3ad_get_active_agg_info without any locking, and since it 
>  traverses the slave list this can easily result in NULL ptr dereference or
>  use of freed memory.

Can a bonding expert please review this series?

Thanks.

Re: [PATCH 4/4] bonding: fix multiple 3ad mode sysfs race conditions

[Jay Vosburgh]

Nikolay Aleksandrov <nikolay@redhat.com> wrote:

>When bond_3ad_get_active_agg_info() is used in all show_ad_ functions
>it is not protected against slave manipulation and since it walks over
>the slaves and uses them, this can easily result in NULL pointer
>dereference or use of freed memory.
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>---
> drivers/net/bonding/bond_sysfs.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index 77ea237..81ef36a 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -1319,6 +1319,17 @@ static ssize_t bonding_show_mii_status(struct device *d,
> }
> static DEVICE_ATTR(mii_status, S_IRUGO, bonding_show_mii_status, NULL);
>
>+/* Wrapper used to hold bond->lock so no slave manipulation can occur */
>+static int get_active_agg_info(struct bonding *bond, struct ad_info *ad)
>+{
>+	int ret;
>+
>+	read_lock(&bond->lock);
>+	ret = bond_3ad_get_active_agg_info(bond, ad);
>+	read_unlock(&bond->lock);
>+
>+	return ret;
>+}

	I think the patch is functionally fine, but the usual
nomenclature for adding a "wrapper" is to have the "functional" piece be
named "__function", and the "wrapper" piece to be just "function".  In
this case, it would be __bond_3ad_get_active_agg_info for the "inside"
function (the current function that is being wrapped), and
bond_3ad_get_active_agg_info for the wrapper.  Yes, this makes for a
different changeset (as some other calls already hold the locks and will
change to the __ version), but the end result is clearer.

	-J

> /*
>  * Show current 802.3ad aggregator ID.
>@@ -1333,7 +1344,7 @@ static ssize_t bonding_show_ad_aggregator(struct device *d,
> 	if (bond->params.mode == BOND_MODE_8023AD) {
> 		struct ad_info ad_info;
> 		count = sprintf(buf, "%d\n",
>-				(bond_3ad_get_active_agg_info(bond, &ad_info))
>+				(get_active_agg_info(bond, &ad_info))
> 				?  0 : ad_info.aggregator_id);
> 	}
>
>@@ -1355,7 +1366,7 @@ static ssize_t bonding_show_ad_num_ports(struct device *d,
> 	if (bond->params.mode == BOND_MODE_8023AD) {
> 		struct ad_info ad_info;
> 		count = sprintf(buf, "%d\n",
>-				(bond_3ad_get_active_agg_info(bond, &ad_info))
>+				(get_active_agg_info(bond, &ad_info))
> 				?  0 : ad_info.ports);
> 	}
>
>@@ -1377,7 +1388,7 @@ static ssize_t bonding_show_ad_actor_key(struct device *d,
> 	if (bond->params.mode == BOND_MODE_8023AD) {
> 		struct ad_info ad_info;
> 		count = sprintf(buf, "%d\n",
>-				(bond_3ad_get_active_agg_info(bond, &ad_info))
>+				(get_active_agg_info(bond, &ad_info))
> 				?  0 : ad_info.actor_key);
> 	}
>
>@@ -1399,7 +1410,7 @@ static ssize_t bonding_show_ad_partner_key(struct device *d,
> 	if (bond->params.mode == BOND_MODE_8023AD) {
> 		struct ad_info ad_info;
> 		count = sprintf(buf, "%d\n",
>-				(bond_3ad_get_active_agg_info(bond, &ad_info))
>+				(get_active_agg_info(bond, &ad_info))
> 				?  0 : ad_info.partner_key);
> 	}
>
>@@ -1420,7 +1431,7 @@ static ssize_t bonding_show_ad_partner_mac(struct device *d,
>
> 	if (bond->params.mode == BOND_MODE_8023AD) {
> 		struct ad_info ad_info;
>-		if (!bond_3ad_get_active_agg_info(bond, &ad_info))
>+		if (!get_active_agg_info(bond, &ad_info))
> 			count = sprintf(buf, "%pM\n", ad_info.partner_system);
> 	}
>
>-- 
>1.8.1.4
>

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 3/4] bonding: arp_ip_count and arp_targets can be wrong

[Jay Vosburgh]

Nikolay Aleksandrov <nikolay@redhat.com> wrote:

>When getting arp_ip_targets if we encounter a bad IP, arp_ip_count still
>gets increased and all the targets after the wrong one will not be probed
>if arp_interval is enabled after that (unless a new IP target is added
>through sysfs) because of the zero entry, in this case reading
>arp_ip_target through sysfs will show valid targets even if there's a
>zero entry.
>Example: 1.2.3.4,4.5.6.7,blah,5.6.7.8
>When retrieving the list from arp_ip_target the output would be:
>1.2.3.4,4.5.6.7,5.6.7.8
>but there will be a 0 entry between 4.5.6.7 and 5.6.7.8. If arp_interval
>is enabled after that 5.6.7.8 will never be checked because of that.

	This patch looks good for the description above.

>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>---
>Question about this one: Do we have to disable arp_interval if we have
>obtained at least 1 valid IP address ? 
>I think this can be addressed in a net-next patch.

	My personal thought is that, ideally, it should be all or none,
i.e., either all of the targets are valid and are accepted as a set, and
if any are wrong, the entire set is rejected.

	That change could break existing installations, although
probably only on embedded or other systems without sysfs; regular
distros generally configure bonding via sysfs.

	If we're not going to do it the right way for fear of
compatibility issues, then I'd just leave it alone.  If compatibility
isn't a concern, then I'd fix it as I described above.

	-J

> drivers/net/bonding/bond_main.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1a0cc13..d6a96cb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4470,7 +4470,7 @@ int bond_parse_parm(const char *buf, const struct bond_parm_tbl *tbl)
>
> static int bond_check_params(struct bond_params *params)
> {
>-	int arp_validate_value, fail_over_mac_value, primary_reselect_value;
>+	int arp_validate_value, fail_over_mac_value, primary_reselect_value, i;
>
> 	/*
> 	 * Convert string parameters.
>@@ -4650,19 +4650,18 @@ static int bond_check_params(struct bond_params *params)
> 		arp_interval = BOND_LINK_ARP_INTERV;
> 	}
>
>-	for (arp_ip_count = 0;
>-	     (arp_ip_count < BOND_MAX_ARP_TARGETS) && arp_ip_target[arp_ip_count];
>-	     arp_ip_count++) {
>+	for (arp_ip_count = 0, i = 0;
>+	     (arp_ip_count < BOND_MAX_ARP_TARGETS) && arp_ip_target[i]; i++) {
> 		/* not complete check, but should be good enough to
> 		   catch mistakes */
>-		__be32 ip = in_aton(arp_ip_target[arp_ip_count]);
>-		if (!isdigit(arp_ip_target[arp_ip_count][0]) ||
>-		    ip == 0 || ip == htonl(INADDR_BROADCAST)) {
>+		__be32 ip = in_aton(arp_ip_target[i]);
>+		if (!isdigit(arp_ip_target[i][0]) || ip == 0 ||
>+		    ip == htonl(INADDR_BROADCAST)) {
> 			pr_warning("Warning: bad arp_ip_target module parameter (%s), ARP monitoring will not be performed\n",
>-				   arp_ip_target[arp_ip_count]);
>+				   arp_ip_target[i]);
> 			arp_interval = 0;
> 		} else {
>-			arp_target[arp_ip_count] = ip;
>+			arp_target[arp_ip_count++] = ip;
> 		}
> 	}
>
>@@ -4696,8 +4695,6 @@ static int bond_check_params(struct bond_params *params)
> 	if (miimon) {
> 		pr_info("MII link monitoring set to %d ms\n", miimon);
> 	} else if (arp_interval) {
>-		int i;
>-
> 		pr_info("ARP monitoring set to %d ms, validate %s, with %d target(s):",
> 			arp_interval,
> 			arp_validate_tbl[arp_validate_value].modename,
>-- 
>1.8.1.4

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 4/4] bonding: fix multiple 3ad mode sysfs race conditions

[Nikolay Aleksandrov]

On 17/05/13 18:59, Jay Vosburgh wrote:
>
> 	I think the patch is functionally fine, but the usual
> nomenclature for adding a "wrapper" is to have the "functional" piece be
> named "__function", and the "wrapper" piece to be just "function".  In
> this case, it would be __bond_3ad_get_active_agg_info for the "inside"
> function (the current function that is being wrapped), and
> bond_3ad_get_active_agg_info for the wrapper.  Yes, this makes for a
> different changeset (as some other calls already hold the locks and will
> change to the __ version), but the end result is clearer.
> 
> 	-J
> 

Jay thank you for the review, I've done the necessary changes to the
function names and moved the wrapper to bond_3ad.c, but had to make them
both global as you said since they're both used at different places.
I'll re-post v2 tomorrow after I do some testing since it's 4 am here
now :-)

Cheers,
 Nik