netdev - Re: [PATCH] tcp: relookup sock for RST+ACK packets handled by obsolete req sock

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] [day] [month] [year] [list]

Message-ID: <7c6d50ad-e09e-fb86-3ee4-c84b76d63417@gmail.com>
Date:   Mon, 15 Mar 2021 11:33:43 +0100
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Alexander Ovechkin <ovov@...dex-team.ru>, netdev@...r.kernel.org
Cc:     zeil@...dex-team.ru, dmtrmonakhov@...dex-team.ru
Subject: Re: [PATCH] tcp: relookup sock for RST+ACK packets handled by
 obsolete req sock



On 3/12/21 12:07 AM, Alexander Ovechkin wrote:
> Currently tcp_check_req can be called with obsolete req socket for which big
> socket have been already created (because of CPU race or early demux
> assigning req socket to multiple packets in gro batch).
> 
> Commit e0f9759f530bf789e984 (\"tcp: try to keep packet if SYN_RCV race
> is lost\") added retry in case when tcp_check_req is called for PSH|ACK packet.
> But if client sends RST+ACK immediatly after connection being
> established (it is performing healthcheck, for example) retry does not
> occur. In that case tcp_check_req tries to close req socket,
> leaving big socket active.
> 

Please insert the following tag, right before your SOB
Fixes: e0f9759f530 ("tcp: try to keep packet if SYN_RCV race is lost")
> Signed-off-by: Alexander Ovechkin <ovov@...dex-team.ru>
> Reported-by: Oleg Senin <olegsenin@...dex-team.ru>

Please CC TCP maintainer for your TCP patches, I almost missed it.


> ---
>  include/net/inet_connection_sock.h | 2 +-
>  net/ipv4/inet_connection_sock.c    | 6 ++++--
>  net/ipv4/tcp_minisocks.c           | 6 ++++--
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
> index 10a625760de9..3c8c59471bc1 100644
> --- a/include/net/inet_connection_sock.h
> +++ b/include/net/inet_connection_sock.h
> @@ -282,7 +282,7 @@ static inline int inet_csk_reqsk_queue_is_full(const struct sock *sk)
>  	return inet_csk_reqsk_queue_len(sk) >= sk->sk_max_ack_backlog;
>  }
>  
> -void inet_csk_reqsk_queue_drop(struct sock *sk, struct request_sock *req);
> +bool inet_csk_reqsk_queue_drop(struct sock *sk, struct request_sock *req);
>  void inet_csk_reqsk_queue_drop_and_put(struct sock *sk, struct request_sock *req);
>  
>  static inline void inet_csk_prepare_for_destroy_sock(struct sock *sk)
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 6bd7ca09af03..08ca9de2a708 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -705,12 +705,14 @@ static bool reqsk_queue_unlink(struct request_sock *req)
>  	return found;
>  }
>  
> -void inet_csk_reqsk_queue_drop(struct sock *sk, struct request_sock *req)
> +bool inet_csk_reqsk_queue_drop(struct sock *sk, struct request_sock *req)
>  {
> -	if (reqsk_queue_unlink(req)) {
> +	bool unlinked = reqsk_queue_unlink(req);

Add an empty line.

> +	if (unlinked) {
>  		reqsk_queue_removed(&inet_csk(sk)->icsk_accept_queue, req);
>  		reqsk_put(req);
>  	}
> +	return unlinked;
>  }
>  EXPORT_SYMBOL(inet_csk_reqsk_queue_drop);
>  
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 0055ae0a3bf8..31ed3423503d 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -804,8 +804,10 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>  		tcp_reset(sk, skb);
>  	}
>  	if (!fastopen) {
> -		inet_csk_reqsk_queue_drop(sk, req);
> -		__NET_INC_STATS(sock_net(sk), LINUX_MIB_EMBRYONICRSTS);
> +		bool unlinked = inet_csk_reqsk_queue_drop(sk, req);

Same here.

> +		if (unlinked)
> +			__NET_INC_STATS(sock_net(sk), LINUX_MIB_EMBRYONICRSTS);
> +		*req_stolen = !unlinked;
>  	}
>  	return NULL;
>  }
> 

Other than that, your patch looks fine to me, thanks.