Re: [PATCH net v2] bonding: fix missing down path on speed/duplex failure after FAIL->UP

[Jay Vosburgh <jv@jvosburgh.net>]

Timur Stukan <timur.stukan.linux@gmail.com> wrote:

>In bond_miimon_commit(), when a slave is proposed as BOND_LINK_UP but
>bond_update_speed_duplex() fails in bonding mode that needs speed/duplex
>(802.3ad, tlb, alb), the slave is set to BOND_LINK_DOWN directly. This
>bypasses bond_set_slave_link_state(), bond_miimon_link_change() and
>failover.
>
>This is fine for slave that coming from BOND_LINK_BACK, which was never
>brought up and is not in use. But since commit 1899bb325149 ("bonding:
>fix state transition issue in link monitoring") the commit phase also
>handles BOND_LINK_FAIL -> BOND_LINK_UP which means a slave that is still
>in use and may be curr_active_slave.
>
>If the link recovers within downdelay and goes down again before the
>commit phase reads speed/duplex, the slave is silently marked down while
>remaining the active slave in worst cases. In alb/tlb the bond MAC stays
>on the dead port, no failover happens and TLB/RLB state is not cleaned,
>so the bond stops passing traffic until that link comes back even though
>other slaves are up. A non-active slave is affected too, in alb, RLB
>clients assigned to it are not reassigned and become unreachable.
>
>Reproduced in tlb and alb mode with miimon=100 and downdelay=10000 by
>flapping a slave within downdelay and dropping it again before commit
>(a longer delay was added between inspect and commit to make window
>wider and easier to test):
>  bond0: (slave enp0s3): link status down for interface, disabling it in 10000 ms
>  bond0: (slave enp0s3): link status up again after 3400 ms
>  bond0: (slave enp0s3): failed to get link speed/duplex
>
>procfs then shows the active slave with "MII Status: down" and "Link
>Failure Count: 0", and all traffic is lost.
>
>Fix this by not committing UP proposals in bond_mii_monitor(): UP is
>already committed by bond_miimon_commit() once speed/duplex are valid, so
>leaving it there means slave->link still holds the previous state at
>that point. When speed/duplex update fails for a slave coming from
>BOND_LINK_FAIL, take it through the regular down path, including
>failover. Slaves coming from BACK/DOWN keep the current behaviour.
>The down path is moved into bond_miimon_slave_down() so both callers share it.
>
>After the fix:
>  bond0: (slave enp0s3): failed to get link speed/duplex
>  bond0: (slave enp0s3): link status definitely down, disabling slave
>  bond0: (slave enp0s4): making interface the new active one
>
>Fixes: 1899bb325149 ("bonding: fix state transition issue in link monitoring")
>Signed-off-by: Timur Stukan <timur.stukan.linux@gmail.com>
>---
>
>Notes:
>    Changes in v2:
>    - Drop link_prev_state. Instead, don't commit UP proposals in
>      bond_mii_monitor() and let bond_miimon_commit() commit them after the
>      speed/duplex check, so no extra state is needed (Jay)
>    - v1: https://lore.kernel.org/netdev/20261022103821.89552-1-timur.stukan.linux@gmail.com/
>    
>    Sashiko pointed out on v1 that the 802.3ad netdev notifier can set
>    BOND_LINK_FAIL on a slave that is already down, which then goes through
>    the full down path again. That is pre-existing and reproducible on an
>    unpatched net tree:
>    
>      ip link add bond0 type bond mode 802.3ad miimon 100 lacp_rate 1
>      ip link set enp0s3 down
>      ip link set enp0s4 down
>      ip link set enp0s3 master bond0
>      ip link set enp0s4 master bond0
>      ip link set enp0s3 up
>      ip link set enp0s4 up
>      ip link set bond0 up
>      (wait for LACP to settle)
>      ip link set enp0s3 down
>      ip link set enp0s3 up
>    
>    "link status definitely down" is logged twice and enp0s3 ends up with
>    "Link Failure Count: 2" instead of 1. I will send a separate fix for it.
>
> drivers/net/bonding/bond_main.c | 58 +++++++++++++++++++++++----------
> 1 file changed, 40 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 7019e5d8fdc8..9bafcddbb453 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2779,6 +2779,27 @@ static void bond_miimon_link_change(struct bonding *bond,
> 	}
> }
> 
>+static bool bond_miimon_slave_down(struct bonding *bond,
>+				   struct slave *slave)
>+{
>+	if (slave->link_failure_count < UINT_MAX)
>+		slave->link_failure_count++;
>+
>+	bond_set_slave_link_state(slave, BOND_LINK_DOWN,
>+				  BOND_SLAVE_NOTIFY_NOW);
>+
>+	if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP ||
>+	    BOND_MODE(bond) == BOND_MODE_8023AD)
>+		bond_set_slave_inactive_flags(slave,
>+					      BOND_SLAVE_NOTIFY_NOW);
>+
>+	slave_info(bond->dev, slave->dev, "link status definitely down, disabling slave\n");
>+
>+	bond_miimon_link_change(bond, slave, BOND_LINK_DOWN);
>+
>+	return slave == rcu_access_pointer(bond->curr_active_slave);
>+}
>+
> static void bond_miimon_commit(struct bonding *bond)
> {
> 	struct slave *slave, *primary, *active;
>@@ -2807,10 +2828,21 @@ static void bond_miimon_commit(struct bonding *bond)
> 			err = bond_update_speed_duplex(slave);
> 			netdev_unlock_ops(slave->dev);
> 			if (err && bond_needs_speed_duplex(bond)) {
>-				slave->link = BOND_LINK_DOWN;
> 				if (net_ratelimit())
> 					slave_warn(bond->dev, slave->dev,
> 						   "failed to get link speed/duplex\n");
>+				/* Coming from FAIL, the slave is still in
>+				 * use and may be curr_active_slave, so take
>+				 * it through the full down path. Coming from
>+				 * DOWN/BACK, it was never brought up, so just
>+				 * revert it to DOWN.
>+				 */
>+				if (slave->link == BOND_LINK_FAIL) {
>+					if (bond_miimon_slave_down(bond, slave))
>+						do_failover = true;
>+					continue;
>+				}
>+				slave->link = BOND_LINK_DOWN;
> 				continue;
> 			}
> 			bond_set_slave_link_state(slave, BOND_LINK_UP,
>@@ -2839,24 +2871,8 @@ static void bond_miimon_commit(struct bonding *bond)
> 			continue;
> 
> 		case BOND_LINK_DOWN:
>-			if (slave->link_failure_count < UINT_MAX)
>-				slave->link_failure_count++;
>-
>-			bond_set_slave_link_state(slave, BOND_LINK_DOWN,
>-						  BOND_SLAVE_NOTIFY_NOW);
>-
>-			if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP ||
>-			    BOND_MODE(bond) == BOND_MODE_8023AD)
>-				bond_set_slave_inactive_flags(slave,
>-							      BOND_SLAVE_NOTIFY_NOW);
>-
>-			slave_info(bond->dev, slave->dev, "link status definitely down, disabling slave\n");
>-
>-			bond_miimon_link_change(bond, slave, BOND_LINK_DOWN);
>-
>-			if (slave == rcu_access_pointer(bond->curr_active_slave))
>+			if (bond_miimon_slave_down(bond, slave))
> 				do_failover = true;
>-
> 			continue;
> 
> 		case BOND_LINK_FAIL:
>@@ -2919,6 +2935,12 @@ static void bond_mii_monitor(struct work_struct *work)
> 
> 		if (commit) {
> 			bond_for_each_slave(bond, slave, iter) {
>+				/* UP state is committed by bond_miimon_commit()
>+				 * after the speed and duplex check, so that it
>+				 * can still see the previous link state.
>+				 */
>+				if (slave->link_new_state == BOND_LINK_UP)
>+					continue;
> 				bond_commit_link_state(slave,
> 						       BOND_SLAVE_NOTIFY_LATER);

	Reading bond_commit_link_state(), for this call site, all it
really does is slave->link = slave->link_new_state, when link_new_state
!= BOND_LINK_NOCHAGE.  The changes to ->should_notify_link only matter
for the ARP monitor, so that part is effectively a no-op here.

	Therefore, I wonder if it would be cleaner overall to not do the
bond_commit_link_state here at all, but instead keep what you've added
in the BOND_LINK_UP part of bond_miimon_commit, and then add calls to
bond_set_slave_link_state(slave, ..., BOND_SLAVE_NOTIFY_LATER) to the
BOND_LINK_FAIL/BOND_LINK_BACK case to make the change stick.

	A quick hack patch that I have not tested to hopefully
illustrate what I mean:

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 756d79281ffc..5fb215fa85f6 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2877,6 +2877,8 @@ static void bond_miimon_commit(struct bonding *bond)
 
 		case BOND_LINK_FAIL:
 		case BOND_LINK_BACK:
+			bond_set_slave_link_state(slave, slave->link_new_state,
+						  BOND_SLAVE_NOTIFY_LATER);
 			slave_dbg(bond->dev, slave->dev, "link_new_state %d on slave\n",
 				  slave->link_new_state);
 			continue;
@@ -2933,19 +2935,8 @@ static void bond_mii_monitor(struct work_struct *work)
 			goto re_arm;
 		}
 
-		if (commit) {
-			bond_for_each_slave(bond, slave, iter) {
-				/* UP state is committed by bond_miimon_commit()
-				 * after the speed and duplex check, so that it
-				 * can still see the previous link state.
-				 */
-				if (slave->link_new_state == BOND_LINK_UP)
-					continue;
-				bond_commit_link_state(slave,
-						       BOND_SLAVE_NOTIFY_LATER);
-			}
+		if (commit)
 			bond_miimon_commit(bond);
-		}
 
 		if (bond->send_peer_notif)
 			bond_peer_notify_may_events(bond, true);


	Thoughts?

	-J

---
	-Jay Vosburgh, jv@jvosburgh.net