[PATCH net] tcp: make TCP_USER_TIMEOUT accurate for zero window probes

From: Enke Chen <redacted>

The TCP_USER_TIMEOUT is checked by the 0-window probe timer. As the
timer has backoff with a max interval of about two minutes, the
actual timeout for TCP_USER_TIMEOUT can be off by up to two minutes.

In this patch the TCP_USER_TIMEOUT is made more accurate by taking it
into account when computing the timer value for the 0-window probes.

This patch is similar to the one that made TCP_USER_TIMEOUT accurate for
RTOs in commit b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout()
helper to improve accuracy").

Signed-off-by: Enke Chen <redacted>
Reviewed-by: Neal Cardwell <ncardwell@google.com>
---
 include/net/tcp.h     |  1 +
 net/ipv4/tcp_input.c  |  4 ++--
 net/ipv4/tcp_output.c |  2 ++
 net/ipv4/tcp_timer.c  | 18 ++++++++++++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 78d13c88720f..ca7e2c6cc663 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h

@@ -630,6 +630,7 @@ static inline void tcp_clear_xmit_timers(struct sock *sk)
 
 unsigned int tcp_sync_mss(struct sock *sk, u32 pmtu);
 unsigned int tcp_current_mss(struct sock *sk);
+u32 tcp_clamp_probe0_to_user_timeout(const struct sock *sk, u32 when);
 
 /* Bound MSS / TSO packet size with the half of the window */
 static inline int tcp_bound_to_half_wnd(struct tcp_sock *tp, int pktsize)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bafcab75f425..4923cdbea95a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -3392,8 +3392,8 @@ static void tcp_ack_probe(struct sock *sk)
 	} else {
 		unsigned long when = tcp_probe0_when(sk, TCP_RTO_MAX);
 
-		tcp_reset_xmit_timer(sk, ICSK_TIME_PROBE0,
-				     when, TCP_RTO_MAX);
+		when = tcp_clamp_probe0_to_user_timeout(sk, when);
+		tcp_reset_xmit_timer(sk, ICSK_TIME_PROBE0, when, TCP_RTO_MAX);
 	}
 }
 

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index ab458697881e..8478cf749821 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c

@@ -4099,6 +4099,8 @@ void tcp_send_probe0(struct sock *sk)
 		 */
 		timeout = TCP_RESOURCE_PROBE_INTERVAL;
 	}
+
+	timeout = tcp_clamp_probe0_to_user_timeout(sk, timeout);
 	tcp_reset_xmit_timer(sk, ICSK_TIME_PROBE0, timeout, TCP_RTO_MAX);
 }
 

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 454732ecc8f3..90722e30ad90 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c

@@ -40,6 +40,24 @@ static u32 tcp_clamp_rto_to_user_timeout(const struct sock *sk)
 	return min_t(u32, icsk->icsk_rto, msecs_to_jiffies(remaining));
 }
 
