|
Jiri Slaby |
6f0e97 |
From: Jay Vosburgh <jay.vosburgh@canonical.com>
|
|
Jiri Slaby |
6f0e97 |
Date: Fri, 1 Nov 2019 21:56:42 -0700
|
|
Jiri Slaby |
6f0e97 |
Subject: bonding: fix state transition issue in link monitoring
|
|
Jiri Slaby |
6f0e97 |
Git-commit: 1899bb325149e481de31a4f32b59ea6f24e176ea
|
|
Jiri Slaby |
6f0e97 |
Patch-mainline: 5.4-rc7
|
|
Jiri Slaby |
6f0e97 |
References: networking-stable-19_11_10
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
Since de77ecd4ef02 ("bonding: improve link-status update in
|
|
Jiri Slaby |
6f0e97 |
mii-monitoring"), the bonding driver has utilized two separate variables
|
|
Jiri Slaby |
6f0e97 |
to indicate the next link state a particular slave should transition to.
|
|
Jiri Slaby |
6f0e97 |
Each is used to communicate to a different portion of the link state
|
|
Jiri Slaby |
6f0e97 |
change commit logic; one to the bond_miimon_commit function itself, and
|
|
Jiri Slaby |
6f0e97 |
another to the state transition logic.
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
Unfortunately, the two variables can become unsynchronized,
|
|
Jiri Slaby |
6f0e97 |
resulting in incorrect link state transitions within bonding. This can
|
|
Jiri Slaby |
6f0e97 |
cause slaves to become stuck in an incorrect link state until a
|
|
Jiri Slaby |
6f0e97 |
subsequent carrier state transition.
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
The issue occurs when a special case in bond_slave_netdev_event
|
|
Jiri Slaby |
6f0e97 |
sets slave->link directly to BOND_LINK_FAIL. On the next pass through
|
|
Jiri Slaby |
6f0e97 |
bond_miimon_inspect after the slave goes carrier up, the BOND_LINK_FAIL
|
|
Jiri Slaby |
6f0e97 |
case will set the proposed next state (link_new_state) to BOND_LINK_UP,
|
|
Jiri Slaby |
6f0e97 |
but the new_link to BOND_LINK_DOWN. The setting of the final link state
|
|
Jiri Slaby |
6f0e97 |
from new_link comes after that from link_new_state, and so the slave
|
|
Jiri Slaby |
6f0e97 |
will end up incorrectly in _DOWN state.
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
Resolve this by combining the two variables into one.
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
Reported-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
|
|
Jiri Slaby |
6f0e97 |
Reported-by: Sha Zhang <zhangsha.zhang@huawei.com>
|
|
Jiri Slaby |
6f0e97 |
Cc: Mahesh Bandewar <maheshb@google.com>
|
|
Jiri Slaby |
6f0e97 |
Fixes: de77ecd4ef02 ("bonding: improve link-status update in mii-monitoring")
|
|
Jiri Slaby |
6f0e97 |
Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
|
|
Jiri Slaby |
6f0e97 |
Signed-off-by: David S. Miller <davem@davemloft.net>
|
|
Jiri Slaby |
6f0e97 |
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
|
|
Jiri Slaby |
6f0e97 |
---
|
|
Jiri Slaby |
6f0e97 |
drivers/net/bonding/bond_main.c | 44 ++++++++++++++++++++--------------------
|
|
Jiri Slaby |
6f0e97 |
include/net/bonding.h | 3 --
|
|
Jiri Slaby |
6f0e97 |
2 files changed, 23 insertions(+), 24 deletions(-)
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
--- a/drivers/net/bonding/bond_main.c
|
|
Jiri Slaby |
6f0e97 |
+++ b/drivers/net/bonding/bond_main.c
|
|
Jiri Slaby |
6f0e97 |
@@ -2048,8 +2048,7 @@ static int bond_miimon_inspect(struct bo
|
|
Jiri Slaby |
6f0e97 |
ignore_updelay = !rcu_dereference(bond->curr_active_slave);
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
bond_for_each_slave_rcu(bond, slave, iter) {
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_NOCHANGE;
|
|
Jiri Slaby |
6f0e97 |
- slave->link_new_state = slave->link;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
link_state = bond_check_dev_link(bond, slave->dev, 0);
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
@@ -2085,7 +2084,7 @@ static int bond_miimon_inspect(struct bo
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
if (slave->delay <= 0) {
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_DOWN;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_DOWN);
|
|
Jiri Slaby |
6f0e97 |
commit++;
|
|
Jiri Slaby |
6f0e97 |
continue;
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
@@ -2124,7 +2123,7 @@ static int bond_miimon_inspect(struct bo
|
|
Jiri Slaby |
6f0e97 |
slave->delay = 0;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
if (slave->delay <= 0) {
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_UP;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_UP);
|
|
Jiri Slaby |
6f0e97 |
commit++;
|
|
Jiri Slaby |
6f0e97 |
ignore_updelay = false;
|
|
Jiri Slaby |
6f0e97 |
continue;
|
|
Jiri Slaby |
6f0e97 |
@@ -2144,7 +2143,7 @@ static void bond_miimon_commit(struct bo
|
|
Jiri Slaby |
6f0e97 |
struct slave *slave, *primary;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
bond_for_each_slave(bond, slave, iter) {
|
|
Jiri Slaby |
6f0e97 |
- switch (slave->new_link) {
|
|
Jiri Slaby |
6f0e97 |
+ switch (slave->link_new_state) {
|
|
Jiri Slaby |
6f0e97 |
case BOND_LINK_NOCHANGE:
|
|
Jiri Slaby |
6f0e97 |
/* For 802.3ad mode, check current slave speed and
|
|
Jiri Slaby |
6f0e97 |
* duplex again in case its port was disabled after
|
|
Jiri Slaby |
6f0e97 |
@@ -2237,8 +2236,8 @@ static void bond_miimon_commit(struct bo
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
default:
|
|
Jiri Slaby |
6f0e97 |
netdev_err(bond->dev, "invalid new link %d on slave %s\n",
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link, slave->dev->name);
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_NOCHANGE;
|
|
Jiri Slaby |
6f0e97 |
+ slave->link_new_state, slave->dev->name);
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
continue;
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
@@ -2637,13 +2636,13 @@ static void bond_loadbalance_arp_mon(str
|
|
Jiri Slaby |
6f0e97 |
bond_for_each_slave_rcu(bond, slave, iter) {
|
|
Jiri Slaby |
6f0e97 |
unsigned long trans_start = dev_trans_start(slave->dev);
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_NOCHANGE;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
if (slave->link != BOND_LINK_UP) {
|
|
Jiri Slaby |
6f0e97 |
if (bond_time_in_interval(bond, trans_start, 1) &&
|
|
Jiri Slaby |
6f0e97 |
bond_time_in_interval(bond, slave->last_rx, 1)) {
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_UP;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_UP);
|
|
Jiri Slaby |
6f0e97 |
slave_state_changed = 1;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
/* primary_slave has no meaning in round-robin
|
|
Jiri Slaby |
6f0e97 |
@@ -2670,7 +2669,7 @@ static void bond_loadbalance_arp_mon(str
|
|
Jiri Slaby |
6f0e97 |
if (!bond_time_in_interval(bond, trans_start, 2) ||
|
|
Jiri Slaby |
6f0e97 |
!bond_time_in_interval(bond, slave->last_rx, 2)) {
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_DOWN;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_DOWN);
|
|
Jiri Slaby |
6f0e97 |
slave_state_changed = 1;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
if (slave->link_failure_count < UINT_MAX)
|
|
Jiri Slaby |
6f0e97 |
@@ -2702,8 +2701,8 @@ static void bond_loadbalance_arp_mon(str
|
|
Jiri Slaby |
6f0e97 |
goto re_arm;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
bond_for_each_slave(bond, slave, iter) {
|
|
Jiri Slaby |
6f0e97 |
- if (slave->new_link != BOND_LINK_NOCHANGE)
|
|
Jiri Slaby |
6f0e97 |
- slave->link = slave->new_link;
|
|
Jiri Slaby |
6f0e97 |
+ if (slave->link_new_state != BOND_LINK_NOCHANGE)
|
|
Jiri Slaby |
6f0e97 |
+ slave->link = slave->link_new_state;
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
if (slave_state_changed) {
|
|
Jiri Slaby |
6f0e97 |
@@ -2726,9 +2725,9 @@ re_arm:
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
/* Called to inspect slaves for active-backup mode ARP monitor link state
|
|
Jiri Slaby |
6f0e97 |
- * changes. Sets new_link in slaves to specify what action should take
|
|
Jiri Slaby |
6f0e97 |
- * place for the slave. Returns 0 if no changes are found, >0 if changes
|
|
Jiri Slaby |
6f0e97 |
- * to link states must be committed.
|
|
Jiri Slaby |
6f0e97 |
+ * changes. Sets proposed link state in slaves to specify what action
|
|
Jiri Slaby |
6f0e97 |
+ * should take place for the slave. Returns 0 if no changes are found, >0
|
|
Jiri Slaby |
6f0e97 |
+ * if changes to link states must be committed.
|
|
Jiri Slaby |
6f0e97 |
*
|
|
Jiri Slaby |
6f0e97 |
* Called with rcu_read_lock held.
|
|
Jiri Slaby |
6f0e97 |
*/
|
|
Jiri Slaby |
6f0e97 |
@@ -2740,12 +2739,12 @@ static int bond_ab_arp_inspect(struct bo
|
|
Jiri Slaby |
6f0e97 |
int commit = 0;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
bond_for_each_slave_rcu(bond, slave, iter) {
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_NOCHANGE;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
|
|
Jiri Slaby |
6f0e97 |
last_rx = slave_last_rx(bond, slave);
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
if (slave->link != BOND_LINK_UP) {
|
|
Jiri Slaby |
6f0e97 |
if (bond_time_in_interval(bond, last_rx, 1)) {
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_UP;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_UP);
|
|
Jiri Slaby |
6f0e97 |
commit++;
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
continue;
|
|
Jiri Slaby |
6f0e97 |
@@ -2773,7 +2772,7 @@ static int bond_ab_arp_inspect(struct bo
|
|
Jiri Slaby |
6f0e97 |
if (!bond_is_active_slave(slave) &&
|
|
Jiri Slaby |
6f0e97 |
!rcu_access_pointer(bond->current_arp_slave) &&
|
|
Jiri Slaby |
6f0e97 |
!bond_time_in_interval(bond, last_rx, 3)) {
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_DOWN;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_DOWN);
|
|
Jiri Slaby |
6f0e97 |
commit++;
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
@@ -2786,7 +2785,7 @@ static int bond_ab_arp_inspect(struct bo
|
|
Jiri Slaby |
6f0e97 |
if (bond_is_active_slave(slave) &&
|
|
Jiri Slaby |
6f0e97 |
(!bond_time_in_interval(bond, trans_start, 2) ||
|
|
Jiri Slaby |
6f0e97 |
!bond_time_in_interval(bond, last_rx, 2))) {
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link = BOND_LINK_DOWN;
|
|
Jiri Slaby |
6f0e97 |
+ bond_propose_link_state(slave, BOND_LINK_DOWN);
|
|
Jiri Slaby |
6f0e97 |
commit++;
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
@@ -2806,7 +2805,7 @@ static void bond_ab_arp_commit(struct bo
|
|
Jiri Slaby |
6f0e97 |
struct slave *slave;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
bond_for_each_slave(bond, slave, iter) {
|
|
Jiri Slaby |
6f0e97 |
- switch (slave->new_link) {
|
|
Jiri Slaby |
6f0e97 |
+ switch (slave->link_new_state) {
|
|
Jiri Slaby |
6f0e97 |
case BOND_LINK_NOCHANGE:
|
|
Jiri Slaby |
6f0e97 |
continue;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
@@ -2858,8 +2857,9 @@ static void bond_ab_arp_commit(struct bo
|
|
Jiri Slaby |
6f0e97 |
continue;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
default:
|
|
Jiri Slaby |
6f0e97 |
- netdev_err(bond->dev, "impossible: new_link %d on slave %s\n",
|
|
Jiri Slaby |
6f0e97 |
- slave->new_link, slave->dev->name);
|
|
Jiri Slaby |
6f0e97 |
+ netdev_err(bond->dev,
|
|
Jiri Slaby |
6f0e97 |
+ "impossible: new_link %d on slave %s\n",
|
|
Jiri Slaby |
6f0e97 |
+ slave->link_new_state, slave->dev->name);
|
|
Jiri Slaby |
6f0e97 |
continue;
|
|
Jiri Slaby |
6f0e97 |
}
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
--- a/include/net/bonding.h
|
|
Jiri Slaby |
6f0e97 |
+++ b/include/net/bonding.h
|
|
Jiri Slaby |
6f0e97 |
@@ -149,7 +149,6 @@ struct slave {
|
|
Jiri Slaby |
6f0e97 |
unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
|
|
Jiri Slaby |
6f0e97 |
s8 link; /* one of BOND_LINK_XXXX */
|
|
Jiri Slaby |
6f0e97 |
s8 link_new_state; /* one of BOND_LINK_XXXX */
|
|
Jiri Slaby |
6f0e97 |
- s8 new_link;
|
|
Jiri Slaby |
6f0e97 |
u8 backup:1, /* indicates backup slave. Value corresponds with
|
|
Jiri Slaby |
6f0e97 |
BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
|
|
Jiri Slaby |
6f0e97 |
inactive:1, /* indicates inactive slave */
|
|
Jiri Slaby |
6f0e97 |
@@ -523,7 +522,7 @@ static inline void bond_propose_link_sta
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
static inline void bond_commit_link_state(struct slave *slave, bool notify)
|
|
Jiri Slaby |
6f0e97 |
{
|
|
Jiri Slaby |
6f0e97 |
- if (slave->link == slave->link_new_state)
|
|
Jiri Slaby |
6f0e97 |
+ if (slave->link_new_state == BOND_LINK_NOCHANGE)
|
|
Jiri Slaby |
6f0e97 |
return;
|
|
Jiri Slaby |
6f0e97 |
|
|
Jiri Slaby |
6f0e97 |
slave->link = slave->link_new_state;
|