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

[PATCH net] 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 recording the link state before the miimon commit and when
the speed/duplex update fails for a slave coming from BOND_LINK_FAIL,
taking it through the regular down path, including failover. The
BOND_LINK_BACK case keeps 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:
    sizeof(struct slave) grows from 608 to 616 bytes due to padding.
    I kept link_prev_state next to link/link_new_state for readability;
    it can be moved into the hole after 'delay' if preferred.

 drivers/net/bonding/bond_main.c | 53 ++++++++++++++++++++++-----------
 include/net/bonding.h           |  1 +
 2 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 09aa45d71fab..602ad697a9c2 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_prev_state == 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,7 @@ static void bond_mii_monitor(struct work_struct *work)
 
 		if (commit) {
 			bond_for_each_slave(bond, slave, iter) {
+				slave->link_prev_state = slave->link;
 				bond_commit_link_state(slave,
 						       BOND_SLAVE_NOTIFY_LATER);
 			}
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 598d56b1bc97..b2b102946fe1 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -165,6 +165,7 @@ 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     link_prev_state; /* one of BOND_LINK_XXXX */
 	u8     backup:1,   /* indicates backup slave. Value corresponds with
 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
 	       inactive:1, /* indicates inactive slave */
-- 
2.43.0

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

[Hangbin Liu]

On Thu, Oct 22, 2026 at 01:38:21PM +0300, 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 recording the link state before the miimon commit and when
> the speed/duplex update fails for a slave coming from BOND_LINK_FAIL,
> taking it through the regular down path, including failover. The
> BOND_LINK_BACK case keeps 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:
>     sizeof(struct slave) grows from 608 to 616 bytes due to padding.
>     I kept link_prev_state next to link/link_new_state for readability;
>     it can be moved into the hole after 'delay' if preferred.
> 
>  drivers/net/bonding/bond_main.c | 53 ++++++++++++++++++++++-----------
>  include/net/bonding.h           |  1 +
>  2 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 09aa45d71fab..602ad697a9c2 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;

Hmm, this issue only happens when link_state is on

	netif_running(slave->dev) && netif_carrier_ok(slave->dev);

while !netif_device_present. Should we set the link up when device detached?

>  				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_prev_state == 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,7 @@ static void bond_mii_monitor(struct work_struct *work)
>  
>  		if (commit) {
>  			bond_for_each_slave(bond, slave, iter) {
> +				slave->link_prev_state = slave->link;
>  				bond_commit_link_state(slave,
>  						       BOND_SLAVE_NOTIFY_LATER);
>  			}
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 598d56b1bc97..b2b102946fe1 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -165,6 +165,7 @@ 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     link_prev_state; /* one of BOND_LINK_XXXX */
>  	u8     backup:1,   /* indicates backup slave. Value corresponds with
>  			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
>  	       inactive:1, /* indicates inactive slave */
> -- 
> 2.43.0
> 

Checking slave->inactive is not enough when user sets all_slaves_active, so
I'm not opposed to adding a new link state. The code looks good to me.
But let's see others opinion.

Thanks
Hangbin

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

[Timur Stukan]

On Fri, 23 Oct 2026 at 10:48, Hangbin Liu <hangbin.liu@linux.dev> wrote:

> >  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;
>
> Hmm, this issue only happens when link_state is on
>
>         netif_running(slave->dev) && netif_carrier_ok(slave->dev);
>
> while !netif_device_present. Should we set the link up when device detached?


Hi Hangbin,

Thanks for the review and the suggestion.

A detached device is one way to get there, but it does not cover all
the ways to hit this bug. In particular, it does not cover the case i
reproduced: during inspect both link_state and netif_device_present()
are true, but the carrier is lost after inspect and before commit, so
reading speed/duplex fails in commit.


Thanks,
Timur

Re: [PATCH net] 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 recording the link state before the miimon commit and when
>the speed/duplex update fails for a slave coming from BOND_LINK_FAIL,
>taking it through the regular down path, including failover. The
>BOND_LINK_BACK case keeps 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>
>---

	Reading the description and patch, I'm concerned that this patch
is introducing the same sort of issue that 1899bb325149 was solving: two
state variables that end up becoming unsynchronized.

	For example, here you set link_prev_state in one place, inspect
it in another, but never clear it.  Thus, once set, it will remain set
forever, raising the possibility of false matches in the future.  Not
necessarily from this patch, which appears to always set then check in
that order, but from subsequent changes down the road that inadvertently
break this guarantee.

	Yes, that can be resolved by clearing it after use, but I'd
first ask whether there is an effective solution that does not require
keeping additional state?

	-J

>
>Notes:
>    sizeof(struct slave) grows from 608 to 616 bytes due to padding.
>    I kept link_prev_state next to link/link_new_state for readability;
>    it can be moved into the hole after 'delay' if preferred.
>
> drivers/net/bonding/bond_main.c | 53 ++++++++++++++++++++++-----------
> include/net/bonding.h           |  1 +
> 2 files changed, 36 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 09aa45d71fab..602ad697a9c2 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_prev_state == 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,7 @@ static void bond_mii_monitor(struct work_struct *work)
> 
> 		if (commit) {
> 			bond_for_each_slave(bond, slave, iter) {
>+				slave->link_prev_state = slave->link;
> 				bond_commit_link_state(slave,
> 						       BOND_SLAVE_NOTIFY_LATER);
> 			}
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 598d56b1bc97..b2b102946fe1 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -165,6 +165,7 @@ 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     link_prev_state; /* one of BOND_LINK_XXXX */
> 	u8     backup:1,   /* indicates backup slave. Value corresponds with
> 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
> 	       inactive:1, /* indicates inactive slave */
>-- 
>2.43.0
>

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

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

[Timur Stukan]

On Fri, 23 Oct 2026 at 16:24, Jay Vosburgh <jv@jvosburgh.net> wrote:
>         Reading the description and patch, I'm concerned that this patch
> is introducing the same sort of issue that 1899bb325149 was solving: two
> state variables that end up becoming unsynchronized.
>
>         For example, here you set link_prev_state in one place, inspect
> it in another, but never clear it.  Thus, once set, it will remain set
> forever, raising the possibility of false matches in the future.  Not
> necessarily from this patch, which appears to always set then check in
> that order, but from subsequent changes down the road that inadvertently
> break this guarantee.
>
>         Yes, that can be resolved by clearing it after use, but I'd
> first ask whether there is an effective solution that does not require
> keeping additional state?
>
>         -J


Hi Jay,

Thanks for the review.

Yes, I think it can be solved without additional state. The previous
link state is only needed when link_new_state is BOND_LINK_UP, so
bond_mii_monitor() could skip committing just that state and leave it
to bond_miimon_commit(), which already sets the link UP after the
speed/duplex check. Then slave->link still holds FAIL, BACK or DOWN at
that point, so no new field is needed.

This also seems more correct to me: a slave becomes BOND_LINK_UP only
after the speed/duplex check, so other slaves processed earlier in the
same pass never see it as up before that.

If this approach is acceptable to you, I'll send a v2.

Thanks,
Timur

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

[Hangbin Liu]

On Fri, Oct 23, 2026 at 03:35:57PM +0300, Timur Stukan wrote:
> On Fri, 23 Oct 2026 at 10:48, Hangbin Liu <hangbin.liu@linux.dev> wrote:
> 
> > >  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;
> >
> > Hmm, this issue only happens when link_state is on
> >
> >         netif_running(slave->dev) && netif_carrier_ok(slave->dev);
> >
> > while !netif_device_present. Should we set the link up when device detached?
> 
> 
> Hi Hangbin,
> 
> Thanks for the review and the suggestion.
> 
> A detached device is one way to get there, but it does not cover all
> the ways to hit this bug. In particular, it does not cover the case i
> reproduced: during inspect both link_state and netif_device_present()
> are true, but the carrier is lost after inspect and before commit, so
> reading speed/duplex fails in commit.

Hi Timur,

Thanks for the explain, but the same question to this patch. How do you
make sure the carrier is not lost during bond_miimon_inspect() and 
this slave->link_prev_state setting?

Thanks
Hangbin

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

[Timur Stukan]

On Mon, 26 Oct 2026 at 02:56, Hangbin Liu <hangbin.liu@linux.dev> wrote:
>
> On Fri, Oct 23, 2026 at 03:35:57PM +0300, Timur Stukan wrote:
> > On Fri, 23 Oct 2026 at 10:48, Hangbin Liu <hangbin.liu@linux.dev> wrote:
> >
> > > >  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;
> > >
> > > Hmm, this issue only happens when link_state is on
> > >
> > >         netif_running(slave->dev) && netif_carrier_ok(slave->dev);
> > >
> > > while !netif_device_present. Should we set the link up when device detached?
> >
> >
> > Hi Hangbin,
> >
> > Thanks for the review and the suggestion.
> >
> > A detached device is one way to get there, but it does not cover all
> > the ways to hit this bug. In particular, it does not cover the case i
> > reproduced: during inspect both link_state and netif_device_present()
> > are true, but the carrier is lost after inspect and before commit, so
> > reading speed/duplex fails in commit.
>
> Hi Timur,
>
> Thanks for the explain, but the same question to this patch. How do you
> make sure the carrier is not lost during bond_miimon_inspect() and
> this slave->link_prev_state setting?
>
> Thanks
> Hangbin

Hi Hangbin,

Thanks for the question. To answer directly: there is no way to make
sure the carrier is not lost between bond_miimon_inspect() and setting
link_prev_state, but the fix does not need that.

Let's split the miimon logic with this patch into 3 steps, executed in
this order:
- bond_miimon_inspect() - carrier is checked here and new state is proposed.
- setting slave->link_prev_state - just a copy of slave->link, it is
not read from the carrier.
- bond_miimon_commit(), case UP - speed/duplex check.

And there is the case where downdelay is set and active slave flaps,
so it is currently in FAIL. Possible scenarios:
a) The carrier is present during inspect, so bond_miimon_inspect()
proposes link_new_state as UP for active slave. Then the carrier is
lost somewhere between bond_miimon_inspect() and bond_miimon_commit().
It does not matter whether this happens before (your question) or
after setting link_prev_state, it is set to FAIL anyway. Then in
bond_miimon_commit() in case UP, speed/duplex check fails because
carrier is lost and because link_prev_state is FAIL, the active slave
goes through a regular down path with failover.
b) The carrier is lost before bond_miimon_inspect(), so UP is not
proposed at all, and after downdelay the slave goes DOWN through the
same regular down path.
c) The carrier is lost after bond_miimon_commit(), so the slave is
already UP. On the next miimon tick bond_miimon_inspect() proposes
FAIL and then it follows the same path as in b).

So slave->link_prev_state is not affected by carrier changes between
inspect and commit, that is just a copy of slave->link, needed to do
the right thing in bond_miimon_commit() if the speed/duplex check
fails (e.g. because the carrier is lost).

The same applies to the approach without link_prev_state I proposed to
Jay, since there slave->link itself still holds the previous state in
bond_miimon_commit().

Thanks,
Timur.

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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, the new `link_prev_state == BOND_LINK_FAIL`
  branch in bond_miimon_commit()…

--

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

This patch adds a link_prev_state field that holds the slave link state
from just before the miimon commit. If bond_update_speed_duplex() fails
while committing a BOND_LINK_UP proposal for a slave that was in
BOND_LINK_FAIL, the slave now goes through the full down path, including
failover. That down path is moved into a new bond_miimon_slave_down()
helper.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 09aa45d71fabb..602ad697a9c2b 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_prev_state == BOND_LINK_FAIL) {
> +					if (bond_miimon_slave_down(bond, slave))
> +						do_failover = true;
> +					continue;
> +				}

[Severity: Medium]
In 802.3ad mode, does BOND_LINK_FAIL always mean the slave is still in use?

bond_slave_netdev_event() also sets BOND_LINK_FAIL on NETDEV_UP and
NETDEV_CHANGE, and that has nothing to do with downdelay:

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;
		}
    ...
}

