[PATCH v1] tcp: fix wrong RTO timeout when received SACK reneging

[PATCH v1] tcp: fix wrong RTO timeout when received SACK reneging

[Fred Chen]

This commit fix wrong RTO timeout when received SACK reneging.

When an ACK arrived pointing to a SACK reneging, tcp_check_sack_reneging()
will rearm the RTO timer for min(1/2*srtt, 10ms) into to the future.

But since the commit 62d9f1a6945b ("tcp: fix TLP timer not set when
CA_STATE changes from DISORDER to OPEN") merged, the tcp_set_xmit_timer()
is moved after tcp_fastretrans_alert()(which do the SACK reneging check),
so the RTO timeout will be overwrited by tcp_set_xmit_timer() with
icsk_rto instead of 1/2*srtt.

Here is a packetdrill script to check this bug:
0     socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0    bind(3, ..., ...) = 0
+0    listen(3, 1) = 0

// simulate srtt to 100ms
+0    < S 0:0(0) win 32792 <mss 1000, sackOK,nop,nop,nop,wscale 7>
+0    > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
+.1    < . 1:1(0) ack 1 win 1024

+0    accept(3, ..., ...) = 4

+0    write(4, ..., 10000) = 10000
+0    > P. 1:10001(10000) ack 1

// inject sack
+.1    < . 1:1(0) ack 1 win 257 <sack 1001:10001,nop,nop>
+0    > . 1:1001(1000) ack 1

// inject sack reneging
+.1    < . 1:1(0) ack 1001 win 257 <sack 9001:10001,nop,nop>

// we expect rto fired in 1/2*srtt (50ms)
+.05    > . 1001:2001(1000) ack 1

This fix remove the FLAG_SET_XMIT_TIMER from ack_flag when
tcp_check_sack_reneging() set RTO timer with 1/2*srtt to avoid
being overwrited later.

Fixes: 62d9f1a6945b ("tcp: fix TLP timer not set when CA_STATE changes from DISORDER to OPEN")
Signed-off-by: Fred Chen <fred.chenchen03@gmail.com>
---
 net/ipv4/tcp_input.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index ab87f02..eee4e95 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2222,16 +2222,17 @@ void tcp_enter_loss(struct sock *sk)
  * restore sanity to the SACK scoreboard. If the apparent reneging
  * persists until this RTO then we'll clear the SACK scoreboard.
  */
-static bool tcp_check_sack_reneging(struct sock *sk, int flag)
+static bool tcp_check_sack_reneging(struct sock *sk, int *ack_flag)
 {
-	if (flag & FLAG_SACK_RENEGING &&
-	    flag & FLAG_SND_UNA_ADVANCED) {
+	if (*ack_flag & FLAG_SACK_RENEGING &&
+	    *ack_flag & FLAG_SND_UNA_ADVANCED) {
 		struct tcp_sock *tp = tcp_sk(sk);
 		unsigned long delay = max(usecs_to_jiffies(tp->srtt_us >> 4),
 					  msecs_to_jiffies(10));
 
 		inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
 					  delay, TCP_RTO_MAX);
+		*ack_flag &= ~FLAG_SET_XMIT_TIMER;
 		return true;
 	}
 	return false;
@@ -3009,7 +3010,7 @@ static void tcp_fastretrans_alert(struct sock *sk, const u32 prior_snd_una,
 		tp->prior_ssthresh = 0;
 
 	/* B. In all the states check for reneging SACKs. */
-	if (tcp_check_sack_reneging(sk, flag))
+	if (tcp_check_sack_reneging(sk, ack_flag))
 		return;
 
 	/* C. Check consistency of the current state. */
-- 
1.8.3.1

Re: [PATCH v1] tcp: fix wrong RTO timeout when received SACK reneging

[Neal Cardwell]

On Fri, Oct 20, 2023 at 8:20 PM Fred Chen <fred.chenchen03@gmail.com> wrote:
>
> This commit fix wrong RTO timeout when received SACK reneging.
>
> When an ACK arrived pointing to a SACK reneging, tcp_check_sack_reneging()
> will rearm the RTO timer for min(1/2*srtt, 10ms) into to the future.
>
> But since the commit 62d9f1a6945b ("tcp: fix TLP timer not set when
> CA_STATE changes from DISORDER to OPEN") merged, the tcp_set_xmit_timer()
> is moved after tcp_fastretrans_alert()(which do the SACK reneging check),
> so the RTO timeout will be overwrited by tcp_set_xmit_timer() with
> icsk_rto instead of 1/2*srtt.
>
> Here is a packetdrill script to check this bug:
> 0     socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0    bind(3, ..., ...) = 0
> +0    listen(3, 1) = 0
>
> // simulate srtt to 100ms
> +0    < S 0:0(0) win 32792 <mss 1000, sackOK,nop,nop,nop,wscale 7>
> +0    > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7>
> +.1    < . 1:1(0) ack 1 win 1024
>
> +0    accept(3, ..., ...) = 4
>
> +0    write(4, ..., 10000) = 10000
> +0    > P. 1:10001(10000) ack 1
>
> // inject sack
> +.1    < . 1:1(0) ack 1 win 257 <sack 1001:10001,nop,nop>
> +0    > . 1:1001(1000) ack 1
>
> // inject sack reneging
> +.1    < . 1:1(0) ack 1001 win 257 <sack 9001:10001,nop,nop>
>
> // we expect rto fired in 1/2*srtt (50ms)
> +.05    > . 1001:2001(1000) ack 1
>
> This fix remove the FLAG_SET_XMIT_TIMER from ack_flag when
> tcp_check_sack_reneging() set RTO timer with 1/2*srtt to avoid
> being overwrited later.
>
> Fixes: 62d9f1a6945b ("tcp: fix TLP timer not set when CA_STATE changes from DISORDER to OPEN")
> Signed-off-by: Fred Chen <fred.chenchen03@gmail.com>
> ---
>  net/ipv4/tcp_input.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index ab87f02..eee4e95 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -2222,16 +2222,17 @@ void tcp_enter_loss(struct sock *sk)
>   * restore sanity to the SACK scoreboard. If the apparent reneging
>   * persists until this RTO then we'll clear the SACK scoreboard.
>   */
> -static bool tcp_check_sack_reneging(struct sock *sk, int flag)
> +static bool tcp_check_sack_reneging(struct sock *sk, int *ack_flag)
>  {
> -       if (flag & FLAG_SACK_RENEGING &&
> -           flag & FLAG_SND_UNA_ADVANCED) {
> +       if (*ack_flag & FLAG_SACK_RENEGING &&
> +           *ack_flag & FLAG_SND_UNA_ADVANCED) {
>                 struct tcp_sock *tp = tcp_sk(sk);
>                 unsigned long delay = max(usecs_to_jiffies(tp->srtt_us >> 4),
>                                           msecs_to_jiffies(10));
>
>                 inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
>                                           delay, TCP_RTO_MAX);
> +               *ack_flag &= ~FLAG_SET_XMIT_TIMER;
>                 return true;
>         }
>         return false;
> @@ -3009,7 +3010,7 @@ static void tcp_fastretrans_alert(struct sock *sk, const u32 prior_snd_una,
>                 tp->prior_ssthresh = 0;
>
>         /* B. In all the states check for reneging SACKs. */
> -       if (tcp_check_sack_reneging(sk, flag))
> +       if (tcp_check_sack_reneging(sk, ack_flag))
>                 return;
>
>         /* C. Check consistency of the current state. */
> --

Thanks a lot for the fix! The code looks good to me, and I ran it
through our internal packetdrill test suite, and, with a few expected
tweaks to reflect the fix, the tests all pass.

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

thanks,
neal

Re: [PATCH v1] tcp: fix wrong RTO timeout when received SACK reneging

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Sat, 21 Oct 2023 08:19:47 +0800 you wrote:
> This commit fix wrong RTO timeout when received SACK reneging.
> 
> When an ACK arrived pointing to a SACK reneging, tcp_check_sack_reneging()
> will rearm the RTO timer for min(1/2*srtt, 10ms) into to the future.
> 
> But since the commit 62d9f1a6945b ("tcp: fix TLP timer not set when
> CA_STATE changes from DISORDER to OPEN") merged, the tcp_set_xmit_timer()
> is moved after tcp_fastretrans_alert()(which do the SACK reneging check),
> so the RTO timeout will be overwrited by tcp_set_xmit_timer() with
> icsk_rto instead of 1/2*srtt.
> 
> [...]

Here is the summary with links:
  - [v1] tcp: fix wrong RTO timeout when received SACK reneging
    https://git.kernel.org/netdev/net/c/d2a0fc372aca

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html