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

[netdev-bot+sashiko@kernel.org]

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