The last_link_up check passes for any slave that has ever been up. That
includes a slave that is currently BOND_LINK_DOWN and not in use.

Take an 802.3ad slave in BOND_LINK_DOWN with a non-zero last_link_up. Its
carrier comes back, but the driver still reports unknown speed or duplex,
for example during slow autoneg:

bond_slave_netdev_event(NETDEV_CHANGE)
    slave->link = BOND_LINK_FAIL
bond_mii_monitor()
    bond_miimon_inspect()
        case BOND_LINK_FAIL, link_state up -> propose BOND_LINK_UP
    slave->link_prev_state = slave->link    /* BOND_LINK_FAIL */
    bond_commit_link_state()                /* link = BOND_LINK_UP */
    bond_miimon_commit()
        bond_update_speed_duplex() fails again
        bond_miimon_slave_down()

Before this patch, this path only set slave->link back to BOND_LINK_DOWN.

With this patch, a slave that was already down now gets:

- link_failure_count incremented again
- BOND_SLAVE_NOTIFY_NOW link-state events
- a "link status definitely down, disabling slave" message, with no rate
  limiting
- a call to bond_3ad_handle_link_change(slave, BOND_LINK_DOWN), which
  reruns aggregator selection and rebuilds the slave array

This can happen again on every NETDEV_CHANGE that arrives while the slave
is down. Could "Link Failure Count" in /proc/net/bonding keep rising for a
port that never actually came back up?

