[PATCH] bonding using arp_ip_target may stay down with active path

[PATCH] bonding using arp_ip_target may stay down with active path

[Eric Paris]

The bonding module may get into a state in which an active path to the
network exists through at least one slave device but the bond remains
down forever.  This situation exists using the bonding options  mode=1
arp_interval=500 arp_ip_target=10.10.10.5.  mode=1 is the active/passive
bonding mode.  We determine link status using the reachability of other
network devices determined by if they respond to arp requests.

Reproducer:
The reproducer is not simple.  Easiest with 3 computers and two
crossover cables.  Configure one computer with bonding and each of the
other computers to have an address in the arp_ip_target entries for the
first machine.  In this way if both single nic computers are up bonding
should believe either of the slave interfaces are valid since each can
reach one of the arp_ip_target entries.  Shutdown the interface on the
single nic computer connected to eth0.  The bond should fail over to
eth1.  Shut down the interface connected to eth1.  The bond should
decide both the eth1 slave and the bond as a whole is down (it cannot
contact either of the arp_ip_target entries).  Run tcpdump on both of
the single nic machines and see that only the machine connected to eth0
is receiving arp requests.  Bring back up the interface connected to
eth1.  At this point we have a "valid" connection since eth1 can talk to
one of the arp targets.  But we are only sending arp requests on eth0
(verify with tcpdump)

The Problem:
The problem is in bond_activebackup_arp_mon where we say (in
bond_main.c)

