Re: [Bugme-new] [Bug 13006] New: Bonding: Holes in arp_ip_target list cause ARP replies to be ignored

Re: [Bugme-new] [Bug 13006] New: Bonding: Holes in arp_ip_target list cause ARP replies to be ignored

[Andrew Morton]

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

On Fri, 3 Apr 2009 20:17:37 GMT
bugzilla-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=13006
> 
>                URL: www.astutenetworks.com
>            Summary: Bonding: Holes in arp_ip_target list cause ARP replies
>                     to be ignored
>            Product: Drivers
>            Version: 2.5
>     Kernel Version: 2.6.25
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: Network
>         AssignedTo: drivers_network@kernel-bugs.osdl.org
>         ReportedBy: steve@astutenetworks.com
>         Regression: No
> 
> 
> I know there have been changes to bonding driver since 2.6.25, but
> this bug is still there.
> 
> To Recreate:
> 
> Set up a bond as follows:
>   mode active-backup
>   arp_interval 1000
>   arp_ip_target 1.1.1.1 2.2.2.2 (address 2 points to a reachable target)
> 
> /sys/class/net/bond0/bonding/active_slave contains an interface name
> 
> Now delete the first target (cat -1.1.1.1 >arp_ip_target)
> 
> The first target is deleted, but active_slave is now empty.
> 
> The proglem is in line 2611 of bond_main.c, in bond_validate_arp()
> 
>     for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) {
> 
> This is the loop that searches the arp_ip_targets array for a match
> with an incomming ARP.  The problem is that when we deleted the first
> entry, it was set to zero.  This "for" loop exits before comparing with
> the remaining entries in targets[].
> 
> One fix is to delete the " && targets[i]" from the termination conditions.
> Then the loop will always search the full list.
> 
> A better fix is to make sure there are no empty holes in the target[]
> array, beceause there may other code which expects no holes. 
> 
> This can be done by modifying this loop in bond_sysfs.c at line 792,
> which is where entries are deleted in bonding_store_arp_targets().
> This change deletes the hole just created by removing an entry.
> 
>         for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
>             if (targets[i] == newtarget) {
>                 printk(KERN_INFO DRV_NAME
>                        ": %s: removing ARP target %d.%d.%d.%d.\n",
>                        bond->dev->name, NIPQUAD(newtarget));
>                 targets[i] = 0;
>                 done = 1;
> 
>                 // Move any entries after this one into the newly
>                 // empty slot, so there are no holes.  Be careful not to access
>                 // more than BOND_MAX_ARP_TARGETS in the array.
>                 for (j = i; (j < (BOND_MAX_ARP_TARGETS-1)) && targets[j + 1];
> j++) {
>                     targets[j] = targets[j + 1];
>                     targets[j + 1] = 0;
>                 }
>             }
>         }1
> 
> 
> Here is the output of ver_linux:
> 
>  Linux caspianB012a 2.6.25-astute-v15 #19 SMP Fri Apr 3 09:32:28 PDT 2009 i686
> GNU/Linux
>  Gnu C                  18:
>  /stim2/steve/work/acp/host/caspian/pd/kernel/scripts/ver_linux: line 24: ld:
> command not found
>  binutils               
>  util-linux             2.13
>  mount                  2.13
>  module-init-tools      3.2-pre1
>  e2fsprogs              1.40
>  Linux C Library        2.5.90
>  /stim2/steve/work/acp/host/caspian/pd/kernel/scripts/ver_linux: line 69: ldd:
> command not found
>  Procps                 3.2.7
>  Net-tools              1.60
>  Kbd                    82:
>  Sh-utils               5.97
>  udev                   105
>  Modules Loaded         nfs lockd sunrpc pegasus iSCSITarget ScsiGatewayShim
> ScsiGateway an2000_l2pthru amod linux_user_bde linux_kernel_bde
> astute_hostapi_mod megaraid_sas an2k afpga e1000e ipmi_poweroff ipmi_watchdog
> ipmi_devintf ipmi_si ipmi_msghandler

[PATCH] Bonding: fix zero address hole bug in arp_ip_target list

[Brian Haley]

Fix a zero address hole bug in the bonding arp_ip_target list
that was causing the bond to ignore ARP replies (bugz 13006).
Instead of just setting the array entry to zero, we now
copy any additional entries down one slot, putting the
zero entry at the end.  With this change we can now have
all the loops that walk the array stop when they hit a zero
since there will be no addresses after it.

