netdev - Re: [PATCH net] bonding: fix 802.3ad state sent to partner when unbinding slave

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] [day] [month] [year] [list]

Message-ID: <20181127114427.GA4458@peltzi-E7470.localdomain>
Date:   Tue, 27 Nov 2018 13:44:27 +0200
From:   Toni Peltonen <peltzi@...tzi.fi>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] bonding: fix 802.3ad state sent to partner when
 unbinding slave

On Mon, Nov 26, 2018 at 04:29:58PM -0800, Jay Vosburgh wrote:
> Toni Peltonen <peltzi@...tzi.fi> wrote:
> 
> >Previously when unbinding a slave the 802.3ad implementation only told
> >partner that the port is not suitable for aggregation by setting the port
> >aggregation state from aggregatable to individual. This is not enough. If the
> >physical layer still stays up and we only unbinded this port from the bond there
> >is nothing in the aggregation status alone to prevent the partner from sending
> >traffic towards us. To ensure that the partner doesn't consider this
> >port at all anymore we should also disable collecting and distributing to
> >signal that this actor is going away.
> >
> >I have tested this behaviour againts Arista EOS switches with mlx5 cards
> >(physical link stays up when even when interface is down) and simulated
> >the same situation virtually Linux <-> Linux with two network namespaces
> >running two veth device pairs. In both cases setting aggregation to
> >individual doesn't alone prevent traffic from being to sent towards this
> >port given that the link stays up in partners end. Partner still keeps
> >it's end in collecting + distributing state and continues until timeout is
> >reached. In most cases this means we are losing the traffic partner sends
> >towards our port while we wait for timeout. This is most visible with slow
> >periodic time (LAPC rate slow).
> 
> 	"LAPC" -> "LACP"
> 

I will fix the typing error and send updated version.

> >Other open source implementations like Open VSwitch and libreswitch, and
> >vendor implementations like Arista EOS, seem to disable collecting +
> >distributing to when doing similar port disabling/detaching/removing change.
> >With this patch kernel implementation would behave the same way and ensure
> >partner doesn't consider our actor viable anymore.
> 
> 	After re-reading the relevant bits of 802.1AX (particularly
> 5.4.9 on recordPDU and update_Selected) I'm going to suggest also
> clearing AD_STATE_SYNCHRONIZATION, based on:
> 
> 	Partner_Oper_Port_State.Synchronization is also set to TRUE if
> 	the value of Actor_State.Aggregation in the received PDU is set
> 	to FALSE (i.e., indicates an Individual link),
> 	Actor_State.Synchronization in the received PDU is set to TRUE,
> 	and LACP will actively maintain the link.
> 
> 	Per the above, learing _SYNC in the LACPDU should un-sync the
> port, inducing the Mux state machine (figure 5-15) to exit C_D state and
> go to ATTACHED state (disabling Coll/Dist).
> 
> 	But, either way, as this is a hint to get the link partner to
> stop using the port, this looks reasonable to me.  

After looking more closely into that I agree. It seems that also
clearing the AD_STATE_SYNCHRONIZATION would make sense here. I will test
this and send a new version with it if it doesn't cause any side
effects.

> 
> Acked-by: Jay Vosburgh <jay.vosburgh@...onical.com>
> 
> 	-J	
> 
> >Signed-off-by: Toni Peltonen <peltzi@...tzi.fi>
> >---
> > drivers/net/bonding/bond_3ad.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> >diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> >index f43fb2f958a5..6776c33753dc 100644
> >--- a/drivers/net/bonding/bond_3ad.c
> >+++ b/drivers/net/bonding/bond_3ad.c
> >@@ -2086,6 +2086,8 @@ void bond_3ad_unbind_slave(struct slave *slave)
> > 		   aggregator->aggregator_identifier);
> > 
> > 	/* Tell the partner that this port is not suitable for aggregation */
> >+	port->actor_oper_port_state &= ~AD_STATE_COLLECTING;
> >+	port->actor_oper_port_state &= ~AD_STATE_DISTRIBUTING;
> > 	port->actor_oper_port_state &= ~AD_STATE_AGGREGATION;
> > 	__update_lacpdu_from_port(port);
> > 	ad_lacpdu_send(port);
> >-- 
> >2.19.0
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@...onical.com