The new comment ("Coming from FAIL, the slave is still in use") and the
commit message only cover the downdelay case. Should this branch also
check that the slave was really in use, instead of relying only on
link_prev_state?

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

[ ... ]

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

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

[Timur Stukan]

On Mon, 26 Oct 2026 at 13:01, <netdev-bot+sashiko@kernel.org> wrote:
> > +                             if (slave->link_prev_state == BOND_LINK_FAIL) {
> > +                                     if (bond_miimon_slave_down(bond, slave))
> > +                                             do_failover = true;
> > +                                     continue;
> > +                             }
>
> [Severity: Medium]
> In 802.3ad mode, does BOND_LINK_FAIL always mean the slave is still in use?

Good catch. No, BOND_LINK_FAIL does not always mean that slave is
still in use, bond_miimon_inspect() follows that rule but
bond_slave_netdev_event() does not.

That is not new behaviour though, before this patch it could be reproduced also:

Take the 802.3ad slave in BOND_LINK_DOWN with a non-zero last_link_up.
Its carrier comes back and is lost again before miimon brings the
slave up and driver still reports unknown speed/duplex, so the
bond_slave_netdev_event() sets slave as FAIL, then in
bond_miimon_inspect() carrier check fails and link_new_state is
proposed as DOWN and bond_miimon_commit() runs the slave through the
full down path although it is already down, with the same
consequences: link_failure_count incremented, a notify event, dmesg
print and a call to bond_3ad_handle_link_change(). This patch only
adds another way into that path, when the carrier is up but
speed/duplex are still invalid.

The root cause is the notifier using the last_link_up rather than the
current link state. I can fix that with a separate patch that sets
FAIL only for a slave that is currently UP or FAIL, either as part of
this series or as a follow-up.

Anyway, I'll wait for Jay's feedback on the overall approach before sending v2.

pw-bot: cr