Re: [PATCH net-next 1/2] bonding: 3ad: Receive packets in COLLECTING state

[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: 1 · Low: 1

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] The fix clears 'inactive' only when ad_enable_collecting()
  runs.
- [Low] The patch fixes a real, user-visible bug but has no Fixes: tag.

--

bonding: 3ad: Receive packets in COLLECTING state

With coupled control disabled, an 802.3ad port in COLLECTING state should
receive packets, but the RX path only delivers from slaves whose inactive
flag is cleared. This patch makes bond_set_slave_rx_enabled_flags() clear
slave->inactive in addition to slave->rx_disabled.

> When coupled control is disabled, a port in COLLECTING state is supposed
> to receive packets, but it does not. The bond only delivers packets from
> slaves whose 'inactive' flag is cleared, whereas
> bond_set_slave_rx_enabled_flags() only clears 'rx_disabled'.
>
> Fix by clearing 'inactive' as well.

[Severity: Low]
Should this carry a Fixes: tag?

bond_set_slave_rx_enabled_flags() was added by commit 240fd405528b
("bonding: Add independent control state machine"), and that commit only
cleared rx_disabled. The RX gate in bond_should_deliver_exact_match()
checks only bond_is_slave_inactive(), so the bug goes back to that commit.

The notes say this targets net-next because it never worked, and that
makes sense. A tag would still let stable and distro maintainers link the
fix to the original commit:

Fixes: 240fd405528b ("bonding: Add independent control state machine")

> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 598d56b1bc970..cb89aff437f44 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -594,6 +594,7 @@ static inline void bond_set_slave_active_flags(struct slave *slave,
>  static inline void bond_set_slave_rx_enabled_flags(struct slave *slave,
>  					       bool notify)
>  {
> +	slave->inactive = 0;
>  	slave->rx_disabled = 0;
>  }

[Severity: Medium]
With this change, inactive is only cleared when ad_enable_collecting()
runs. Can a COLLECTING port still be left with inactive=1 and
rx_disabled=0?

bond_option_all_slaves_active_set() can still set inactive without
touching rx_disabled when all_slaves_active goes from 1 to 0:

drivers/net/bonding/bond_options.c:bond_option_all_slaves_active_set() {
    ...
	bond_for_each_slave(bond, slave, iter) {
		if (!bond_is_active_slave(slave)) {
			if (newval->value)
				slave->inactive = 0;
			else
				slave->inactive = 1;
		}
	}
    ...
}

A COLLECTING port matches !bond_is_active_slave(). This is because
ad_disable_distributing()->__disable_distributing_port()->
bond_set_slave_tx_disabled_flags() left it in BOND_STATE_BACKUP.

The steady-state COLLECTING recheck in ad_mux_machine() only looks at
rx_disabled:

drivers/net/bonding/bond_3ad.c:ad_mux_machine() {
    ...
					if (aggregator->is_active &&
					    bond_is_slave_rx_disabled(slave)) {
						ad_enable_collecting(port);
						*update_slave_arr = true;
					}
    ...
}

Since rx_disabled stays 0, the recheck never fires. Until the next mux
transition, bond_should_deliver_exact_match() keeps returning true, and
every non-LACPDU frame on that port gets RX_HANDLER_EXACT.

The notes list this as known issue 1 and describe COLLECTING as
transient. However, the companion selftest ("selftests: bonding: Add a
test for 802.3ad with coupled control disabled") keeps a port in
COLLECTING indefinitely by clearing the partner's Collecting bit. So a
partner that never sets LACP_STATE_COLLECTING would keep the port
dropping traffic.

Would it work to also check bond_is_slave_inactive(slave) in the recheck,
for example:

    bond_is_slave_rx_disabled(slave) || bond_is_slave_inactive(slave)

That would let the port recover on the next state machine tick.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111171947.2816258-1-idosch%40nvidia.com