[PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

When setting RTO through BPF program, SYN ACK packets were unaffected and
continued to use TCP_TIMEOUT_INIT constant. This patch makes SYN ACK
retransmits use tcp_timeout_init() function instead.

Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
---
 net/ipv4/inet_connection_sock.c | 2 +-
 net/ipv4/tcp_minisocks.c        | 4 ++--
 net/ipv4/tcp_timer.c            | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 0d477c816309..41663d1ffd0a 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -870,7 +870,7 @@ static void reqsk_timer_handler(struct timer_list *t)
 
 		if (req->num_timeout++ == 0)
 			atomic_dec(&queue->young);
-		timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
+		timeo = min(tcp_timeout_init((struct sock *)req) << req->num_timeout, TCP_RTO_MAX);
 		mod_timer(&req->rsk_timer, jiffies + timeo);
 
 		if (!nreq)
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 0a4f3f16140a..8ddc3aa9e3a6 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -590,7 +590,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 			 * it can be estimated (approximately)
 			 * from another data.
 			 */
-			tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((TCP_TIMEOUT_INIT/HZ)<<req->num_timeout);
+			tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((tcp_timeout_init((struct sock *)req)/HZ)<<req->num_timeout);
 			paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
 		}
 	}
@@ -629,7 +629,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 		    !inet_rtx_syn_ack(sk, req)) {
 			unsigned long expires = jiffies;
 
-			expires += min(TCP_TIMEOUT_INIT << req->num_timeout,
+			expires += min(tcp_timeout_init((struct sock *)req) << req->num_timeout,
 				       TCP_RTO_MAX);
 			if (!fastopen)
 				mod_timer_pending(&req->rsk_timer, expires);
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 20cf4a98c69d..0954e3685ad2 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -430,7 +430,7 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req)
 	if (!tp->retrans_stamp)
 		tp->retrans_stamp = tcp_time_stamp(tp);
 	inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
-			  TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
+			  tcp_timeout_init((struct sock *)req) << req->num_timeout, TCP_RTO_MAX);
 }
 
 
-- 
2.17.1

Re: [PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[Eric Dumazet]

On 10/25/21 5:12 AM, Akhmat Karakotov wrote:
> When setting RTO through BPF program, SYN ACK packets were unaffected and
> continued to use TCP_TIMEOUT_INIT constant. This patch makes SYN ACK
> retransmits use tcp_timeout_init() function instead.
> 

I do not think we want this, this will break existing BPF programs
that did not expect this risky change of behavior.

Please next time you send controversial TCP patches, add TCP experts/maintainers
in CC.

> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> ---
>  net/ipv4/inet_connection_sock.c | 2 +-
>  net/ipv4/tcp_minisocks.c        | 4 ++--
>  net/ipv4/tcp_timer.c            | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 0d477c816309..41663d1ffd0a 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -870,7 +870,7 @@ static void reqsk_timer_handler(struct timer_list *t)
>  
>  		if (req->num_timeout++ == 0)
>  			atomic_dec(&queue->young);
> -		timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
> +		timeo = min(tcp_timeout_init((struct sock *)req) << req->num_timeout, TCP_RTO_MAX);
>  		mod_timer(&req->rsk_timer, jiffies + timeo);
>  
>  		if (!nreq)
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 0a4f3f16140a..8ddc3aa9e3a6 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -590,7 +590,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>  			 * it can be estimated (approximately)
>  			 * from another data.
>  			 */
> -			tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((TCP_TIMEOUT_INIT/HZ)<<req->num_timeout);
> +			tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((tcp_timeout_init((struct sock *)req)/HZ)<<req->num_timeout);
>  			paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
>  		}
>  	}
> @@ -629,7 +629,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>  		    !inet_rtx_syn_ack(sk, req)) {
>  			unsigned long expires = jiffies;
>  
> -			expires += min(TCP_TIMEOUT_INIT << req->num_timeout,
> +			expires += min(tcp_timeout_init((struct sock *)req) << req->num_timeout,
>  				       TCP_RTO_MAX);
>  			if (!fastopen)
>  				mod_timer_pending(&req->rsk_timer, expires);
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 20cf4a98c69d..0954e3685ad2 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -430,7 +430,7 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req)
>  	if (!tp->retrans_stamp)
>  		tp->retrans_stamp = tcp_time_stamp(tp);
>  	inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
> -			  TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
> +			  tcp_timeout_init((struct sock *)req) << req->num_timeout, TCP_RTO_MAX);
>  }
>  
>  
> 

