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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <81A927CB-03C7-409C-BE3F-B37D24DA4FE0@yandex-team.ru>
Date:   Wed, 3 Nov 2021 12:31:46 +0300
From:   Akhmat Karakotov <hmukos@...dex-team.ru>
To:     Martin KaFai Lau <kafai@...com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Lawrence Brakmo <brakmo@...com>,
        Alexander Azimov <mitradir@...dex-team.ru>,
        ncardwell@...gle.com, netdev@...r.kernel.org, ycheng@...gle.com,
        zeil@...dex-team.ru
Subject: Re: [PATCH v2] tcp: Use BPF timeout setting for SYN ACK RTO

> On Nov 3, 2021, at 02:17, Martin KaFai Lau <kafai@...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@...dex-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?