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

[Jay Vosburgh <jv@jvosburgh.net>]

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

>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.
>
>As a result, upper-layer daemons consider the interface operational
>while traffic is effectively blackholed.
>
>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 | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index af7f74cfdc08..7d7661972c5e 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;
>+}

	Did you consider instead rolling the COLLECTING / DISTRIBUTING
test into __agg_active_ports?  It appears at first glance that the users
of __agg_active_ports would do the right thing with the "able to
communicate" logic.

	As an example, logically the aggregator selection logic should
count "usable" ports, not simply ports that are enabled and may or may
not be usable.

	-J

>+
> 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,8 @@ 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