Re: [PATCH net v2] 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 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