if (!slave) {
        if (!bond->current_arp_slave) {
                bond->current_arp_slave = bond->first_slave;
        }
        if (bond->current_arp_slave) {
                bond_set_slave_inactive_flags(bond->current_arp_slave);

                /* search for next candidate */
                bond_for_each_slave_from(bond, slave, i,
bond->current_arp_slave) {
                        if (IS_UP(slave->dev)) {
                                slave->link = BOND_LINK_BACK;
                                bond_set_slave_active_flags(slave);
                                bond_arp_send_all(bond, slave);
                                slave->jiffies = jiffies;
                                bond->current_arp_slave = slave;
                                break;
                        }

What happens is that we set the current_arp_slave to the first interface
in the bond, bond->current_arp_slave = bond->first_slave; (in our case
eth0) and then if that slave IS_UP we send the arp requests.  IS_UP
checks only physical device information, so the NIC is up if it has
link.

We can make it fail over by pulling the cable, in which case we are !
IS_UP(eth0) and so the bond_for_each_slave_from loop continues to
IS_UP(eth1) and it finds eth1 is physically up.  It then sends the arp
requests on eth1, gets a response from the connected single nic machine
and marks the bond as a whole as up.

The patch below instead just uses bond_for_each_slave_from(bond, slave,
i, bond->current_arp_slave->next) which means that each time we enter
bond_activebackup_arp_mon without a bond->current_active_slave we will
try an interface (actually starting with the second in the list) and if
that interface does not get success the next go round
bond->current_arp_slave will be the next in the list.  This way we will
try all interfaces in turn.  I unconditionally use
current_arp_slave->next since it is a circular list and should always
have a next.

The patch below has been tested by me and appears to fix the problem.
All of the failover tests I performed seem to work including pulling
cables and stopping responses from the arp_ip_target entries.  

--- linux-2.6.11/drivers/net/bonding/bond_main.c.orig   2005-05-12 12:22:52.000000000 -0400
+++ linux-2.6.11/drivers/net/bonding/bond_main.c        2005-05-12 15:13:53.000000000 -0400
@@ -3046,7 +3046,7 @@ static void bond_activebackup_arp_mon(st
                        bond_set_slave_inactive_flags(bond->current_arp_slave);

                        /* search for next candidate */
-                       bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave) {
+                       bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave->next) {
                                if (IS_UP(slave->dev)) {
                                        slave->link = BOND_LINK_BACK;
                                        bond_set_slave_active_flags(slave);

Re: [PATCH] bonding using arp_ip_target may stay down with active path

[Jay Vosburgh]

Eric Paris <eparis@parisplace.org> wrote:

>[...]  Bring back up the interface connected to
>eth1.  At this point we have a "valid" connection since eth1 can talk to
>one of the arp targets.  But we are only sending arp requests on eth0
>(verify with tcpdump)

	The trick is to have a situation with a partitioned network and
a failure such that the device still has link, but does not respond to
the ARP queries.  That's not an unreasonable failure if there's a switch
in each path to the arp_ip_target peers (which is how I set it up
locally).

>The patch below has been tested by me and appears to fix the problem.
>All of the failover tests I performed seem to work including pulling
>cables and stopping responses from the arp_ip_target entries.  

	The patch looks good to me, also (although I made the change by
hand instead of via patch).

	-J

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


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


--- linux-2.6.11/drivers/net/bonding/bond_main.c.orig   2005-05-12 12:22:52.000000000 -0400
+++ linux-2.6.11/drivers/net/bonding/bond_main.c        2005-05-12 15:13:53.000000000 -0400
@@ -3046,7 +3046,7 @@ static void bond_activebackup_arp_mon(st
                        bond_set_slave_inactive_flags(bond->current_arp_slave);

                        /* search for next candidate */
-                       bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave) {
+                       bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave->next) {
                                if (IS_UP(slave->dev)) {
                                        slave->link = BOND_LINK_BACK;
                                        bond_set_slave_active_flags(slave);

Re: [PATCH] bonding using arp_ip_target may stay down with active path

[David S. Miller]

Patch doesn't apply, tabs turned into spaces by your
email client.

Re: [PATCH] bonding using arp_ip_target may stay down with active path

[Jay Vosburgh]

David S. Miller <davem@davemloft.net> wrote:
>Patch doesn't apply, tabs turned into spaces by your
>email client.

	As penance for test applying trivial patches by hand, here's a
proper version.

	-J

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


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

diff -urN linux-2.6.11/drivers/net/bonding/bond_main.c linux-2.6.11-fix/drivers/net/bonding/bond_main.c
--- linux-2.6.11/drivers/net/bonding/bond_main.c	2005-05-23 14:07:37.000000000 -0700
+++ linux-2.6.11-fix/drivers/net/bonding/bond_main.c	2005-05-23 14:08:13.000000000 -0700
@@ -3046,7 +3046,7 @@
 			bond_set_slave_inactive_flags(bond->current_arp_slave);
 
 			/* search for next candidate */
-			bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave) {
+			bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave->next) {
 				if (IS_UP(slave->dev)) {
 					slave->link = BOND_LINK_BACK;
 					bond_set_slave_active_flags(slave);

Re: [PATCH] bonding using arp_ip_target may stay down with active path

[Eric Paris]

>On Mon, 2005-05-23 at 14:21 -0700, Jay Vosburgh wrote:
David S. Miller <davem@davemloft.net> wrote:
>Patch doesn't apply, tabs turned into spaces by your
>email client.
>
>	As penance for test applying trivial patches by hand, here's a
>proper version.
>
>	-J
>
>---
>	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

I guess I should be in the signed off list as the original author.
sorry I'm an idiot and copied and pasted my original message (to the
bonding-devel list) which screwed up the spacing.

Signed-off-by: Eric Paris <eparis@parisplace.org>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

diff -urN linux-2.6.11/drivers/net/bonding/bond_main.c linux-2.6.11-fix/drivers/net/bonding/bond_main.c
--- linux-2.6.11/drivers/net/bonding/bond_main.c	2005-05-23 14:07:37.000000000 -0700
+++ linux-2.6.11-fix/drivers/net/bonding/bond_main.c	2005-05-23 14:08:13.000000000 -0700
@@ -3046,7 +3046,7 @@
 			bond_set_slave_inactive_flags(bond->current_arp_slave);
 
 			/* search for next candidate */
-			bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave) {
+			bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave->next) {
 				if (IS_UP(slave->dev)) {
 					slave->link = BOND_LINK_BACK;
 					bond_set_slave_active_flags(slave);

Re: [PATCH] bonding using arp_ip_target may stay down with active path

[Eric Paris]

It ate my tabs again.  This mailer is going out the window.

-Eric

On Tue, 2005-05-24 at 14:26 -0400, Eric Paris wrote:
> >On Mon, 2005-05-23 at 14:21 -0700, Jay Vosburgh wrote:
> David S. Miller <davem@davemloft.net> wrote:
> >Patch doesn't apply, tabs turned into spaces by your
> >email client.
> >
> >	As penance for test applying trivial patches by hand, here's a
> >proper version.
> >
> >	-J
> >
> >---
> >	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
> 
> I guess I should be in the signed off list as the original author.
> sorry I'm an idiot and copied and pasted my original message (to the
> bonding-devel list) which screwed up the spacing.
> 
> Signed-off-by: Eric Paris <eparis@parisplace.org>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
> 
> diff -urN linux-2.6.11/drivers/net/bonding/bond_main.c linux-2.6.11-fix/drivers/net/bonding/bond_main.c
> --- linux-2.6.11/drivers/net/bonding/bond_main.c	2005-05-23 14:07:37.000000000 -0700
> +++ linux-2.6.11-fix/drivers/net/bonding/bond_main.c	2005-05-23 14:08:13.000000000 -0700
> @@ -3046,7 +3046,7 @@
>  			bond_set_slave_inactive_flags(bond->current_arp_slave);
>  
>  			/* search for next candidate */
> -			bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave) {
> +			bond_for_each_slave_from(bond, slave, i, bond->current_arp_slave->next) {
>  				if (IS_UP(slave->dev)) {
>  					slave->link = BOND_LINK_BACK;
>  					bond_set_slave_active_flags(slave);
> 

Re: [PATCH] bonding using arp_ip_target may stay down with active path

[David S. Miller]

From: Eric Paris <eparis@parisplace.org>
Date: Tue, 24 May 2005 14:31:02 -0400

> It ate my tabs again.  This mailer is going out the window.

This is why I personally stick to Emacs/MEW and Sylpheed.

I tried Evolution for a while long ago, but stopped the first
time it refused to read in my entire mbox because it failed to
parse a multi-byte character in _one_ of the emails contained
within.

I'm sure that's fixed, but it's very much a non-ascii-text
mailer by default, so you have to pluck with it's configuration
a little bit if you're sending patches and code around a lot.

Re: [PATCH] bonding using arp_ip_target may stay down with active path

[David S. Miller]

Fixed patch applied, thanks a lot Jay.