Message-ID: <SN4PR0601MB3728C60F00DB0781FF098F4286950@SN4PR0601MB3728.namprd06.prod.outlook.com>
Date:   Wed, 24 Jun 2020 16:00:12 +0000
From:   "Scheffenegger, Richard" <Richard.Scheffenegger@...app.com>
To:     Eric Dumazet <edumazet@...gle.com>,
        Neal Cardwell <ncardwell@...gle.com>
CC:     Denis Kirjanov <kda@...ux-powerpc.org>,
        Netdev <netdev@...r.kernel.org>,
        Yuchung Cheng <ycheng@...gle.com>,
        Bob Briscoe <ietf@...briscoe.net>
Subject: RE: [PATCH v2] tcp: don't ignore ECN CWR on pure ACK

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
Richard.Scheffenegger@...app.com

https://ts.la/richard49892


-----Original Message-----
From: Eric Dumazet <edumazet@...gle.com> 
Sent: Mittwoch, 24. Juni 2020 17:47
To: Neal Cardwell <ncardwell@...gle.com>
Cc: Denis Kirjanov <kda@...ux-powerpc.org>; Netdev <netdev@...r.kernel.org>; Yuchung Cheng <ycheng@...gle.com>; Scheffenegger, Richard <Richard.Scheffenegger@...app.com>; Bob Briscoe <ietf@...briscoe.net>
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 <ncardwell@...gle.com> wrote:
>
> On Wed, Jun 24, 2020 at 5:58 AM Denis Kirjanov <kda@...ux-powerpc.org> 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 <denis.kirjanov@...e.com>
> > ---
> >  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 <ncardwell@...gle.com>
>

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;
        }
 }

Powered by blists - more mailing lists