netdev - Re: [PATCH net 1/2] bonding: update ntt to true in passive mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aHd4ddc1YzeT1lN3@fedora>
Date: Wed, 16 Jul 2025 10:01:25 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Jay Vosburgh <jv@...sburgh.net>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Simon Horman <horms@...nel.org>, Shuah Khan <shuah@...nel.org>,
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/2] bonding: update ntt to true in passive mode

On Tue, Jul 15, 2025 at 09:19:49PM -0700, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@...il.com> wrote:
> 
> >When lacp_active is set to off, the bond operates in passive mode, meaning it
> >will only "speak when spoken to." However, the current kernel implementation
> >only sends an LACPDU in response when the partner's state changes.
> >
> >In this situation, once LACP negotiation succeeds, the actor stops sending
> >LACPDUs until the partner times out and sends an "expired" LACPDU.
> >This leads to endless LACP state flapping.
> 
> 	From the above, I suspect our implementation isn't compliant to
> the standard.  Per IEEE 802.1AX-2014 6.4.1 LACP design elements:
> 
> c)	Active or passive participation in LACP is controlled by
> 	LACP_Activity, an administrative control associated with each
> 	Aggregation Port, that can take the value Active LACP or Passive
> 	LACP. Passive LACP indicates the Aggregation Port’s preference
> 	for not transmitting LACPDUs unless its Partner’s control value
> 	is Active LACP (i.e., a preference not to speak unless spoken
> 	to). Active LACP indicates the Aggregation Port’s preference to

OK, so this means the passive side should start sending LACPDUs when receive
passive actor's LACPDUs, with the slow/fast rate based on partner's rate?

Hmm, then when we should stop sending LACPDUs? After
port->sm_mux_state == AD_MUX_DETACHED ?

> 	participate in the protocol regardless of the Partner’s control
> 	value (i.e., a preference to speak regardless).
> 
> d)	Periodic transmission of LACPDUs occurs if the LACP_Activity
> 	control of either the Actor or the Partner is Active LACP. These
> 	periodic transmissions will occur at either a slow or fast
> 	transmission rate depending upon the expressed LACP_Timeout
> 	preference (Long Timeout or Short Timeout) of the Partner
> 	System.
> 
> 	Which, in summary, means that if either end (actor or partner)
> has LACP_Activity set, both ends must send periodic LACPDUs at the rate
> specified by their respective partner's LACP_Timeout rate.
> 
> >To avoid this, we need update ntt to true once received an LACPDU from the
> >partner, ensuring an immediate reply. With this fix, the link becomes stable
> >in most cases, except for one specific scenario:
> >
> >Actor: lacp_active=off, lacp_rate=slow
> >Partner: lacp_active=on, lacp_rate=fast
> >
> >In this case, the partner expects frequent LACPDUs (every 1 second), but the
> >actor only responds after receiving an LACPDU, which, in this setup, the
> >partner sends every 30 seconds due to the actor's lacp_rate=slow. By the time
> >the actor replies, the partner has already timed out and sent an "expired"
> >LACPDU.
> 
> 	Presuming that I'm correct that we're not implementing 6.4.1 d),
> above, correctly, then I don't think this is a proper fix, as it kind of
> band-aids over the problem a bit.
> 
> 	Looking at the code, I suspect the problem revolves around the
> "lacp_active" check in ad_periodic_machine():
> 
> static void ad_periodic_machine(struct port *port, struct bond_params *bond_params)
> {
> 	periodic_states_t last_state;
> 
> 	/* keep current state machine state to compare later if it was changed */
> 	last_state = port->sm_periodic_state;
> 
> 	/* check if port was reinitialized */
> 	if (((port->sm_vars & AD_PORT_BEGIN) || !(port->sm_vars & AD_PORT_LACP_ENABLED) || !port->is_enabled) ||
> 	    (!(port->actor_oper_port_state & LACP_STATE_LACP_ACTIVITY) && !(port->partner_oper.port_state & LACP_STATE_LACP_ACTIVITY)) ||
> 	    !bond_params->lacp_active) {
> 		port->sm_periodic_state = AD_NO_PERIODIC;
> 	}
> 
> 	In the above, because all the tests are chained with ||, the
> lacp_active test overrides the two correct-looking
> LACP_STATE_LACP_ACTIVITY tests.
> 
> 	It looks like ad_initialize_port() always sets
> LACP_STATE_LACP_ACTIVITY in the port->actor_oper_port_state, and nothing
> ever clears it.
> 
> 	Thinking out loud, perhaps this could be fixed by
> 
> 	a) remove the test of bond_params->lacp_active here, and,
> 
> 	b) The lacp_active option setting controls whether LACP_ACTIVITY
> is set in port->actor_oper_port_state.
> 
> 	Thoughts?

As the upper question. When should we stop sending the LACPDUs?

Thanks
Hangbin