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: <20211102231737.nt6o7jehcm7qzjbx@kafai-mbp.dhcp.thefacebook.com>
Date:   Tue, 2 Nov 2021 16:17:37 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        Akhmat Karakotov <hmukos@...dex-team.ru>
CC:     <brakmo@...com>, <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 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.