Re:[PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[Alexander Azimov]

Hi Eric,
 
Can you please clarify why do you think that SYN RTO should be accessible through BPF and SYN ACK RTO should be bound to TCP_TIMEOUT_INIT constant?
I can't see the reason for such asymmetry, please advise.
 
I also wonder what kind of existing BPF programs can suffer from these changes. Please give us your insights.

ps: this is a copy of the original message, hope this one will come in a plain text

Re: [PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[Eric Dumazet]

On 10/25/21 12:26 PM, Alexander Azimov wrote:
> Hi Eric,
>  
> Can you please clarify why do you think that SYN RTO should be accessible through BPF and SYN ACK RTO should be bound to TCP_TIMEOUT_INIT constant?
> I can't see the reason for such asymmetry, please advise.
>  
> I also wonder what kind of existing BPF programs can suffer from these changes. Please give us your insights.
> 
> ps: this is a copy of the original message, hope this one will come in a plain text
> 

When commit 8550f328f45db6d37981eb2041bc465810245c03
("bpf: Support for per connection SYN/SYN-ACK RTOs")
was added, tcp_timeout_init() (and potential eBPF prog)
would be called from tcp_conn_request() and tcp_connect_init()

So some users are now using this eBPF program, expecting
it to have an effect only from these call sites.

If you are adding more call sites, suddenly the behavior
of TCP stack for these users will change. You have to
document if bad things could happen for them, like unexpected
max acceptable delays for connection establishment being severely reduced.

In any case, I would prefer adding a new @timeout field
in struct request_sock to carry the base timeout
instead of calling a BPF program all the times,
otherwise we could have weird behavior (eg PAWS checks)
if the return from eBPF program is variable for one 5-tuple.

Also, have you checked if TCP syn cookies would still work
if tcp_timeout_init() returns a small value like 5ms ?

tcp_check_req()
...
tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((tcp_timeout_init((struct sock *)req)/HZ)<<req->num_timeout);

-> tmp_opt.ts_recent_stamp = ktime_get_seconds()

Re: [PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[Martin KaFai Lau]

On Mon, Oct 25, 2021 at 01:48:12PM -0700, Eric Dumazet wrote:
> 
> 
> On 10/25/21 12:26 PM, Alexander Azimov wrote:
> > Hi Eric,
> >  
> > Can you please clarify why do you think that SYN RTO should be accessible through BPF and SYN ACK RTO should be bound to TCP_TIMEOUT_INIT constant?

> > I can't see the reason for such asymmetry, please advise.
In tcp_conn_request(),  tcp_timeout_init() is used, so the very
first SYN-ACK RTO should work?

iiuc, this patch's changes in reqsk_timer_handler() only affects the
later RTOs (e.g. 2nd RTOs).  For this particular
function (reqsk_timer_handler()), it looks like an overlook
in the original bpf's tcp_timeout_init() implementation.

> > I also wonder what kind of existing BPF programs can suffer from these changes. Please give us your insights.
> > 
> > ps: this is a copy of the original message, hope this one will come in a plain text
> > 
> 
> When commit 8550f328f45db6d37981eb2041bc465810245c03
> ("bpf: Support for per connection SYN/SYN-ACK RTOs")
> was added, tcp_timeout_init() (and potential eBPF prog)
> would be called from tcp_conn_request() and tcp_connect_init()
> 
> So some users are now using this eBPF program, expecting
> it to have an effect only from these call sites.
> 
> If you are adding more call sites, suddenly the behavior
> of TCP stack for these users will change. You have to
> document if bad things could happen for them, like unexpected
> max acceptable delays for connection establishment being severely reduced.
> 
> In any case, I would prefer adding a new @timeout field
> in struct request_sock to carry the base timeout
> instead of calling a BPF program all the times,
> otherwise we could have weird behavior (eg PAWS checks)
> if the return from eBPF program is variable for one 5-tuple.
Seems like a good idea.  This field can be set to
the timeout-value returned by the bpf prog.

Re: [PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

> On Oct 25, 2021, at 23:48, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> Also, have you checked if TCP syn cookies would still work
> if tcp_timeout_init() returns a small value like 5ms ?
> 
> tcp_check_req()
> ...
> tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((tcp_timeout_init((struct sock *)req)/HZ)<<req->num_timeout);
> 
> -> tmp_opt.ts_recent_stamp = ktime_get_seconds()
> 
> 

I may have overlooked this. As long as I remember TCP SYN cookies worked
but I will recheck this place again. Also could you please tell in what way exactly
does this relate to syn cookies? I may have misunderstood what the code does.

Re: [PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

Excuse me for lots of duplicate messages. I couldn't get default client
to consistently send plain text messages and delivery system kept rejecting
them. I forgot to remove everyone from cc when tried again.

> On Oct 29, 2021, at 19:19, Akhmat Karakotov <hmukos@yandex-team.ru> wrote:
> 
> 
>> On Oct 25, 2021, at 23:48, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> 
>> Also, have you checked if TCP syn cookies would still work
>> if tcp_timeout_init() returns a small value like 5ms ?
>> 
>> tcp_check_req()
>> ...
>> tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((tcp_timeout_init((struct sock *)req)/HZ)<<req->num_timeout);
>> 
>> -> tmp_opt.ts_recent_stamp = ktime_get_seconds()
>> 
>> 
> 
> I may have overlooked this. As long as I remember TCP SYN cookies worked
> but I will recheck this place again. Also could you please tell in what way exactly
> does this relate to syn cookies? I may have misunderstood what the code does.

Re: [PATCH] tcp: Use BPF timeout setting for SYN ACK RTO

[Eric Dumazet]

On 10/29/21 9:19 AM, Akhmat Karakotov wrote:
> 
>> On Oct 25, 2021, at 23:48, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> Also, have you checked if TCP syn cookies would still work
>> if tcp_timeout_init() returns a small value like 5ms ?
>>
>> tcp_check_req()
>> ...
>> tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((tcp_timeout_init((struct sock *)req)/HZ)<<req->num_timeout);
>>
>> -> tmp_opt.ts_recent_stamp = ktime_get_seconds()
>>
>>
> 
> I may have overlooked this. As long as I remember TCP SYN cookies worked
> but I will recheck this place again. Also could you please tell in what way exactly
> does this relate to syn cookies? I may have misunderstood what the code does.
> 

This was badly written.

This should have been two different questions.

[PATCH v2] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

When setting RTO through BPF program, some SYN ACK packets were unaffected
and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
and is reassigned through BPF using tcp_timeout_init call. SYN ACK
retransmits now use newly added timeout option.

Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
---
 include/net/request_sock.h      | 2 ++
 net/ipv4/inet_connection_sock.c | 2 +-
 net/ipv4/tcp_input.c            | 8 +++++---
 net/ipv4/tcp_minisocks.c        | 4 ++--
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 29e41ff3ec93..144c39db9898 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -70,6 +70,7 @@ struct request_sock {
 	struct saved_syn		*saved_syn;
 	u32				secid;
 	u32				peer_secid;
+	u32				timeout;
 };
 
 static inline struct request_sock *inet_reqsk(const struct sock *sk)
@@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
 	sk_node_init(&req_to_sk(req)->sk_node);
 	sk_tx_queue_clear(req_to_sk(req));
 	req->saved_syn = NULL;
+	req->timeout = 0;
 	req->num_timeout = 0;
 	req->num_retrans = 0;
 	req->sk = NULL;
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 0d477c816309..c43cc1f22092 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -870,7 +870,7 @@ static void reqsk_timer_handler(struct timer_list *t)
 
 		if (req->num_timeout++ == 0)
 			atomic_dec(&queue->young);
-		timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
+		timeo = min(req->timeout << req->num_timeout, TCP_RTO_MAX);
 		mod_timer(&req->rsk_timer, jiffies + timeo);
 
 		if (!nreq)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 3f7bd7ae7d7a..5c181dc4e96f 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6706,6 +6706,7 @@ struct request_sock *inet_reqsk_alloc(const struct request_sock_ops *ops,
 		ireq->ireq_state = TCP_NEW_SYN_RECV;
 		write_pnet(&ireq->ireq_net, sock_net(sk_listener));
 		ireq->ireq_family = sk_listener->sk_family;
+		req->timeout = TCP_TIMEOUT_INIT;
 	}
 
 	return req;
@@ -6922,9 +6923,10 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 		sock_put(fastopen_sk);
 	} else {
 		tcp_rsk(req)->tfo_listener = false;
-		if (!want_cookie)
-			inet_csk_reqsk_queue_hash_add(sk, req,
-				tcp_timeout_init((struct sock *)req));
+		if (!want_cookie) {
+			req->timeout = tcp_timeout_init((struct sock *)req);
+			inet_csk_reqsk_queue_hash_add(sk, req, req->timeout);
+		}
 		af_ops->send_synack(sk, dst, &fl, req, &foc,
 				    !want_cookie ? TCP_SYNACK_NORMAL :
 						   TCP_SYNACK_COOKIE,
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 0a4f3f16140a..9724c9c6d331 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -590,7 +590,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 			 * it can be estimated (approximately)
 			 * from another data.
 			 */
-			tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((TCP_TIMEOUT_INIT/HZ)<<req->num_timeout);
+			tmp_opt.ts_recent_stamp = ktime_get_seconds() - (req->timeout << req->num_timeout) / HZ;
 			paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
 		}
 	}
@@ -629,7 +629,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 		    !inet_rtx_syn_ack(sk, req)) {
 			unsigned long expires = jiffies;
 
-			expires += min(TCP_TIMEOUT_INIT << req->num_timeout,
+			expires += min(req->timeout << req->num_timeout,
 				       TCP_RTO_MAX);
 			if (!fastopen)
 				mod_timer_pending(&req->rsk_timer, expires);
-- 
2.17.1

Re: [PATCH v2] tcp: Use BPF timeout setting for SYN ACK RTO

[Yuchung Cheng]

On Tue, Nov 2, 2021 at 11:33 AM Akhmat Karakotov <hmukos@yandex-team.ru> wrote:
>
> When setting RTO through BPF program, some SYN ACK packets were unaffected
> and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
> option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
> and is reassigned through BPF using tcp_timeout_init call. SYN ACK
> retransmits now use newly added timeout option.
>
> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> ---
>  include/net/request_sock.h      | 2 ++
>  net/ipv4/inet_connection_sock.c | 2 +-
>  net/ipv4/tcp_input.c            | 8 +++++---
>  net/ipv4/tcp_minisocks.c        | 4 ++--
>  4 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 29e41ff3ec93..144c39db9898 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -70,6 +70,7 @@ struct request_sock {
>         struct saved_syn                *saved_syn;
>         u32                             secid;
>         u32                             peer_secid;
> +       u32                             timeout;
>  };
>
>  static inline struct request_sock *inet_reqsk(const struct sock *sk)
> @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
>         sk_node_init(&req_to_sk(req)->sk_node);
>         sk_tx_queue_clear(req_to_sk(req));
>         req->saved_syn = NULL;
> +       req->timeout = 0;
why not just set to TCP_TIMEOUT_INIT to avoid setting it again in
inet_reqsk_alloc?

>         req->num_timeout = 0;
>         req->num_retrans = 0;
>         req->sk = NULL;
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 0d477c816309..c43cc1f22092 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -870,7 +870,7 @@ static void reqsk_timer_handler(struct timer_list *t)
>
>                 if (req->num_timeout++ == 0)
>                         atomic_dec(&queue->young);
> -               timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
> +               timeo = min(req->timeout << req->num_timeout, TCP_RTO_MAX);
>                 mod_timer(&req->rsk_timer, jiffies + timeo);
>
>                 if (!nreq)
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 3f7bd7ae7d7a..5c181dc4e96f 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6706,6 +6706,7 @@ struct request_sock *inet_reqsk_alloc(const struct request_sock_ops *ops,
>                 ireq->ireq_state = TCP_NEW_SYN_RECV;
>                 write_pnet(&ireq->ireq_net, sock_net(sk_listener));
>                 ireq->ireq_family = sk_listener->sk_family;
> +               req->timeout = TCP_TIMEOUT_INIT;
>         }
>
>         return req;
> @@ -6922,9 +6923,10 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>                 sock_put(fastopen_sk);
>         } else {
>                 tcp_rsk(req)->tfo_listener = false;
> -               if (!want_cookie)
> -                       inet_csk_reqsk_queue_hash_add(sk, req,
> -                               tcp_timeout_init((struct sock *)req));
> +               if (!want_cookie) {
> +                       req->timeout = tcp_timeout_init((struct sock *)req);
> +                       inet_csk_reqsk_queue_hash_add(sk, req, req->timeout);
> +               }
>                 af_ops->send_synack(sk, dst, &fl, req, &foc,
>                                     !want_cookie ? TCP_SYNACK_NORMAL :
>                                                    TCP_SYNACK_COOKIE,
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 0a4f3f16140a..9724c9c6d331 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -590,7 +590,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>                          * it can be estimated (approximately)
>                          * from another data.
>                          */
> -                       tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((TCP_TIMEOUT_INIT/HZ)<<req->num_timeout);
> +                       tmp_opt.ts_recent_stamp = ktime_get_seconds() - (req->timeout << req->num_timeout) / HZ;
>                         paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
>                 }
>         }
> @@ -629,7 +629,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>                     !inet_rtx_syn_ack(sk, req)) {
>                         unsigned long expires = jiffies;
>
> -                       expires += min(TCP_TIMEOUT_INIT << req->num_timeout,
> +                       expires += min(req->timeout << req->num_timeout,
>                                        TCP_RTO_MAX);
>                         if (!fastopen)
>                                 mod_timer_pending(&req->rsk_timer, expires);
> --
> 2.17.1
>

Re: [PATCH v2] tcp: Use BPF timeout setting for SYN ACK RTO

[Eric Dumazet]

On 11/2/21 11:32 AM, Akhmat Karakotov wrote:
> When setting RTO through BPF program, some SYN ACK packets were unaffected
> and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
> option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
> and is reassigned through BPF using tcp_timeout_init call. SYN ACK
> retransmits now use newly added timeout option.
> 
> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> ---
>  include/net/request_sock.h      | 2 ++
>  net/ipv4/inet_connection_sock.c | 2 +-
>  net/ipv4/tcp_input.c            | 8 +++++---
>  net/ipv4/tcp_minisocks.c        | 4 ++--
>  4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 29e41ff3ec93..144c39db9898 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -70,6 +70,7 @@ struct request_sock {
>  	struct saved_syn		*saved_syn;
>  	u32				secid;
>  	u32				peer_secid;
> +	u32				timeout;
>  };
>  
>  static inline struct request_sock *inet_reqsk(const struct sock *sk)
> @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
>  	sk_node_init(&req_to_sk(req)->sk_node);
>  	sk_tx_queue_clear(req_to_sk(req));
>  	req->saved_syn = NULL;
> +	req->timeout = 0;
>  	req->num_timeout = 0;
>  	req->num_retrans = 0;
>  	req->sk = NULL;
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 0d477c816309..c43cc1f22092 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -870,7 +870,7 @@ static void reqsk_timer_handler(struct timer_list *t)
>  
>  		if (req->num_timeout++ == 0)
>  			atomic_dec(&queue->young);
> -		timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
> +		timeo = min(req->timeout << req->num_timeout, TCP_RTO_MAX);

I wonder how much time it will take to syzbot to trigger an overflow here and
other parts.

(Not sure BPF_SOCK_OPS_TIMEOUT_INIT has any sanity checks)

Maybe take the opportunity of this patch to use wider type

     timeo = min_t(unsigned long,
                   (unsigned long)req->timeout << req->num_timeout,
                   TCP_RTO_MAX);

Overall, your patch looks good to me, thanks.

Re: [PATCH v2] tcp: Use BPF timeout setting for SYN ACK RTO

[Martin KaFai Lau]

On Tue, Nov 02, 2021 at 03:06:31PM -0700, Eric Dumazet wrote:
> 
> 
> On 11/2/21 11:32 AM, Akhmat Karakotov wrote:
> > When setting RTO through BPF program, some SYN ACK packets were unaffected
> > and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
> > option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
> > and is reassigned through BPF using tcp_timeout_init call. SYN ACK
> > retransmits now use newly added timeout option.
> > 
> > Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> > ---
> >  include/net/request_sock.h      | 2 ++
> >  net/ipv4/inet_connection_sock.c | 2 +-
> >  net/ipv4/tcp_input.c            | 8 +++++---
> >  net/ipv4/tcp_minisocks.c        | 4 ++--
> >  4 files changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> > index 29e41ff3ec93..144c39db9898 100644
> > --- a/include/net/request_sock.h
> > +++ b/include/net/request_sock.h
> > @@ -70,6 +70,7 @@ struct request_sock {
> >  	struct saved_syn		*saved_syn;
> >  	u32				secid;
> >  	u32				peer_secid;
> > +	u32				timeout;
> >  };
> >  
> >  static inline struct request_sock *inet_reqsk(const struct sock *sk)
> > @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
> >  	sk_node_init(&req_to_sk(req)->sk_node);
> >  	sk_tx_queue_clear(req_to_sk(req));
> >  	req->saved_syn = NULL;
> > +	req->timeout = 0;
> >  	req->num_timeout = 0;
> >  	req->num_retrans = 0;
> >  	req->sk = NULL;
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index 0d477c816309..c43cc1f22092 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -870,7 +870,7 @@ static void reqsk_timer_handler(struct timer_list *t)
> >  
> >  		if (req->num_timeout++ == 0)
> >  			atomic_dec(&queue->young);
> > -		timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
> > +		timeo = min(req->timeout << req->num_timeout, TCP_RTO_MAX);
> 
> I wonder how much time it will take to syzbot to trigger an overflow here and
> other parts.
> 
> (Not sure BPF_SOCK_OPS_TIMEOUT_INIT has any sanity checks)
Not now.  It probably makes sense to take this chance to bound
it by TCP_RTO_MAX.

Re: [PATCH v2] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

> On Nov 3, 2021, at 02:17, Martin KaFai Lau <kafai@fb.com> wrote:
> 
> On Tue, Nov 02, 2021 at 03:06:31PM -0700, Eric Dumazet wrote:
>> 
>> 
>> On 11/2/21 11:32 AM, Akhmat Karakotov wrote:
>>> When setting RTO through BPF program, some SYN ACK packets were unaffected
>>> and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
>>> option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
>>> and is reassigned through BPF using tcp_timeout_init call. SYN ACK
>>> retransmits now use newly added timeout option.
>>> 
>>> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
>>> ---
>>> include/net/request_sock.h      | 2 ++
>>> net/ipv4/inet_connection_sock.c | 2 +-
>>> net/ipv4/tcp_input.c            | 8 +++++---
>>> net/ipv4/tcp_minisocks.c        | 4 ++--
>>> 4 files changed, 10 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
>>> index 29e41ff3ec93..144c39db9898 100644
>>> --- a/include/net/request_sock.h
>>> +++ b/include/net/request_sock.h
>>> @@ -70,6 +70,7 @@ struct request_sock {
>>> 	struct saved_syn		*saved_syn;
>>> 	u32				secid;
>>> 	u32				peer_secid;
>>> +	u32				timeout;
>>> };
>>> 
>>> static inline struct request_sock *inet_reqsk(const struct sock *sk)
>>> @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
>>> 	sk_node_init(&req_to_sk(req)->sk_node);
>>> 	sk_tx_queue_clear(req_to_sk(req));
>>> 	req->saved_syn = NULL;
>>> +	req->timeout = 0;
>>> 	req->num_timeout = 0;
>>> 	req->num_retrans = 0;
>>> 	req->sk = NULL;
>>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
>>> index 0d477c816309..c43cc1f22092 100644
>>> --- a/net/ipv4/inet_connection_sock.c
>>> +++ b/net/ipv4/inet_connection_sock.c
>>> @@ -870,7 +870,7 @@ static void reqsk_timer_handler(struct timer_list *t)
>>> 
>>> 		if (req->num_timeout++ == 0)
>>> 			atomic_dec(&queue->young);
>>> -		timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
>>> +		timeo = min(req->timeout << req->num_timeout, TCP_RTO_MAX);
>> 
>> I wonder how much time it will take to syzbot to trigger an overflow here and
>> other parts.
>> 
>> (Not sure BPF_SOCK_OPS_TIMEOUT_INIT has any sanity checks)
> Not now.  It probably makes sense to take this chance to bound
> it by TCP_RTO_MAX.
Where do you suggest to bound to TCP_RTO_MAX? In tcp_timeout_init?

Re: [PATCH v2] tcp: Use BPF timeout setting for SYN ACK RTO

[Martin KaFai Lau]

On Wed, Nov 03, 2021 at 12:31:46PM +0300, Akhmat Karakotov wrote:
> > On Nov 3, 2021, at 02:17, Martin KaFai Lau <kafai@fb.com> wrote:
> > 
> > On Tue, Nov 02, 2021 at 03:06:31PM -0700, Eric Dumazet wrote:
> >> 
> >> 
> >> On 11/2/21 11:32 AM, Akhmat Karakotov wrote:
> >>> When setting RTO through BPF program, some SYN ACK packets were unaffected
> >>> and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
> >>> option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
> >>> and is reassigned through BPF using tcp_timeout_init call. SYN ACK
> >>> retransmits now use newly added timeout option.
> >>> 
> >>> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> >>> ---
> >>> include/net/request_sock.h      | 2 ++
> >>> net/ipv4/inet_connection_sock.c | 2 +-
> >>> net/ipv4/tcp_input.c            | 8 +++++---
> >>> net/ipv4/tcp_minisocks.c        | 4 ++--
> >>> 4 files changed, 10 insertions(+), 6 deletions(-)
> >>> 
> >>> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> >>> index 29e41ff3ec93..144c39db9898 100644
> >>> --- a/include/net/request_sock.h
> >>> +++ b/include/net/request_sock.h
> >>> @@ -70,6 +70,7 @@ struct request_sock {
> >>> 	struct saved_syn		*saved_syn;
> >>> 	u32				secid;
> >>> 	u32				peer_secid;
> >>> +	u32				timeout;
> >>> };
> >>> 
> >>> static inline struct request_sock *inet_reqsk(const struct sock *sk)
> >>> @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
> >>> 	sk_node_init(&req_to_sk(req)->sk_node);
> >>> 	sk_tx_queue_clear(req_to_sk(req));
> >>> 	req->saved_syn = NULL;
> >>> +	req->timeout = 0;
> >>> 	req->num_timeout = 0;
> >>> 	req->num_retrans = 0;
> >>> 	req->sk = NULL;
> >>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> >>> index 0d477c816309..c43cc1f22092 100644
> >>> --- a/net/ipv4/inet_connection_sock.c
> >>> +++ b/net/ipv4/inet_connection_sock.c
> >>> @@ -870,7 +870,7 @@ static void reqsk_timer_handler(struct timer_list *t)
> >>> 
> >>> 		if (req->num_timeout++ == 0)
> >>> 			atomic_dec(&queue->young);
> >>> -		timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
> >>> +		timeo = min(req->timeout << req->num_timeout, TCP_RTO_MAX);
> >> 
> >> I wonder how much time it will take to syzbot to trigger an overflow here and
> >> other parts.
> >> 
> >> (Not sure BPF_SOCK_OPS_TIMEOUT_INIT has any sanity checks)
> > Not now.  It probably makes sense to take this chance to bound
> > it by TCP_RTO_MAX.
> Where do you suggest to bound to TCP_RTO_MAX? In tcp_timeout_init?
Right, tcp_timeout_init should work.

[PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

When setting RTO through BPF program, some SYN ACK packets were unaffected
and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
and is reassigned through BPF using tcp_timeout_init call. SYN ACK
retransmits now use newly added timeout option.

Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
---
 include/net/request_sock.h      |  2 ++
 include/net/tcp.h               |  2 +-
 net/ipv4/inet_connection_sock.c |  4 +++-
 net/ipv4/tcp_input.c            |  8 +++++---
 net/ipv4/tcp_minisocks.c        | 12 +++++++++---
 5 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 29e41ff3ec93..144c39db9898 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -70,6 +70,7 @@ struct request_sock {
 	struct saved_syn		*saved_syn;
 	u32				secid;
 	u32				peer_secid;
+	u32				timeout;
 };
 
 static inline struct request_sock *inet_reqsk(const struct sock *sk)
@@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
 	sk_node_init(&req_to_sk(req)->sk_node);
 	sk_tx_queue_clear(req_to_sk(req));
 	req->saved_syn = NULL;
+	req->timeout = 0;
 	req->num_timeout = 0;
 	req->num_retrans = 0;
 	req->sk = NULL;
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3166dc15d7d6..e328d6735e38 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2323,7 +2323,7 @@ static inline u32 tcp_timeout_init(struct sock *sk)
 
 	if (timeout <= 0)
 		timeout = TCP_TIMEOUT_INIT;
-	return timeout;
+	return min_t(int, timeout, TCP_RTO_MAX);
 }
 
 static inline u32 tcp_rwnd_init_bpf(struct sock *sk)
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 0d477c816309..cdf16285e193 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -870,7 +870,9 @@ static void reqsk_timer_handler(struct timer_list *t)
 
 		if (req->num_timeout++ == 0)
 			atomic_dec(&queue->young);
-		timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
+		timeo = min_t(unsigned long,
+			      (unsigned long)req->timeout << req->num_timeout,
+			      TCP_RTO_MAX);
 		mod_timer(&req->rsk_timer, jiffies + timeo);
 
 		if (!nreq)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 3f7bd7ae7d7a..5c181dc4e96f 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6706,6 +6706,7 @@ struct request_sock *inet_reqsk_alloc(const struct request_sock_ops *ops,
 		ireq->ireq_state = TCP_NEW_SYN_RECV;
 		write_pnet(&ireq->ireq_net, sock_net(sk_listener));
 		ireq->ireq_family = sk_listener->sk_family;
+		req->timeout = TCP_TIMEOUT_INIT;
 	}
 
 	return req;
@@ -6922,9 +6923,10 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 		sock_put(fastopen_sk);
 	} else {
 		tcp_rsk(req)->tfo_listener = false;
-		if (!want_cookie)
-			inet_csk_reqsk_queue_hash_add(sk, req,
-				tcp_timeout_init((struct sock *)req));
+		if (!want_cookie) {
+			req->timeout = tcp_timeout_init((struct sock *)req);
+			inet_csk_reqsk_queue_hash_add(sk, req, req->timeout);
+		}
 		af_ops->send_synack(sk, dst, &fl, req, &foc,
 				    !want_cookie ? TCP_SYNACK_NORMAL :
 						   TCP_SYNACK_COOKIE,
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 0a4f3f16140a..9ebcd554f601 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -583,6 +583,8 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 		tcp_parse_options(sock_net(sk), skb, &tmp_opt, 0, NULL);
 
 		if (tmp_opt.saw_tstamp) {
+			unsigned long timeo;
+
 			tmp_opt.ts_recent = req->ts_recent;
 			if (tmp_opt.rcv_tsecr)
 				tmp_opt.rcv_tsecr -= tcp_rsk(req)->ts_off;
@@ -590,7 +592,10 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 			 * it can be estimated (approximately)
 			 * from another data.
 			 */
-			tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((TCP_TIMEOUT_INIT/HZ)<<req->num_timeout);
+			timeo = min_t(unsigned long,
+				      (unsigned long)req->timeout << req->num_timeout,
+				      TCP_RTO_MAX);
+			tmp_opt.ts_recent_stamp = ktime_get_seconds() - timeo / HZ;
 			paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
 		}
 	}
@@ -629,8 +634,9 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 		    !inet_rtx_syn_ack(sk, req)) {
 			unsigned long expires = jiffies;
 
-			expires += min(TCP_TIMEOUT_INIT << req->num_timeout,
-				       TCP_RTO_MAX);
+			expires += min_t(unsigned long,
+					 (unsigned long)req->timeout << req->num_timeout,
+					 TCP_RTO_MAX);
 			if (!fastopen)
 				mod_timer_pending(&req->rsk_timer, expires);
 			else
-- 
2.17.1

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Eric Dumazet]

On 11/3/21 1:46 PM, Akhmat Karakotov wrote:
> When setting RTO through BPF program, some SYN ACK packets were unaffected
> and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
> option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
> and is reassigned through BPF using tcp_timeout_init call. SYN ACK
> retransmits now use newly added timeout option.
> 
> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> ---
>  include/net/request_sock.h      |  2 ++
>  include/net/tcp.h               |  2 +-
>  net/ipv4/inet_connection_sock.c |  4 +++-
>  net/ipv4/tcp_input.c            |  8 +++++---
>  net/ipv4/tcp_minisocks.c        | 12 +++++++++---
>  5 files changed, 20 insertions(+), 8 deletions(-)

SGTM, thanks.

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

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Yuchung Cheng]

On Wed, Nov 3, 2021 at 1:46 PM Akhmat Karakotov <hmukos@yandex-team.ru> wrote:
>
> When setting RTO through BPF program, some SYN ACK packets were unaffected
> and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
> option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
> and is reassigned through BPF using tcp_timeout_init call. SYN ACK
> retransmits now use newly added timeout option.
>
> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> ---
>  include/net/request_sock.h      |  2 ++
>  include/net/tcp.h               |  2 +-
>  net/ipv4/inet_connection_sock.c |  4 +++-
>  net/ipv4/tcp_input.c            |  8 +++++---
>  net/ipv4/tcp_minisocks.c        | 12 +++++++++---
>  5 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 29e41ff3ec93..144c39db9898 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -70,6 +70,7 @@ struct request_sock {
>         struct saved_syn                *saved_syn;
>         u32                             secid;
>         u32                             peer_secid;
> +       u32                             timeout;
>  };
>
>  static inline struct request_sock *inet_reqsk(const struct sock *sk)
> @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
>         sk_node_init(&req_to_sk(req)->sk_node);
>         sk_tx_queue_clear(req_to_sk(req));
>         req->saved_syn = NULL;
> +       req->timeout = 0;
>         req->num_timeout = 0;
>         req->num_retrans = 0;
>         req->sk = NULL;
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 3166dc15d7d6..e328d6735e38 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2323,7 +2323,7 @@ static inline u32 tcp_timeout_init(struct sock *sk)
>
>         if (timeout <= 0)
>                 timeout = TCP_TIMEOUT_INIT;
> -       return timeout;
> +       return min_t(int, timeout, TCP_RTO_MAX);
>  }
>
>  static inline u32 tcp_rwnd_init_bpf(struct sock *sk)
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 0d477c816309..cdf16285e193 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -870,7 +870,9 @@ static void reqsk_timer_handler(struct timer_list *t)
>
>                 if (req->num_timeout++ == 0)
>                         atomic_dec(&queue->young);
> -               timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
> +               timeo = min_t(unsigned long,
> +                             (unsigned long)req->timeout << req->num_timeout,
> +                             TCP_RTO_MAX);

would it make sense to have a helper tcp_timeout_max() to reduce
clutter? but that can be done by a later refactor patch

>                 mod_timer(&req->rsk_timer, jiffies + timeo);
>
>                 if (!nreq)
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 3f7bd7ae7d7a..5c181dc4e96f 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6706,6 +6706,7 @@ struct request_sock *inet_reqsk_alloc(const struct request_sock_ops *ops,
>                 ireq->ireq_state = TCP_NEW_SYN_RECV;
>                 write_pnet(&ireq->ireq_net, sock_net(sk_listener));
>                 ireq->ireq_family = sk_listener->sk_family;
> +               req->timeout = TCP_TIMEOUT_INIT;
>         }
>
>         return req;
> @@ -6922,9 +6923,10 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>                 sock_put(fastopen_sk);
>         } else {
>                 tcp_rsk(req)->tfo_listener = false;
> -               if (!want_cookie)
> -                       inet_csk_reqsk_queue_hash_add(sk, req,
> -                               tcp_timeout_init((struct sock *)req));
> +               if (!want_cookie) {
> +                       req->timeout = tcp_timeout_init((struct sock *)req);
> +                       inet_csk_reqsk_queue_hash_add(sk, req, req->timeout);
> +               }
>                 af_ops->send_synack(sk, dst, &fl, req, &foc,
>                                     !want_cookie ? TCP_SYNACK_NORMAL :
>                                                    TCP_SYNACK_COOKIE,
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 0a4f3f16140a..9ebcd554f601 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -583,6 +583,8 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>                 tcp_parse_options(sock_net(sk), skb, &tmp_opt, 0, NULL);
>
>                 if (tmp_opt.saw_tstamp) {
> +                       unsigned long timeo;
> +
>                         tmp_opt.ts_recent = req->ts_recent;
>                         if (tmp_opt.rcv_tsecr)
>                                 tmp_opt.rcv_tsecr -= tcp_rsk(req)->ts_off;
> @@ -590,7 +592,10 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>                          * it can be estimated (approximately)
>                          * from another data.
>                          */
> -                       tmp_opt.ts_recent_stamp = ktime_get_seconds() - ((TCP_TIMEOUT_INIT/HZ)<<req->num_timeout);
> +                       timeo = min_t(unsigned long,
> +                                     (unsigned long)req->timeout << req->num_timeout,
> +                                     TCP_RTO_MAX);
> +                       tmp_opt.ts_recent_stamp = ktime_get_seconds() - timeo / HZ;
>                         paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
>                 }
>         }
> @@ -629,8 +634,9 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>                     !inet_rtx_syn_ack(sk, req)) {
>                         unsigned long expires = jiffies;
>
> -                       expires += min(TCP_TIMEOUT_INIT << req->num_timeout,
> -                                      TCP_RTO_MAX);
> +                       expires += min_t(unsigned long,
> +                                        (unsigned long)req->timeout << req->num_timeout,
> +                                        TCP_RTO_MAX);
>                         if (!fastopen)
>                                 mod_timer_pending(&req->rsk_timer, expires);
>                         else
> --
> 2.17.1
>

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Neal Cardwell]

On Wed, Nov 3, 2021 at 5:23 PM Yuchung Cheng <ycheng@google.com> wrote:
>
> On Wed, Nov 3, 2021 at 1:46 PM Akhmat Karakotov <hmukos@yandex-team.ru> wrote:
> >
> > When setting RTO through BPF program, some SYN ACK packets were unaffected
> > and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
> > option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
> > and is reassigned through BPF using tcp_timeout_init call. SYN ACK
> > retransmits now use newly added timeout option.
> >
> > Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> > ---
> >  include/net/request_sock.h      |  2 ++
> >  include/net/tcp.h               |  2 +-
> >  net/ipv4/inet_connection_sock.c |  4 +++-
> >  net/ipv4/tcp_input.c            |  8 +++++---
> >  net/ipv4/tcp_minisocks.c        | 12 +++++++++---
> >  5 files changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> > index 29e41ff3ec93..144c39db9898 100644
> > --- a/include/net/request_sock.h
> > +++ b/include/net/request_sock.h
> > @@ -70,6 +70,7 @@ struct request_sock {
> >         struct saved_syn                *saved_syn;
> >         u32                             secid;
> >         u32                             peer_secid;
> > +       u32                             timeout;
> >  };
> >
> >  static inline struct request_sock *inet_reqsk(const struct sock *sk)
> > @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
> >         sk_node_init(&req_to_sk(req)->sk_node);
> >         sk_tx_queue_clear(req_to_sk(req));
> >         req->saved_syn = NULL;
> > +       req->timeout = 0;
> >         req->num_timeout = 0;
> >         req->num_retrans = 0;
> >         req->sk = NULL;
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index 3166dc15d7d6..e328d6735e38 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -2323,7 +2323,7 @@ static inline u32 tcp_timeout_init(struct sock *sk)
> >
> >         if (timeout <= 0)
> >                 timeout = TCP_TIMEOUT_INIT;
> > -       return timeout;
> > +       return min_t(int, timeout, TCP_RTO_MAX);
> >  }
> >
> >  static inline u32 tcp_rwnd_init_bpf(struct sock *sk)
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index 0d477c816309..cdf16285e193 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -870,7 +870,9 @@ static void reqsk_timer_handler(struct timer_list *t)
> >
> >                 if (req->num_timeout++ == 0)
> >                         atomic_dec(&queue->young);
> > -               timeo = min(TCP_TIMEOUT_INIT << req->num_timeout, TCP_RTO_MAX);
> > +               timeo = min_t(unsigned long,
> > +                             (unsigned long)req->timeout << req->num_timeout,
> > +                             TCP_RTO_MAX);
>
> would it make sense to have a helper tcp_timeout_max() to reduce
> clutter? but that can be done by a later refactor patch

