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

[Hangbin Liu <hangbin.liu@linux.dev>]

Hi Ido,
On Thu, Nov 12, 2026 at 08:21:57PM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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")

Makes sense to me.

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

If the partner does not set LACP_STATE_COLLECTING, it is not supposed to
send any packets.

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

I'm fine with or without this checking. But having this check looks better.
Up to you.

Thanks
Hangbin