[PATCH v3] tcp: don't ignore ECN CWR on pure ACK

[PATCH v3] tcp: don't ignore ECN CWR on pure ACK

[Denis Kirjanov]

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.

v3:
- Add a sequence check to avoid sending an ACK to an ACK

v2:
- Adjusted the comment
- move CWR check before checking for unacknowledged packets

Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
---
 net/ipv4/tcp_input.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 12fda8f27b08..f3a0eb139b76 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;
 	}
 }
 
@@ -3665,6 +3666,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 +4810,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.
-- 
2.27.0

Re: [PATCH v3] tcp: don't ignore ECN CWR on pure ACK

[Neal Cardwell]

On Thu, Jun 25, 2020 at 7:51 AM Denis Kirjanov <kda@linux-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.
>
> v3:
> - Add a sequence check to avoid sending an ACK to an ACK
>
> v2:
> - Adjusted the comment
> - move CWR check before checking for unacknowledged packets
>
> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
> ---
>  net/ipv4/tcp_input.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 12fda8f27b08..f3a0eb139b76 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;
>         }
>  }
>
> @@ -3665,6 +3666,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 +4810,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, Denis!

Acked-by: Neal Cardwell <ncardwell@google.com>

neal

Re: [PATCH v3] tcp: don't ignore ECN CWR on pure ACK

[Eric Dumazet]

On Thu, Jun 25, 2020 at 5:51 AM Neal Cardwell <ncardwell@google.com> wrote:
>
> On Thu, Jun 25, 2020 at 7:51 AM Denis Kirjanov <kda@linux-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
> >

> > Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>

> Thanks, Denis!
>
> Acked-by: Neal Cardwell <ncardwell@google.com>
>
> neal

Signed-off-by: Eric Dumazet <edumazet@google.com>

Thanks

Re: [PATCH v3] tcp: don't ignore ECN CWR on pure ACK

[David Miller]

From: Denis Kirjanov <kda@linux-powerpc.org>
Date: Thu, 25 Jun 2020 14:51:06 +0300

> 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:
 ...
> 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.
> 
> v3:
> - Add a sequence check to avoid sending an ACK to an ACK
> 
> v2:
> - Adjusted the comment
> - move CWR check before checking for unacknowledged packets
> 
> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>

Applied, thanks.