I like Yuchung's idea to have a helper function (perhaps
reqsk_timeout() to go with reqsk_timer_handler()?)  to calculate the
timeout value, since there are 3 of these non-trivial expressions.
Otherwise this looks good to me.

thanks,
neal

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Martin KaFai Lau]

On Wed, Nov 03, 2021 at 11:46:07PM +0300, Akhmat Karakotov wrote:
> When setting RTO through BPF program, some SYN ACK packets were unaffected
> and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
> option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
> and is reassigned through BPF using tcp_timeout_init call. SYN ACK
> retransmits now use newly added timeout option.
> 
> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
> ---
>  include/net/request_sock.h      |  2 ++
>  include/net/tcp.h               |  2 +-
>  net/ipv4/inet_connection_sock.c |  4 +++-
>  net/ipv4/tcp_input.c            |  8 +++++---
>  net/ipv4/tcp_minisocks.c        | 12 +++++++++---
>  5 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 29e41ff3ec93..144c39db9898 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -70,6 +70,7 @@ struct request_sock {
>  	struct saved_syn		*saved_syn;
>  	u32				secid;
>  	u32				peer_secid;
> +	u32				timeout;
>  };
>  
>  static inline struct request_sock *inet_reqsk(const struct sock *sk)
> @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
>  	sk_node_init(&req_to_sk(req)->sk_node);
>  	sk_tx_queue_clear(req_to_sk(req));
>  	req->saved_syn = NULL;
> +	req->timeout = 0;
>  	req->num_timeout = 0;
>  	req->num_retrans = 0;
>  	req->sk = NULL;
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 3166dc15d7d6..e328d6735e38 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2323,7 +2323,7 @@ static inline u32 tcp_timeout_init(struct sock *sk)
>  
>  	if (timeout <= 0)
>  		timeout = TCP_TIMEOUT_INIT;
> -	return timeout;
> +	return min_t(int, timeout, TCP_RTO_MAX);
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