Signed-off-by: Brian Haley <brian.haley@hp.com>
---
 Documentation/networking/bonding.txt |    2 +-
 drivers/net/bonding/bond_main.c      |    5 ++---
 drivers/net/bonding/bond_sysfs.c     |   14 ++++++++------
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index 5ede747..0876275 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -1242,7 +1242,7 @@ monitoring is enabled, and vice-versa.
 To add ARP targets:
 # echo +192.168.0.100 > /sys/class/net/bond0/bonding/arp_ip_target
 # echo +192.168.0.101 > /sys/class/net/bond0/bonding/arp_ip_target
-	NOTE:  up to 10 target addresses may be specified.
+	NOTE:  up to 16 target addresses may be specified.
 
 To remove an ARP target:
 # echo -192.168.0.100 > /sys/class/net/bond0/bonding/arp_ip_target
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 99610f3..63369b6 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2570,7 +2570,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 
 	for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
 		if (!targets[i])
-			continue;
+			break;
 		pr_debug("basa: target %x\n", targets[i]);
 		if (list_empty(&bond->vlan_list)) {
 			pr_debug("basa: empty vlan: arp_send\n");
@@ -2677,7 +2677,6 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
 	int i;
 	__be32 *targets = bond->params.arp_targets;
 
-	targets = bond->params.arp_targets;
 	for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) {
 		pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip) %d\n",
 			&sip, &tip, i, &targets[i], bond_has_this_ip(bond, tip));
