Hi Eric,
At least the FreeBSD stack has got the fix (and it will be in the widely used FBSD 11.4 release). MacOS had fixed this independently for a long time, but it was really all introduced back in 2006 or so, when the various *BSD variants got their ECN support implemented - literally the very same code lines were shared among many variants back then. And I fully agree, that it will take many years for this fix to percolate through the the install base - but with the fix, people who want to start using old 3168 type ECN can at least look which patch they can more easily deploy / backport. BSD stacks often run in closed source software, so though luck, unless the vendor of that appliance gets the freebsd patch (https://reviews.freebsd.org/D23364. Releng/11.4 https://reviews.freebsd.org/rS361565). So tackling this from both side (especially as patching Linux is often easier when deployed) is definitely helping getting that problem addressed more quickly. Thanks! Richard Scheffenegger Consulting Solution Architect NAS & Networking NetApp +43 1 3676 811 3157 Direct Phone +43 664 8866 1857 Mobile Phone [email protected] https://ts.la/richard49892 -----Original Message----- From: Eric Dumazet <[email protected]> Sent: Mittwoch, 24. Juni 2020 17:47 To: Neal Cardwell <[email protected]> Cc: Denis Kirjanov <[email protected]>; Netdev <[email protected]>; Yuchung Cheng <[email protected]>; Scheffenegger, Richard <[email protected]>; Bob Briscoe <[email protected]> Subject: Re: [PATCH v2] tcp: don't ignore ECN CWR on pure ACK NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe. On Wed, Jun 24, 2020 at 6:43 AM Neal Cardwell <[email protected]> wrote: > > On Wed, Jun 24, 2020 at 5:58 AM Denis Kirjanov <[email protected]> wrote: > > > > there is a problem with the CWR flag set in an incoming ACK segment > > and it leads to the situation when the ECE flag is latched forever > > > > the following packetdrill script shows what happens: > > > > // Stack receives incoming segments with CE set > > +0.1 <[ect0] . 11001:12001(1000) ack 1001 win 65535 > > +0.0 <[ce] . 12001:13001(1000) ack 1001 win 65535 > > +0.0 <[ect0] P. 13001:14001(1000) ack 1001 win 65535 > > > > // Stack repsonds with ECN ECHO > > +0.0 >[noecn] . 1001:1001(0) ack 12001 > > +0.0 >[noecn] E. 1001:1001(0) ack 13001 > > +0.0 >[noecn] E. 1001:1001(0) ack 14001 > > > > // Write a packet > > +0.1 write(3, ..., 1000) = 1000 > > +0.0 >[ect0] PE. 1001:2001(1000) ack 14001 > > > > // Pure ACK received > > +0.01 <[noecn] W. 14001:14001(0) ack 2001 win 65535 > > > > // Since CWR was sent, this packet should NOT have ECE set > > > > +0.1 write(3, ..., 1000) = 1000 > > +0.0 >[ect0] P. 2001:3001(1000) ack 14001 > > // but Linux will still keep ECE latched here, with packetdrill // > > flagging a missing ECE flag, expecting // >[ect0] PE. > > 2001:3001(1000) ack 14001 // in the script > > > > In the situation above we will continue to send ECN ECHO packets and > > trigger the peer to reduce the congestion window. To avoid that we > > can check CWR on pure ACKs received. > > > > v2: > > - Adjusted the comment > > - move CWR check before checking for unacknowledged packets > > > > Signed-off-by: Denis Kirjanov <[email protected]> > > --- > > net/ipv4/tcp_input.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index > > 12fda8f27b08..f1936c0cb684 100644 > > --- a/net/ipv4/tcp_input.c > > +++ b/net/ipv4/tcp_input.c > > @@ -3665,6 +3665,15 @@ static int tcp_ack(struct sock *sk, const struct > > sk_buff *skb, int flag) > > tcp_in_ack_event(sk, ack_ev_flags); > > } > > > > + /* This is a deviation from RFC3168 since it states that: > > + * "When the TCP data sender is ready to set the CWR bit after > > reducing > > + * the congestion window, it SHOULD set the CWR bit only on the > > first > > + * new data packet that it transmits." > > + * We accept CWR on pure ACKs to be more robust > > + * with widely-deployed TCP implementations that do this. > > + */ > > + tcp_ecn_accept_cwr(sk, skb); > > + > > /* We passed data and got it acked, remove any soft error > > * log. Something worked... > > */ > > @@ -4800,8 +4809,6 @@ static void tcp_data_queue(struct sock *sk, struct > > sk_buff *skb) > > skb_dst_drop(skb); > > __skb_pull(skb, tcp_hdr(skb)->doff * 4); > > > > - tcp_ecn_accept_cwr(sk, skb); > > - > > tp->rx_opt.dsack = 0; > > > > /* Queue data for delivery to the user. > > -- > > Thanks for the patch! > > Acked-by: Neal Cardwell <[email protected]> > Hmm... It would be nice maybe to fix the offenders, because many linux devices won't get this work around before years. Do we really want to trigger an ACK if we received a packet with no payload ? I would think that the following is also needed : diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 12fda8f27b08bdf5c9f3bad422734f6b1965cef9..023dc90569c89d7d17d72f73641598a03a03b0a9 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -261,7 +261,8 @@ static void tcp_ecn_accept_cwr(struct sock *sk, const struct sk_buff *skb) * cwnd may be very low (even just 1 packet), so we should ACK * immediately. */ - inet_csk(sk)->icsk_ack.pending |= ICSK_ACK_NOW; + if (TCP_SKB_CB(skb)->seq != TCP_SKB_CB(skb)->end_seq) + inet_csk(sk)->icsk_ack.pending |= ICSK_ACK_NOW; } }