Dear David,

We got the patch acked couple of weeks ago, please let us know what further steps are required before merge.

Thanks, Akhmat.

> On Nov 4, 2021, at 04:06, Martin KaFai Lau <kafai@fb.com> wrote:
> 
> On Wed, Nov 03, 2021 at 11:46:07PM +0300, Akhmat Karakotov wrote:
>> When setting RTO through BPF program, some SYN ACK packets were unaffected
>> and continued to use TCP_TIMEOUT_INIT constant. This patch adds timeout
>> option to struct request_sock. Option is initialized with TCP_TIMEOUT_INIT
>> and is reassigned through BPF using tcp_timeout_init call. SYN ACK
>> retransmits now use newly added timeout option.
>> 
>> Signed-off-by: Akhmat Karakotov <hmukos@yandex-team.ru>
>> ---
>> include/net/request_sock.h      |  2 ++
>> include/net/tcp.h               |  2 +-
>> net/ipv4/inet_connection_sock.c |  4 +++-
>> net/ipv4/tcp_input.c            |  8 +++++---
>> net/ipv4/tcp_minisocks.c        | 12 +++++++++---
>> 5 files changed, 20 insertions(+), 8 deletions(-)
>> 
>> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
>> index 29e41ff3ec93..144c39db9898 100644
>> --- a/include/net/request_sock.h
>> +++ b/include/net/request_sock.h
>> @@ -70,6 +70,7 @@ struct request_sock {
>> 	struct saved_syn		*saved_syn;
>> 	u32				secid;
>> 	u32				peer_secid;
>> +	u32				timeout;
>> };
>> 
>> static inline struct request_sock *inet_reqsk(const struct sock *sk)
>> @@ -104,6 +105,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener,
>> 	sk_node_init(&req_to_sk(req)->sk_node);
>> 	sk_tx_queue_clear(req_to_sk(req));
>> 	req->saved_syn = NULL;
>> +	req->timeout = 0;
>> 	req->num_timeout = 0;
>> 	req->num_retrans = 0;
>> 	req->sk = NULL;
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index 3166dc15d7d6..e328d6735e38 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -2323,7 +2323,7 @@ static inline u32 tcp_timeout_init(struct sock *sk)
>> 
>> 	if (timeout <= 0)
>> 		timeout = TCP_TIMEOUT_INIT;
>> -	return timeout;
>> +	return min_t(int, timeout, TCP_RTO_MAX);
> Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Jakub Kicinski]

