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

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

[Timur Stukan]

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);
 			}
-- 
2.43.0

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

[Hangbin Liu]

Hi Timur,
On Wed, Oct 28, 2026 at 12:47:57PM +0200, Timur Stukan 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);
>  			}
> -- 
> 2.43.0
> 

This version looks good to me. Thanks!

Reviewed-by: Hangbin Liu <liuhangbin@kylinos.cn>

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

[Jay Vosburgh]

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

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

[Hangbin Liu]

Hi Jay,
On Thu, Oct 29, 2026 at 10:51:11AM -0500, Jay Vosburgh wrote:
> >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
> > 
> > 		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.

+1

> 
> 	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);

- bond_set_slave_link_state(slave, slave->link_new_state, BOND_SLAVE_NOTIFY_LATER)
  - bond_propose_link_state(slave, slave->link_new_state)
    - slave->link_new_state = slave->link_new_state

This looks like a no-op here.

Maybe just call bond_commit_link_state()?

The bond_set_slave_link_state() in bond_miimon_commit() seems also have this
issue.

Thanks
Hangbin
>  			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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In 802.3ad mode, a slave that has already gone through the
  full down path can now go through it a second time.
- [Medium] The fix only sends `slave->link == BOND_LINK_FAIL` through the
  full down path.

--

bonding: fix missing down path on speed/duplex failure after FAIL->UP

With this patch, bond_mii_monitor() no longer pre-commits BOND_LINK_UP
proposals, so bond_miimon_commit() can still see the previous link state
when the speed/duplex lookup fails. Slaves coming from BOND_LINK_FAIL now
go through the regular down path, including failover. That path is moved
into the new bond_miimon_slave_down(), and DOWN/BACK slaves are still
reverted to DOWN directly.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7019e5d8fdc8e..9bafcddbb453c 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c

[ ... ]

> @@ -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;
> +				}

[Severity: Medium]
Does slave->link == BOND_LINK_FAIL always mean the slave is still in use?

In 802.3ad mode, bond_slave_netdev_event() can set FAIL on a slave that
is already DOWN:

drivers/net/bonding/bond_main.c:bond_slave_netdev_event() {
    ...
		if (bond_update_speed_duplex(slave) &&
		    BOND_MODE(bond) == BOND_MODE_8023AD) {
			if (slave->last_link_up)
				slave->link = BOND_LINK_FAIL;
			else
				slave->link = BOND_LINK_DOWN;
		}
    ...
}

last_link_up is a timestamp and is never cleared. Take a slave that was
up once and then went through the down path. If NETDEV_CHANGE arrives
when carrier returns and speed/duplex is still unknown, the slave is
relabelled FAIL. The next bond_miimon_inspect() sees FAIL with carrier up
and proposes BOND_LINK_UP from the "recovered before downdelay expired"
branch.

There is also a race: inspect proposes UP for a DOWN/BACK slave, and a
carrier notifier sets FAIL between rcu_read_unlock() and rtnl_trylock()
in bond_mii_monitor().

Before this patch, bond_mii_monitor() committed the UP proposal first.
That overwrote the notifier's FAIL, and a speed/duplex failure then only
did slave->link = BOND_LINK_DOWN. With the new skip in bond_mii_monitor():

	if (slave->link_new_state == BOND_LINK_UP)
		continue;

FAIL is still set when this branch runs. If speed/duplex is still
invalid, bond_miimon_slave_down() runs a second time for a port that is
already disabled.

Would that increment link_failure_count again and log "link status
definitely down" twice? It would also send another NOTIFY_NOW lower state
event, rerun bond_3ad_handle_link_change(BOND_LINK_DOWN), and possibly
request failover.

The patch notes call the notifier relabelling pre-existing, but only for
the FAIL->DOWN downdelay route. This looks like a new route to the same
double down accounting, and the new comment states the "still in use"
assumption as fact.

