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

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 <redacted>
---
 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

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.

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

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()

On Mon, Oct 25, 2021 at 01:48:12PM -0700, Eric Dumazet wrote:

On 10/25/21 12:26 PM, Alexander Azimov wrote:

quoted

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?

quoted

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.

quoted

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.

On Oct 25, 2021, at 23:48, Eric Dumazet [off-list ref] 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.

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 [off-list ref] wrote:

quoted

On Oct 25, 2021, at 23:48, Eric Dumazet [off-list ref] 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.

On 10/29/21 9:19 AM, Akhmat Karakotov wrote:

quoted

On Oct 25, 2021, at 23:48, Eric Dumazet [off-list ref] 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.

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 <redacted>
---
 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

On Tue, Nov 2, 2021 at 11:33 AM Akhmat Karakotov [off-list ref] wrote:

why not just set to TCP_TIMEOUT_INIT to avoid setting it again in
inet_reqsk_alloc?

On 11/2/21 11:32 AM, Akhmat Karakotov wrote:

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.

On Tue, Nov 02, 2021 at 03:06:31PM -0700, Eric Dumazet wrote:

On 11/2/21 11:32 AM, Akhmat Karakotov wrote:

quoted

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 <redacted>
---
 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.

On Nov 3, 2021, at 02:17, Martin KaFai Lau [off-list ref] wrote:

On Tue, Nov 02, 2021 at 03:06:31PM -0700, Eric Dumazet wrote:

quoted

On 11/2/21 11:32 AM, Akhmat Karakotov wrote:

quoted

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 <redacted>
---
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?

On Wed, Nov 03, 2021 at 12:31:46PM +0300, Akhmat Karakotov wrote:

quoted

On Nov 3, 2021, at 02:17, Martin KaFai Lau [off-list ref] wrote:

On Tue, Nov 02, 2021 at 03:06:31PM -0700, Eric Dumazet wrote:

quoted

On 11/2/21 11:32 AM, Akhmat Karakotov wrote:

quoted

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 <redacted>
---
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.

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 <redacted>
---
 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

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 <redacted>
---
 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>

On Wed, Nov 3, 2021 at 1:46 PM Akhmat Karakotov [off-list ref] wrote:

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

On Wed, Nov 3, 2021 at 5:23 PM Yuchung Cheng [off-list ref] wrote:

On Wed, Nov 3, 2021 at 1:46 PM Akhmat Karakotov [off-list ref] wrote:

quoted

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 <redacted>
---
 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

On Wed, Nov 03, 2021 at 11:46:07PM +0300, Akhmat Karakotov wrote:

Acked-by: Martin KaFai Lau <redacted>

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 [off-list ref] wrote:

On Wed, Nov 03, 2021 at 11:46:07PM +0300, Akhmat Karakotov wrote:

quoted

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 <redacted>
---
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 <redacted>

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?

On Jan 18, 2022, at 18:57, Jakub Kicinski [off-list ref] wrote:

On Mon, 17 Jan 2022 18:26:45 +0300 Akhmat Karakotov wrote:

quoted

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.

On Tue, 18 Jan 2022 19:49:49 +0300 Akhmat Karakotov wrote:

On Jan 18, 2022, at 18:57, Jakub Kicinski [off-list ref] wrote:

quoted

On Mon, 17 Jan 2022 18:26:45 +0300 Akhmat Karakotov wrote:  

quoted

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.

quoted

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.

On Jan 18, 2022, at 20:04, Jakub Kicinski [off-list ref] wrote:

On Tue, 18 Jan 2022 19:49:49 +0300 Akhmat Karakotov wrote:

quoted

On Jan 18, 2022, at 18:57, Jakub Kicinski [off-list ref] wrote:

quoted

On Mon, 17 Jan 2022 18:26:45 +0300 Akhmat Karakotov wrote:  

quoted

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.

quoted

quoted

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?

On Tue, 18 Jan 2022 20:07:40 +0300 Akhmat Karakotov wrote:

On Jan 18, 2022, at 20:04, Jakub Kicinski [off-list ref] wrote:

quoted

quoted

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.