+u32 tcp_clamp_probe0_to_user_timeout(const struct sock *sk, u32 when)
+{
+	struct inet_connection_sock *icsk = inet_csk(sk);
+	u32 remaining;
+	s32 elapsed;
+
+	if (!icsk->icsk_user_timeout || !icsk->icsk_probes_tstamp)
+		return when;
+
+	elapsed = tcp_jiffies32 - icsk->icsk_probes_tstamp;
+	if (unlikely(elapsed < 0))
+		elapsed = 0;
+	remaining = msecs_to_jiffies(icsk->icsk_user_timeout) - elapsed;
+	remaining = max_t(u32, remaining, TCP_TIMEOUT_MIN);
+
+	return min_t(u32, remaining, when);
+}
+
 /**
  *  tcp_write_err() - close socket and save error info
  *  @sk:  The socket the error has appeared on.

-- 
2.29.2

On Fri, Jan 22, 2021 at 8:13 PM Enke Chen [off-list ref] wrote:

From: Enke Chen <redacted>

The TCP_USER_TIMEOUT is checked by the 0-window probe timer. As the
timer has backoff with a max interval of about two minutes, the
actual timeout for TCP_USER_TIMEOUT can be off by up to two minutes.

In this patch the TCP_USER_TIMEOUT is made more accurate by taking it
into account when computing the timer value for the 0-window probes.

This patch is similar to the one that made TCP_USER_TIMEOUT accurate for
RTOs in commit b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout()
helper to improve accuracy").

Signed-off-by: Enke Chen <redacted>
Reviewed-by: Neal Cardwell <ncardwell@google.com>
---

SGTM, thanks !

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

On Fri, 22 Jan 2021 11:13:06 -0800 Enke Chen wrote:

From: Enke Chen <redacted>

The TCP_USER_TIMEOUT is checked by the 0-window probe timer. As the
timer has backoff with a max interval of about two minutes, the
actual timeout for TCP_USER_TIMEOUT can be off by up to two minutes.

In this patch the TCP_USER_TIMEOUT is made more accurate by taking it
into account when computing the timer value for the 0-window probes.

This patch is similar to the one that made TCP_USER_TIMEOUT accurate for
RTOs in commit b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout()
helper to improve accuracy").

Signed-off-by: Enke Chen <redacted>
Reviewed-by: Neal Cardwell <ncardwell@google.com>

This is targeting net, any guidance on Fixes / backporting?

Hi, Jakub:

In terms of backporting, this patch should go together with:

    9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

Thanks.  -- Enke

On Fri, Jan 22, 2021 at 05:43:25PM -0800, Jakub Kicinski wrote:

On Fri, 22 Jan 2021 11:13:06 -0800 Enke Chen wrote:

quoted

From: Enke Chen <redacted>

The TCP_USER_TIMEOUT is checked by the 0-window probe timer. As the
timer has backoff with a max interval of about two minutes, the
actual timeout for TCP_USER_TIMEOUT can be off by up to two minutes.

In this patch the TCP_USER_TIMEOUT is made more accurate by taking it
into account when computing the timer value for the 0-window probes.

This patch is similar to the one that made TCP_USER_TIMEOUT accurate for
RTOs in commit b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout()
helper to improve accuracy").

Signed-off-by: Enke Chen <redacted>
Reviewed-by: Neal Cardwell <ncardwell@google.com>

This is targeting net, any guidance on Fixes / backporting?

On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:

Hi, Jakub:

In terms of backporting, this patch should go together with:

    9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

As in it:

Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

or does it further fix the same issue, so:

Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

?

Hi, Jakub:

On Fri, Jan 22, 2021 at 06:34:24PM -0800, Jakub Kicinski wrote:

On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:

quoted

Hi, Jakub:

In terms of backporting, this patch should go together with:

    9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

As in it:

Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

or does it further fix the same issue, so:

Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

?

Let me clarify:

1) 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

   fixes the bug and makes it work.

2) The current patch makes the TCP_USER_TIMEOUT accurate for 0-window probes.
   It's independent.

With 1) and 2), the known issues with TCP_USER_TIMEOUT for 0-window probes
would be resolved.

Thanks.   -- Enke

On Fri, Jan 22, 2021 at 9:45 PM Enke Chen [off-list ref] wrote:

Hi, Jakub:

On Fri, Jan 22, 2021 at 06:34:24PM -0800, Jakub Kicinski wrote:

quoted

On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:

quoted

Hi, Jakub:

In terms of backporting, this patch should go together with:

    9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

As in it:

Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

or does it further fix the same issue, so:

Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

?

Let me clarify:

1) 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

   fixes the bug and makes it work.

2) The current patch makes the TCP_USER_TIMEOUT accurate for 0-window probes.
   It's independent.

Patch (2) ("tcp: make TCP_USER_TIMEOUT accurate for zero window
probes") is indeed conceptually independent of (1) but its
implementation depends on the icsk_probes_tstamp field defined in (1),
so AFAICT (2) cannot be backported further back than (1).

Patch (1) fixes a bug in 5.1:
    Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

So probably (1) and (2) should be backported as a pair, and only back
as far as 5.1. (That covers 2 LTS kernels, 5.4 and 5.10, so hopefully
that is good enough.)

neal

Hi, Neal:

What you described is more accurate, and is correct.

Thanks.  -- Enke

On Sat, Jan 23, 2021 at 07:19:13PM -0500, Neal Cardwell wrote:

On Fri, Jan 22, 2021 at 9:45 PM Enke Chen [off-list ref] wrote:

quoted

Hi, Jakub:

On Fri, Jan 22, 2021 at 06:34:24PM -0800, Jakub Kicinski wrote:

quoted

On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:

quoted

Hi, Jakub:

In terms of backporting, this patch should go together with:

    9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

As in it:

Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

or does it further fix the same issue, so:

Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

?

Let me clarify:

1) 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

   fixes the bug and makes it work.

2) The current patch makes the TCP_USER_TIMEOUT accurate for 0-window probes.
   It's independent.

Patch (2) ("tcp: make TCP_USER_TIMEOUT accurate for zero window
probes") is indeed conceptually independent of (1) but its
implementation depends on the icsk_probes_tstamp field defined in (1),
so AFAICT (2) cannot be backported further back than (1).

Patch (1) fixes a bug in 5.1:
    Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

So probably (1) and (2) should be backported as a pair, and only back
as far as 5.1. (That covers 2 LTS kernels, 5.4 and 5.10, so hopefully
that is good enough.)

neal

On Sat, 23 Jan 2021 16:56:43 -0800 Enke Chen wrote:

On Sat, Jan 23, 2021 at 07:19:13PM -0500, Neal Cardwell wrote:

quoted

On Fri, Jan 22, 2021 at 9:45 PM Enke Chen [off-list ref] wrote:  

quoted

On Fri, Jan 22, 2021 at 06:34:24PM -0800, Jakub Kicinski wrote:  

quoted

On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:  

quoted

In terms of backporting, this patch should go together with:

    9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window  

As in it:

Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

or does it further fix the same issue, so:

Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

?  

Let me clarify:

1) 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

   fixes the bug and makes it work.

2) The current patch makes the TCP_USER_TIMEOUT accurate for 0-window probes.
   It's independent.  

Patch (2) ("tcp: make TCP_USER_TIMEOUT accurate for zero window
probes") is indeed conceptually independent of (1) but its
implementation depends on the icsk_probes_tstamp field defined in (1),
so AFAICT (2) cannot be backported further back than (1).

Patch (1) fixes a bug in 5.1:
    Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

So probably (1) and (2) should be backported as a pair, and only back
as far as 5.1. (That covers 2 LTS kernels, 5.4 and 5.10, so hopefully
that is good enough.)

What you described is more accurate, and is correct.

That makes it clear.

I added a Fixes tag, reworded the message slightly and applied, thanks!