[PATCH net] tcp: fix fastopen code vs usec TS

[PATCH net] tcp: fix fastopen code vs usec TS

[Eric Dumazet]

After blamed commit, TFO client-ack-dropped-then-recovery-ms-timestamps
packetdrill test failed.

David Morley and Neal Cardwell started investigating and Neal pointed
that we had :

tcp_conn_request()
  tcp_try_fastopen()
   -> tcp_fastopen_create_child
     -> child = inet_csk(sk)->icsk_af_ops->syn_recv_sock()
       -> tcp_create_openreq_child()
          -> copy req_usec_ts from req:
          newtp->tcp_usec_ts = treq->req_usec_ts;
          // now the new TFO server socket always does usec TS, no matter
          // what the route options are...
  send_synack()
    -> tcp_make_synack()
        // disable tcp_rsk(req)->req_usec_ts if route option is not present:
        if (tcp_rsk(req)->req_usec_ts < 0)
                tcp_rsk(req)->req_usec_ts = dst_tcp_usec_ts(dst);

tcp_conn_request() has the initial dst, we can initialize
tcp_rsk(req)->req_usec_ts there instead of later in send_synack();

This means tcp_rsk(req)->req_usec_ts can be a boolean.

Many thanks to David an Neal for their help.

Fixes: 614e8316aa4c ("tcp: add support for usec resolution in TCP TS values")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202310302216.f79d78bc-oliver.sang@intel.com
Suggested-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Morley <morleyd@google.com>
---
 include/linux/tcp.h   | 2 +-
 net/ipv4/syncookies.c | 2 +-
 net/ipv4/tcp_input.c  | 7 ++++---
 net/ipv4/tcp_output.c | 2 --
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index ec4e9367f5b03be610f5f88621855f3a512604eb..68f3d315d2e18d93a356b0738e4ed855fac94591 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -152,7 +152,7 @@ struct tcp_request_sock {
 	u64				snt_synack; /* first SYNACK sent time */
 	bool				tfo_listener;
 	bool				is_mptcp;
-	s8				req_usec_ts;
+	bool				req_usec_ts;
 #if IS_ENABLED(CONFIG_MPTCP)
 	bool				drop_req;
 #endif
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 98b25e5d147bac5262982681b0bc5b38434a473a..d37282c06e3da05fd36c48e6b4236d74ac2b7fe2 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -306,7 +306,7 @@ struct request_sock *cookie_tcp_reqsk_alloc(const struct request_sock_ops *ops,
 	treq->af_specific = af_ops;
 
 	treq->syn_tos = TCP_SKB_CB(skb)->ip_dsfield;
-	treq->req_usec_ts = -1;
+	treq->req_usec_ts = false;
 
 #if IS_ENABLED(CONFIG_MPTCP)
 	treq->is_mptcp = sk_is_mptcp(sk);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 50aaa1527150bd8adabce125775aab8b97018d53..bcb55d98004c5213f0095613124d5193b15b2793 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -7115,7 +7115,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	req->syncookie = want_cookie;
 	tcp_rsk(req)->af_specific = af_ops;
 	tcp_rsk(req)->ts_off = 0;
-	tcp_rsk(req)->req_usec_ts = -1;
+	tcp_rsk(req)->req_usec_ts = false;
 #if IS_ENABLED(CONFIG_MPTCP)
 	tcp_rsk(req)->is_mptcp = 0;
 #endif
@@ -7143,9 +7143,10 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	if (!dst)
 		goto drop_and_free;
 
-	if (tmp_opt.tstamp_ok)
+	if (tmp_opt.tstamp_ok) {
+		tcp_rsk(req)->req_usec_ts = dst_tcp_usec_ts(dst);
 		tcp_rsk(req)->ts_off = af_ops->init_ts_off(net, skb);
-
+	}
 	if (!want_cookie && !isn) {
 		int max_syn_backlog = READ_ONCE(net->ipv4.sysctl_max_syn_backlog);
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f558c054cf6e7538ecc3d711637af0bd44872318..0d8dd5b7e2e5e078d3e4bcc0d4270215f1be366d 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3693,8 +3693,6 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
 	mss = tcp_mss_clamp(tp, dst_metric_advmss(dst));
 
 	memset(&opts, 0, sizeof(opts));
-	if (tcp_rsk(req)->req_usec_ts < 0)
-		tcp_rsk(req)->req_usec_ts = dst_tcp_usec_ts(dst);
 	now = tcp_clock_ns();
 #ifdef CONFIG_SYN_COOKIES
 	if (unlikely(synack_type == TCP_SYNACK_COOKIE && ireq->tstamp_ok))
-- 
2.42.0.820.g83a721a137-goog

Re: [PATCH net] tcp: fix fastopen code vs usec TS

[Neal Cardwell]

On Tue, Oct 31, 2023 at 2:19 AM Eric Dumazet <edumazet@google.com> wrote:
>
> After blamed commit, TFO client-ack-dropped-then-recovery-ms-timestamps
> packetdrill test failed.
>
> David Morley and Neal Cardwell started investigating and Neal pointed
> that we had :
>
> tcp_conn_request()
>   tcp_try_fastopen()
>    -> tcp_fastopen_create_child
>      -> child = inet_csk(sk)->icsk_af_ops->syn_recv_sock()
>        -> tcp_create_openreq_child()
>           -> copy req_usec_ts from req:
>           newtp->tcp_usec_ts = treq->req_usec_ts;
>           // now the new TFO server socket always does usec TS, no matter
>           // what the route options are...
>   send_synack()
>     -> tcp_make_synack()
>         // disable tcp_rsk(req)->req_usec_ts if route option is not present:
>         if (tcp_rsk(req)->req_usec_ts < 0)
>                 tcp_rsk(req)->req_usec_ts = dst_tcp_usec_ts(dst);
>
> tcp_conn_request() has the initial dst, we can initialize
> tcp_rsk(req)->req_usec_ts there instead of later in send_synack();
>
> This means tcp_rsk(req)->req_usec_ts can be a boolean.
>
> Many thanks to David an Neal for their help.
>
> Fixes: 614e8316aa4c ("tcp: add support for usec resolution in TCP TS values")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202310302216.f79d78bc-oliver.sang@intel.com
> Suggested-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Morley <morleyd@google.com>
> ---

Looks great to me. Thanks, Eric!

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

neal

Re: [PATCH net] tcp: fix fastopen code vs usec TS

[patchwork-bot+netdevbpf]

Hello:

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

On Tue, 31 Oct 2023 06:19:45 +0000 you wrote:
> After blamed commit, TFO client-ack-dropped-then-recovery-ms-timestamps
> packetdrill test failed.
> 
> David Morley and Neal Cardwell started investigating and Neal pointed
> that we had :
> 
> tcp_conn_request()
>   tcp_try_fastopen()
>    -> tcp_fastopen_create_child
>      -> child = inet_csk(sk)->icsk_af_ops->syn_recv_sock()
>        -> tcp_create_openreq_child()
>           -> copy req_usec_ts from req:
>           newtp->tcp_usec_ts = treq->req_usec_ts;
>           // now the new TFO server socket always does usec TS, no matter
>           // what the route options are...
>   send_synack()
>     -> tcp_make_synack()
>         // disable tcp_rsk(req)->req_usec_ts if route option is not present:
>         if (tcp_rsk(req)->req_usec_ts < 0)
>                 tcp_rsk(req)->req_usec_ts = dst_tcp_usec_ts(dst);
> 
> [...]

Here is the summary with links:
  - [net] tcp: fix fastopen code vs usec TS
    https://git.kernel.org/netdev/net/c/cdbab6236605

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