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

On Fri, Mar 27, 2026 at 9:22 AM 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.

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.

The legacy behavior is to "maintain connectivity" by selecting a
functioning aggregator, as long as there are no link events. It's
crucial to maintain this legacy behavior.

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 don't have any specific preference for the name, but the default
value must comply with the legacy behavior.

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