On Mon, 17 Jan 2022 18:26:45 +0300 Akhmat Karakotov wrote:
> We got the patch acked couple of weeks ago, please let us know what
> further steps are required before merge.

Did you post a v4 addressing Yuchung's request?

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

On Jan 18, 2022, at 18:57, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Mon, 17 Jan 2022 18:26:45 +0300 Akhmat Karakotov wrote:
>> We got the patch acked couple of weeks ago, please let us know what
>> further steps are required before merge.
> 
> Did you post a v4 addressing Yuchung's request?

I thought that Yuchung suggested to make a separate refactor patch?

> but that can be done by a later refactor patch

But if necessary I will integrate those changes in this patch with v4.

Thanks, Akhmat.

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Jakub Kicinski]

On Tue, 18 Jan 2022 19:49:49 +0300 Akhmat Karakotov wrote:
> On Jan 18, 2022, at 18:57, Jakub Kicinski <kuba@kernel.org> wrote:
> > 
> > On Mon, 17 Jan 2022 18:26:45 +0300 Akhmat Karakotov wrote:  
> >> We got the patch acked couple of weeks ago, please let us know what
> >> further steps are required before merge.  
> > 
> > Did you post a v4 addressing Yuchung's request?  
> 
> I thought that Yuchung suggested to make a separate refactor patch?

