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

Louis Scalbert [off-list ref] wrote:

Hi everyone,

Le mer. 25 mars 2026 à 14:44, Louis Scalbert
[off-list ref] a écrit :

quoted

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.

quoted

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

quoted

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