Re: [PATCH net v2 1/3] bonding: 3ad: fix carrier when no valid slaves

[Jay Vosburgh <jv@jvosburgh.net>]

Louis Scalbert <louis.scalbert@6wind.com> wrote:

>Hi everyone,
>
>Le mer. 25 mars 2026 à 14:44, Louis Scalbert
><louis.scalbert@6wind.com> a écrit :
>>
>> When an 802.3ad (LACP) bonding interface has no slaves in the
>> collecting/distributing state, the bonding master may still report
>> carrier as up. In this situation, no slave is actually able to transmit
>> or receive traffic.
>>
>I’ve just realized that the issue description is not entirely accurate.
>
>Bonding links that are not in LACP Collecting/Distributing state are
>excluded from traffic
>distribution in favor of those that are. However, if no links are in
>Collecting/Distributing state,
>the kernel considers all links as valid for distributing traffic.
>
>> As a result, upper-layer daemons consider the interface operational
>> while traffic is effectively blackholed.
>
>There are situations where traffic is not blackholed.
>
>For example, a Linux machine A is connected to machine B using two
>aggregated links.
>On A, LACP is configured, but not on B. As a result, LACP will not
>reach the Collecting/Distributing
>state since B does not send LACP frames. However, the bonding
>interface will still be up on both
>sides, and traffic can be exchanged.

	Bonding does this because, in the absense of a LACP partner, the
interfaces in the bond will become Individual links, each of which will
become its own aggregator, one of which will be chosen to be active.

	This is in compliance with the standard, (802.1AX-2014, 6.1.1.j)
and is intended to permit some communication between a system that is
LACP-enabled and a system that is not.  The common example I'm familiar
with is a host that PXE boots over a network, connected to a
LACP-enabled switch.  During early boot, the PXE environment does not
perform LACP, but still needs to communicate with the switch.

>This behavior is, of course, not compliant with the LACP (802.3ad)
>standard, but applying this fix
>as-is could introduce regressions in some setups. Under normal
>conditions, having no Collecting /
>Distributing links means that no interfaces are operational for
>handling traffic - that is precisely the
>purpose of using LACP.

	FWIW, generally speaking we've settled on IEEE 802.1AX-2014 as
the version of the standard to which the bonding implementation should
be conformant.  There is a more recent version, -2020, but it is a
significant change from the prior versions.

>I will publish a new version with a knob to enforce compliance with
>the LACP standard. It will be
>disabled by default for backward compatibility.
>
>What do you think of "lacp_strict" or "lacp_enforce" ?

	I'm not convinced that we are in violation of the standard with
the present behavior.  That said, if such a switch is necessary, I'd
vote for "lacp_strict".

	-J

>>
>> Fix this by asserting carrier only when at least 'min_links' slaves are
>> in the collecting/distributing state (or collecting only if the
>> coupled_control default behavior is disabled).
>>
>> Fixes: 655f8919d549 ("bonding: add min links parameter to 802.3ad")
>> Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
>> ---
>>  drivers/net/bonding/bond_3ad.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index af7f74cfdc08..6d3613755d45 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -745,6 +745,22 @@ static void __set_agg_ports_ready(struct aggregator *aggregator, int val)
>>         }
>>  }
>>
>> +static int __agg_valid_ports(struct aggregator *agg)
>> +{
>> +       struct port *port;
>> +       int valid = 0;
>> +
>> +       for (port = agg->lag_ports; port;
>> +            port = port->next_port_in_aggregator) {
>> +               if (port->actor_oper_port_state & LACP_STATE_COLLECTING &&
>> +                   (!port->slave->bond->params.coupled_control ||
>> +                    port->actor_oper_port_state & LACP_STATE_DISTRIBUTING))
>> +                       valid++;
>> +       }
>> +
>> +       return valid;
>> +}
>> +
>>  static int __agg_active_ports(struct aggregator *agg)
>>  {
>>         struct port *port;
>> @@ -2120,6 +2136,7 @@ static void ad_enable_collecting_distributing(struct port *port,
>>                           port->actor_port_number,
>>                           port->aggregator->aggregator_identifier);
>>                 __enable_port(port);
>> +               bond_3ad_set_carrier(port->slave->bond);
>>                 /* Slave array needs update */
>>                 *update_slave_arr = true;
>>                 /* Should notify peers if possible */
>> @@ -2141,6 +2158,7 @@ static void ad_disable_collecting_distributing(struct port *port,
>>                           port->actor_port_number,
>>                           port->aggregator->aggregator_identifier);
>>                 __disable_port(port);
>> +               bond_3ad_set_carrier(port->slave->bond);
>>                 /* Slave array needs an update */
>>                 *update_slave_arr = true;
>>         }
>> @@ -2819,8 +2837,10 @@ int bond_3ad_set_carrier(struct bonding *bond)
>>         }
>>         active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
>>         if (active) {
>> -               /* are enough slaves available to consider link up? */
>> -               if (__agg_active_ports(active) < bond->params.min_links) {
>> +               /* are enough slaves in collecting (and distributing) state to consider
>> +                * link up?
>> +                */
>> +               if (__agg_valid_ports(active) < bond->params.min_links) {
>>                         if (netif_carrier_ok(bond->dev)) {
>>                                 netif_carrier_off(bond->dev);
>>                                 goto out;
>> --
>> 2.39.2
>>

---
	-Jay Vosburgh, jv@jvosburgh.net