netdev - Re: [PATCH v2] tcp: don't ignore ECN CWR on pure ACK

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

Date:   Wed, 24 Jun 2020 08:47:05 -0700
From:   Eric Dumazet <edumazet@...gle.com>
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

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