> +				slave->link = BOND_LINK_DOWN;
>  				continue;
>  			}

[Severity: Medium]
Can an in-use slave reach this fallback with slave->link == BOND_LINK_UP?

bond_miimon_inspect() runs under RCU only. In the BOND_LINK_BACK case it
can propose BOND_LINK_UP once updelay expires or ignore_updelay is set.

Between rcu_read_unlock() and rtnl_trylock() in bond_mii_monitor(),
another RTNL holder can call bond_select_active_slave(). Examples are
__bond_release_one(), bond_enslave(), the primary/prio/active_slave
option setters and NETDEV_CHANGENAME. If that picks the BACK slave, it
gets promoted:

drivers/net/bonding/bond_main.c:bond_change_active_slave() {
    ...
		if (new_active->link == BOND_LINK_BACK) {
			...
			new_active->delay = 0;
			bond_set_slave_link_state(new_active, BOND_LINK_UP,
						  BOND_SLAVE_NOTIFY_NOW);

			if (BOND_MODE(bond) == BOND_MODE_8023AD)
				bond_3ad_handle_link_change(new_active, BOND_LINK_UP);

			if (bond_is_lb(bond))
				bond_alb_handle_link_change(bond, new_active, BOND_LINK_UP);
    ...
}

The slave is now curr_active_slave with link == UP, and in alb/tlb it
holds the bond MAC. bond_mii_monitor() skips the UP proposal. If
bond_update_speed_duplex() then fails here, the FAIL check is false. The
active slave is set to DOWN without bond_miimon_link_change(), TLB/RLB
cleanup or failover. The next inspect sees DOWN with no carrier and does
nothing.

bond_enslave() can leave the same link == UP / link_new_state == UP state
if it completes in that window, even when its own speed/duplex check
failed.

This path behaves the same as before the patch. But the new comment says
DOWN/BACK slaves were never brought up, and that is not true here.

Would checking for "not DOWN/BACK", or for slave == curr_active_slave,
catch this case as well as FAIL?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028104758.299129-1-timur.stukan.linux%40gmail.com

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

[Timur Stukan]

On Thu, 29 Oct 2026 at 17:51, Jay Vosburgh <jv@jvosburgh.net> wrote:

>         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


Hi Jay,

Thanks for the review.

I had the same thought and was planning to send it later as a separate
patch, but doing it here makes sense.

I went through the possible consequences. Without the loop, DOWN is
also applied only when bond_miimon_commit() reaches that slave. I
don't think other slaves handled earlier in the same pass can be
affected, because when each such slave goes DOWN, its own link change
rebuilds the arrays and redoes the aggregator selection anyway, so
nothing stale is left after the pass.

I tested it in 802.3ad, alb and active-backup with downdelay 0,
including two slaves going down, and the original FAIL->UP case with
downdelay. I'll send v3 with this change

Thanks,
Timur.

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

[Timur Stukan]

On Fri, 30 Oct 2026 at 12:50, <netdev-bot+sashiko@kernel.org> wrote:

> Does slave->link == BOND_LINK_FAIL always mean the slave is still in use?

No, but both routes described here start with the 802.3ad notifier
setting FAIL on a slave that is DOWN or BACK, because it checks
last_link_up rather than the current link state. I'll change the
notifier to set FAIL only for a slave that is currently UP, so FAIL
again means the slave was in use, and send it as patch 1 of v3.

> Can an in-use slave reach this fallback with slave->link == BOND_LINK_UP?

Yes, this is also pre-existing, but it is the same problem this patch
is fixing: a slave that may be in use being set to DOWN silently. I
will add that to v3 also, and in v3 the full down path will be taken
for a slave that is FAIL or UP if speed/duplex check fails, and only
DOWN/BACK will be reverted silently. Checking curr_active_slave alone
would miss non-active slaves in alb mode. In a normal miimon flow UP
is never proposed for a slave that is already UP, so this only affects
the racy cases.

pw-bot: cr