@@ -3303,7 +3302,7 @@ static void bond_info_show_master(struct seq_file *seq)
 
 		for(i = 0; (i < BOND_MAX_ARP_TARGETS) ;i++) {
 			if (!bond->params.arp_targets[i])
-				continue;
+				break;
 			if (printed)
 				seq_printf(seq, ",");
 			seq_printf(seq, " %pI4", &bond->params.arp_targets[i]);
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 18cf478..d287315 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -684,17 +684,15 @@ static ssize_t bonding_store_arp_targets(struct device *d,
 			goto out;
 		}
 		/* look for an empty slot to put the target in, and check for dupes */
-		for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
+		for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) {
 			if (targets[i] == newtarget) { /* duplicate */
 				printk(KERN_ERR DRV_NAME
 				       ": %s: ARP target %pI4 is already present\n",
 				       bond->dev->name, &newtarget);
-				if (done)
-					targets[i] = 0;
 				ret = -EINVAL;
 				goto out;
 			}
-			if (targets[i] == 0 && !done) {
+			if (targets[i] == 0) {
 				printk(KERN_INFO DRV_NAME
 				       ": %s: adding ARP target %pI4.\n",
 				       bond->dev->name, &newtarget);
@@ -720,12 +718,16 @@ static ssize_t bonding_store_arp_targets(struct device *d,
 			goto out;
 		}
 
-		for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
+		for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) {
 			if (targets[i] == newtarget) {
+				int j;
 				printk(KERN_INFO DRV_NAME
 				       ": %s: removing ARP target %pI4.\n",
 				       bond->dev->name, &newtarget);
-				targets[i] = 0;
+				for (j = i; (j < (BOND_MAX_ARP_TARGETS-1)) && targets[j+1]; j++)
+					targets[j] = targets[j+1];
+
+				targets[j] = 0;
 				done = 1;
 			}
 		}
-- 
1.5.4.3

Re: [PATCH] Bonding: fix zero address hole bug in arp_ip_target list

[Jay Vosburgh]

Brian Haley <brian.haley@hp.com> wrote:

>Fix a zero address hole bug in the bonding arp_ip_target list
>that was causing the bond to ignore ARP replies (bugz 13006).
>Instead of just setting the array entry to zero, we now
>copy any additional entries down one slot, putting the
>zero entry at the end.  With this change we can now have
>all the loops that walk the array stop when they hit a zero
>since there will be no addresses after it.
>
>Signed-off-by: Brian Haley <brian.haley@hp.com>

	This all looks reasonable to me.  This patch appears to be an
extension of some code (not as a patch) provided in bugzilla 13006 by
Steve Howard <steve@astutenetworks.com>, so I suspect he should sign off
on this as well, or at the very least be credited in the log; I'd write
that as:

	Changes are based in part on code fragment provided in kernel
bugzilla 13006 by Steve Howard <steve@astutenetworks.com>.

	This can go to -stable, too.

	-J

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

>---
> Documentation/networking/bonding.txt |    2 +-
> drivers/net/bonding/bond_main.c      |    5 ++---
> drivers/net/bonding/bond_sysfs.c     |   14 ++++++++------
> 3 files changed, 11 insertions(+), 10 deletions(-)
>
>diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
>index 5ede747..0876275 100644
>--- a/Documentation/networking/bonding.txt
>+++ b/Documentation/networking/bonding.txt
>@@ -1242,7 +1242,7 @@ monitoring is enabled, and vice-versa.
> To add ARP targets:
> # echo +192.168.0.100 > /sys/class/net/bond0/bonding/arp_ip_target
> # echo +192.168.0.101 > /sys/class/net/bond0/bonding/arp_ip_target
>-	NOTE:  up to 10 target addresses may be specified.
>+	NOTE:  up to 16 target addresses may be specified.
>
> To remove an ARP target:
> # echo -192.168.0.100 > /sys/class/net/bond0/bonding/arp_ip_target
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 99610f3..63369b6 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2570,7 +2570,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>
> 	for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
> 		if (!targets[i])
>-			continue;
>+			break;
> 		pr_debug("basa: target %x\n", targets[i]);
> 		if (list_empty(&bond->vlan_list)) {
> 			pr_debug("basa: empty vlan: arp_send\n");
>@@ -2677,7 +2677,6 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
> 	int i;
> 	__be32 *targets = bond->params.arp_targets;
>
>-	targets = bond->params.arp_targets;
> 	for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) {
> 		pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip) %d\n",
> 			&sip, &tip, i, &targets[i], bond_has_this_ip(bond, tip));
>@@ -3303,7 +3302,7 @@ static void bond_info_show_master(struct seq_file *seq)
>
> 		for(i = 0; (i < BOND_MAX_ARP_TARGETS) ;i++) {
> 			if (!bond->params.arp_targets[i])
>-				continue;
>+				break;
> 			if (printed)
> 				seq_printf(seq, ",");
> 			seq_printf(seq, " %pI4", &bond->params.arp_targets[i]);
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index 18cf478..d287315 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -684,17 +684,15 @@ static ssize_t bonding_store_arp_targets(struct device *d,
> 			goto out;
> 		}
> 		/* look for an empty slot to put the target in, and check for dupes */
>-		for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
>+		for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) {
> 			if (targets[i] == newtarget) { /* duplicate */
> 				printk(KERN_ERR DRV_NAME
> 				       ": %s: ARP target %pI4 is already present\n",
> 				       bond->dev->name, &newtarget);
>-				if (done)
>-					targets[i] = 0;
> 				ret = -EINVAL;
> 				goto out;
> 			}
>-			if (targets[i] == 0 && !done) {
>+			if (targets[i] == 0) {
> 				printk(KERN_INFO DRV_NAME
> 				       ": %s: adding ARP target %pI4.\n",
> 				       bond->dev->name, &newtarget);
>@@ -720,12 +718,16 @@ static ssize_t bonding_store_arp_targets(struct device *d,
> 			goto out;
> 		}
>
>-		for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
>+		for (i = 0; (i < BOND_MAX_ARP_TARGETS) && !done; i++) {
> 			if (targets[i] == newtarget) {
>+				int j;
> 				printk(KERN_INFO DRV_NAME
> 				       ": %s: removing ARP target %pI4.\n",
> 				       bond->dev->name, &newtarget);
>-				targets[i] = 0;
>+				for (j = i; (j < (BOND_MAX_ARP_TARGETS-1)) && targets[j+1]; j++)
>+					targets[j] = targets[j+1];
>+
>+				targets[j] = 0;
> 				done = 1;
> 			}
> 		}
>-- 
>1.5.4.3
>
>--
>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

Re: [PATCH] Bonding: fix zero address hole bug in arp_ip_target list

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Fri, 10 Apr 2009 23:51:19 -0700

> Brian Haley <brian.haley@hp.com> wrote:
> 
>>Fix a zero address hole bug in the bonding arp_ip_target list
>>that was causing the bond to ignore ARP replies (bugz 13006).
>>Instead of just setting the array entry to zero, we now
>>copy any additional entries down one slot, putting the
>>zero entry at the end.  With this change we can now have
>>all the loops that walk the array stop when they hit a zero
>>since there will be no addresses after it.
>>
>>Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> 	This all looks reasonable to me.  This patch appears to be an
> extension of some code (not as a patch) provided in bugzilla 13006 by
> Steve Howard <steve@astutenetworks.com>, so I suspect he should sign off
> on this as well, or at the very least be credited in the log; I'd write
> that as:
> 
> 	Changes are based in part on code fragment provided in kernel
> bugzilla 13006 by Steve Howard <steve@astutenetworks.com>.
> 
> 	This can go to -stable, too.
> 
> 	-J
> 
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

Indeed, please fix up the attribution and I will apply this and
queue up for -stable.