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: <20211103172209.7ywfmm6zoyjxhqng@kafai-mbp.dhcp.thefacebook.com>
Date:   Wed, 3 Nov 2021 10:22:09 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Akhmat Karakotov <hmukos@...dex-team.ru>
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 Wed, Nov 03, 2021 at 12:31:46PM +0300, Akhmat Karakotov wrote:
> > 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?
Right, tcp_timeout_init should work.