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

[PATCH v2 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.

 v2 is to address a minor style fix, to make the wrapper
 global and move it to bond_3ad.c, and also to fix the naming of the
 functions, all in patch 4/4.

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_3ad.c    | 21 +++++++++++++++++----
 drivers/net/bonding/bond_3ad.h    |  2 ++
 drivers/net/bonding/bond_main.c   | 25 +++++++++++--------------
 drivers/net/bonding/bond_procfs.c |  2 +-
 drivers/net/bonding/bond_sysfs.c  | 13 ++++++++-----
 5 files changed, 39 insertions(+), 24 deletions(-)

-- 
1.8.1.4

[PATCH v2 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 v2 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 v2 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>
---
 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 v2 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. Both the new wrapper and the
internal function are exported to the bonding as they're needed in
different places.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: Make the wrapper global and move it to bond_3ad.c, also fix the naming
    of the functions, and remove the extra brackets.

 drivers/net/bonding/bond_3ad.c    | 21 +++++++++++++++++----
 drivers/net/bonding/bond_3ad.h    |  2 ++
 drivers/net/bonding/bond_procfs.c |  2 +-
 drivers/net/bonding/bond_sysfs.c  |  9 ++++-----
 4 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index fc58d11..390061d 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2360,14 +2360,15 @@ int bond_3ad_set_carrier(struct bonding *bond)
 }
 
 /**
- * bond_3ad_get_active_agg_info - get information of the active aggregator
+ * __bond_3ad_get_active_agg_info - get information of the active aggregator
  * @bond: bonding struct to work on
  * @ad_info: ad_info struct to fill with the bond's info
  *
  * Returns:   0 on success
  *          < 0 on error
  */
-int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
+int __bond_3ad_get_active_agg_info(struct bonding *bond,
+				   struct ad_info *ad_info)
 {
 	struct aggregator *aggregator = NULL;
 	struct port *port;
@@ -2391,6 +2392,18 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
 	return -1;
 }
 
+/* Wrapper used to hold bond->lock so no slave manipulation can occur */
+int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
+{
+	int ret;
+
+	read_lock(&bond->lock);
+	ret = __bond_3ad_get_active_agg_info(bond, ad_info);
+	read_unlock(&bond->lock);
+
+	return ret;
+}
+
 int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 {
 	struct slave *slave, *start_at;
@@ -2402,8 +2415,8 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 	struct ad_info ad_info;
 	int res = 1;
 
-	if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
-		pr_debug("%s: Error: bond_3ad_get_active_agg_info failed\n",
+	if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
+		pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
 			 dev->name);
 		goto out;
 	}
diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
index 0cfaa4a..5d91ad0 100644
--- a/drivers/net/bonding/bond_3ad.h
+++ b/drivers/net/bonding/bond_3ad.h
@@ -273,6 +273,8 @@ void bond_3ad_adapter_speed_changed(struct slave *slave);
 void bond_3ad_adapter_duplex_changed(struct slave *slave);
 void bond_3ad_handle_link_change(struct slave *slave, char link);
 int  bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
+int  __bond_3ad_get_active_agg_info(struct bonding *bond,
+				    struct ad_info *ad_info);
 int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
 int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
 			 struct slave *slave);
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index 94d06f1..4060d41 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -130,7 +130,7 @@ static void bond_info_show_master(struct seq_file *seq)
 		seq_printf(seq, "Aggregator selection policy (ad_select): %s\n",
 			   ad_select_tbl[bond->params.ad_select].modename);
 
-		if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
+		if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
 			seq_printf(seq, "bond %s has no active aggregator\n",
 				   bond->dev->name);
 		} else {
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 77ea237..d7434e0 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1319,7 +1319,6 @@ static ssize_t bonding_show_mii_status(struct device *d,
 }
 static DEVICE_ATTR(mii_status, S_IRUGO, bonding_show_mii_status, NULL);
 
-
 /*
  * Show current 802.3ad aggregator ID.
  */
@@ -1333,7 +1332,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))
+				bond_3ad_get_active_agg_info(bond, &ad_info)
 				?  0 : ad_info.aggregator_id);
 	}
 
@@ -1355,7 +1354,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))
+				bond_3ad_get_active_agg_info(bond, &ad_info)
 				?  0 : ad_info.ports);
 	}
 
@@ -1377,7 +1376,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))
+				bond_3ad_get_active_agg_info(bond, &ad_info)
 				?  0 : ad_info.actor_key);
 	}
 
@@ -1399,7 +1398,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))
+				bond_3ad_get_active_agg_info(bond, &ad_info)
 				?  0 : ad_info.partner_key);
 	}
 
-- 
1.8.1.4

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

[Sergei Shtylyov]

Hello.

On 18-05-2013 15:18, Nikolay Aleksandrov 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.

> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
>   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
[...]
> @@ -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]);

    Empty line wouldn't hurt here, after declaration.

> +		if (!isdigit(arp_ip_target[i][0]) || ip == 0 ||
> +		    ip == htonl(INADDR_BROADCAST)) {

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

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Sat, 18 May 2013 13:18:27 +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.
> 
>  v2 is to address a minor style fix, to make the wrapper
>  global and move it to bond_3ad.c, and also to fix the naming of the
>  functions, all in patch 4/4.

Series applied, thanks.