* Re: [PATCH net v2] bonding: fix missing down path on speed/duplex failure after FAIL->UP
2026-10-28 10:47 [PATCH net v2] bonding: fix missing down path on speed/duplex failure after FAIL->UP Timur Stukan
@ 2026-10-29 1:49 ` Hangbin Liu
2026-10-29 15:51 ` Jay Vosburgh
1 sibling, 0 replies; 3+ messages in thread
From: Hangbin Liu @ 2026-10-29 1:49 UTC (permalink / raw)
To: Timur Stukan
Cc: netdev, Nikolay Aleksandrov, Fernando Fernandez Mancera,
Jay Vosburgh, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-kernel
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>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net v2] bonding: fix missing down path on speed/duplex failure after FAIL->UP
2026-10-28 10:47 [PATCH net v2] bonding: fix missing down path on speed/duplex failure after FAIL->UP Timur Stukan
2026-10-29 1:49 ` Hangbin Liu
@ 2026-10-29 15:51 ` Jay Vosburgh
1 sibling, 0 replies; 3+ messages in thread
From: Jay Vosburgh @ 2026-10-29 15:51 UTC (permalink / raw)
To: Timur Stukan
Cc: netdev, Hangbin Liu, Nikolay Aleksandrov,
Fernando Fernandez Mancera, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread