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 for Android: free password hash cracker in your pocket

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

Message-ID: <eb593fea-b5a5-c871-a762-a48127e91f75@gmail.com>
Date:   Tue, 2 Nov 2021 15:06:31 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Akhmat Karakotov <hmukos@...dex-team.ru>, eric.dumazet@...il.com
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 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)

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.