Unclear whether separate patch implies separate "series" there.

> > but that can be done by a later refactor patch  
> 
> But if necessary I will integrate those changes in this patch with v4.

Right, net-next is closed, anyway, v4 as a 2-patch mini-series may be
the best way.

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Akhmat Karakotov]

On Jan 18, 2022, at 20:04, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Tue, 18 Jan 2022 19:49:49 +0300 Akhmat Karakotov wrote:
>> On Jan 18, 2022, at 18:57, Jakub Kicinski <kuba@kernel.org> wrote:
>>> 
>>> On Mon, 17 Jan 2022 18:26:45 +0300 Akhmat Karakotov wrote:  
>>>> We got the patch acked couple of weeks ago, please let us know what
>>>> further steps are required before merge.  
>>> 
>>> Did you post a v4 addressing Yuchung's request?  
>> 
>> I thought that Yuchung suggested to make a separate refactor patch?
> 
> Unclear whether separate patch implies separate "series" there.
> 
>>> but that can be done by a later refactor patch  
>> 
>> But if necessary I will integrate those changes in this patch with v4.
> 
> Right, net-next is closed, anyway, v4 as a 2-patch mini-series may be
> the best way.

Why separate then if the second patch is just a refactor? Wouldn't single patch be simpler and better?

Re: [PATCH v3] tcp: Use BPF timeout setting for SYN ACK RTO

[Jakub Kicinski]

On Tue, 18 Jan 2022 20:07:40 +0300 Akhmat Karakotov wrote:
> On Jan 18, 2022, at 20:04, Jakub Kicinski <kuba@kernel.org> wrote:
> >> But if necessary I will integrate those changes in this patch with v4.  
> > 
> > Right, net-next is closed, anyway, v4 as a 2-patch mini-series may be
> > the best way.  
> 
> Why separate then if the second patch is just a refactor? Wouldn't single patch be simpler and better?

Sure.