Skip to content

Commit

Permalink
bonding: fix state transition issue in link monitoring
Browse files Browse the repository at this point in the history 
Since de77ecd ("bonding: improve link-status update in
mii-monitoring"), the bonding driver has utilized two separate variables
to indicate the next link state a particular slave should transition to.
Each is used to communicate to a different portion of the link state
change commit logic; one to the bond_miimon_commit function itself, and
another to the state transition logic.

	Unfortunately, the two variables can become unsynchronized,
resulting in incorrect link state transitions within bonding.  This can
cause slaves to become stuck in an incorrect link state until a
subsequent carrier state transition.

	The issue occurs when a special case in bond_slave_netdev_event
sets slave->link directly to BOND_LINK_FAIL.  On the next pass through
bond_miimon_inspect after the slave goes carrier up, the BOND_LINK_FAIL
case will set the proposed next state (link_new_state) to BOND_LINK_UP,
but the new_link to BOND_LINK_DOWN.  The setting of the final link state
from new_link comes after that from link_new_state, and so the slave
will end up incorrectly in _DOWN state.

	Resolve this by combining the two variables into one.

Reported-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
Reported-by: Sha Zhang <zhangsha.zhang@huawei.com>
Cc: Mahesh Bandewar <maheshb@google.com>
Fixes: de77ecd ("bonding: improve link-status update in mii-monitoring")
Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Jay Vosburgh authored and David S. Miller committed Nov 6, 2019
1 parent 41de23e commit 1899bb3
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 24 deletions.
44 changes: 22 additions & 22 deletions drivers/net/bonding/bond_main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2083,8 +2083,7 @@ static int bond_miimon_inspect(struct bonding *bond)
ignore_updelay = !rcu_dereference(bond->curr_active_slave);

bond_for_each_slave_rcu(bond, slave, iter) {
slave->new_link = BOND_LINK_NOCHANGE;
slave->link_new_state = slave->link;
bond_propose_link_state(slave, BOND_LINK_NOCHANGE);

link_state = bond_check_dev_link(bond, slave->dev, 0);

Expand Down Expand Up @@ -2118,7 +2117,7 @@ static int bond_miimon_inspect(struct bonding *bond)
}

if (slave->delay <= 0) {
slave->new_link = BOND_LINK_DOWN;
bond_propose_link_state(slave, BOND_LINK_DOWN);
commit++;
continue;
}
Expand Down Expand Up @@ -2155,7 +2154,7 @@ static int bond_miimon_inspect(struct bonding *bond)
slave->delay = 0;

if (slave->delay <= 0) {
slave->new_link = BOND_LINK_UP;
bond_propose_link_state(slave, BOND_LINK_UP);
commit++;
ignore_updelay = false;
continue;
Expand Down Expand Up @@ -2193,7 +2192,7 @@ static void bond_miimon_commit(struct bonding *bond)
struct slave *slave, *primary;

bond_for_each_slave(bond, slave, iter) {
switch (slave->new_link) {
switch (slave->link_new_state) {
case BOND_LINK_NOCHANGE:
/* For 802.3ad mode, check current slave speed and
* duplex again in case its port was disabled after
Expand Down Expand Up @@ -2265,8 +2264,8 @@ static void bond_miimon_commit(struct bonding *bond)

default:
slave_err(bond->dev, slave->dev, "invalid new link %d on slave\n",
slave->new_link);
slave->new_link = BOND_LINK_NOCHANGE;
slave->link_new_state);
bond_propose_link_state(slave, BOND_LINK_NOCHANGE);

continue;
}
Expand Down Expand Up @@ -2674,13 +2673,13 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
bond_for_each_slave_rcu(bond, slave, iter) {
unsigned long trans_start = dev_trans_start(slave->dev);

slave->new_link = BOND_LINK_NOCHANGE;
bond_propose_link_state(slave, BOND_LINK_NOCHANGE);

if (slave->link != BOND_LINK_UP) {
if (bond_time_in_interval(bond, trans_start, 1) &&
bond_time_in_interval(bond, slave->last_rx, 1)) {

slave->new_link = BOND_LINK_UP;
bond_propose_link_state(slave, BOND_LINK_UP);
slave_state_changed = 1;

/* primary_slave has no meaning in round-robin
Expand All @@ -2705,7 +2704,7 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
if (!bond_time_in_interval(bond, trans_start, 2) ||
!bond_time_in_interval(bond, slave->last_rx, 2)) {

slave->new_link = BOND_LINK_DOWN;
bond_propose_link_state(slave, BOND_LINK_DOWN);
slave_state_changed = 1;

if (slave->link_failure_count < UINT_MAX)
Expand Down Expand Up @@ -2736,8 +2735,8 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
goto re_arm;

bond_for_each_slave(bond, slave, iter) {
if (slave->new_link != BOND_LINK_NOCHANGE)
slave->link = slave->new_link;
if (slave->link_new_state != BOND_LINK_NOCHANGE)
slave->link = slave->link_new_state;
}

if (slave_state_changed) {
Expand All @@ -2760,9 +2759,9 @@ static void bond_loadbalance_arp_mon(struct bonding *bond)
}

/* Called to inspect slaves for active-backup mode ARP monitor link state
* changes. Sets new_link in slaves to specify what action should take
* place for the slave. Returns 0 if no changes are found, >0 if changes
* to link states must be committed.
* changes. Sets proposed link state in slaves to specify what action
* should take place for the slave. Returns 0 if no changes are found, >0
* if changes to link states must be committed.
*
* Called with rcu_read_lock held.
*/
Expand All @@ -2774,12 +2773,12 @@ static int bond_ab_arp_inspect(struct bonding *bond)
int commit = 0;

bond_for_each_slave_rcu(bond, slave, iter) {
slave->new_link = BOND_LINK_NOCHANGE;
bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
last_rx = slave_last_rx(bond, slave);

if (slave->link != BOND_LINK_UP) {
if (bond_time_in_interval(bond, last_rx, 1)) {
slave->new_link = BOND_LINK_UP;
bond_propose_link_state(slave, BOND_LINK_UP);
commit++;
}
continue;
Expand Down Expand Up @@ -2807,7 +2806,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
if (!bond_is_active_slave(slave) &&
!rcu_access_pointer(bond->current_arp_slave) &&
!bond_time_in_interval(bond, last_rx, 3)) {
slave->new_link = BOND_LINK_DOWN;
bond_propose_link_state(slave, BOND_LINK_DOWN);
commit++;
}

Expand All @@ -2820,7 +2819,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
if (bond_is_active_slave(slave) &&
(!bond_time_in_interval(bond, trans_start, 2) ||
!bond_time_in_interval(bond, last_rx, 2))) {
slave->new_link = BOND_LINK_DOWN;
bond_propose_link_state(slave, BOND_LINK_DOWN);
commit++;
}
}
Expand All @@ -2840,7 +2839,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
struct slave *slave;

bond_for_each_slave(bond, slave, iter) {
switch (slave->new_link) {
switch (slave->link_new_state) {
case BOND_LINK_NOCHANGE:
continue;

Expand Down Expand Up @@ -2890,8 +2889,9 @@ static void bond_ab_arp_commit(struct bonding *bond)
continue;

default:
slave_err(bond->dev, slave->dev, "impossible: new_link %d on slave\n",
slave->new_link);
slave_err(bond->dev, slave->dev,
"impossible: link_new_state %d on slave\n",
slave->link_new_state);
continue;
}

Expand Down
3 changes: 1 addition & 2 deletions include/net/bonding.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ struct slave {
unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
s8 link; /* one of BOND_LINK_XXXX */
s8 link_new_state; /* one of BOND_LINK_XXXX */
s8 new_link;
u8 backup:1, /* indicates backup slave. Value corresponds with
BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
inactive:1, /* indicates inactive slave */
Expand Down Expand Up @@ -549,7 +548,7 @@ static inline void bond_propose_link_state(struct slave *slave, int state)

static inline void bond_commit_link_state(struct slave *slave, bool notify)
{
if (slave->link == slave->link_new_state)
if (slave->link_new_state == BOND_LINK_NOCHANGE)
return;

slave->link = slave->link_new_state;
Expand Down

0 comments on commit 1899bb3

Please sign in to comment.