Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 22 Jan 2021 11:45:08 -0800
From:   Enke Chen <enkechen2020@gmail.com>
To:     Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        Jakub Kicinski <kuba@kernel.org>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Neal Cardwell <ncardwell@google.com>, enkechen2020@gmail.com
Subject: Re: [PATCH] tcp: keepalive fixes
Message-ID: <20210122194508.GB99540@localhost.localdomain>
References: <20210112192544.GA12209@localhost.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210112192544.GA12209@localhost.localdomain>
Precedence: bulk

Hi, Folks:

Please ignore this patch. I will split it into separate ones as suggested
off-list by Neal Cardwell <ncardwell@google.com>.

Thanks.  -- Enke

On Tue, Jan 12, 2021 at 11:25:44AM -0800, Enke Chen wrote:
> From: Enke Chen <enchen@paloaltonetworks.com>
> 
> In this patch two issues with TCP keepalives are fixed:
> 
> 1) TCP keepalive does not timeout when there are data waiting to be
>    delivered and then the connection got broken. The TCP keepalive
>    timeout is not evaluated in that condition.
> 
>    The fix is to remove the code that prevents TCP keepalive from
>    being evaluated for timeout.
> 
> 2) With the fix for #1, TCP keepalive can erroneously timeout after
>    the 0-window probe kicks in. The 0-window probe counter is wrongly
>    applied to TCP keepalives.
> 
>    The fix is to use the elapsed time instead of the 0-window probe
>    counter in evaluating TCP keepalive timeout.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
> ---
>  net/ipv4/tcp_timer.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index 6c62b9ea1320..40953aa40d53 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -696,12 +696,6 @@ static void tcp_keepalive_timer (struct timer_list *t)
>  	    ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_SYN_SENT)))
>  		goto out;
>  
> -	elapsed = keepalive_time_when(tp);
> -
> -	/* It is alive without keepalive 8) */
> -	if (tp->packets_out || !tcp_write_queue_empty(sk))
> -		goto resched;
> -
>  	elapsed = keepalive_time_elapsed(tp);
>  
>  	if (elapsed >= keepalive_time_when(tp)) {
> @@ -709,16 +703,15 @@ static void tcp_keepalive_timer (struct timer_list *t)
>  		 * to determine when to timeout instead.
>  		 */
>  		if ((icsk->icsk_user_timeout != 0 &&
> -		    elapsed >= msecs_to_jiffies(icsk->icsk_user_timeout) &&
> -		    icsk->icsk_probes_out > 0) ||
> +		     elapsed >= msecs_to_jiffies(icsk->icsk_user_timeout)) ||
>  		    (icsk->icsk_user_timeout == 0 &&
> -		    icsk->icsk_probes_out >= keepalive_probes(tp))) {
> +		     (elapsed >= keepalive_time_when(tp) +
> +		      keepalive_intvl_when(tp) * keepalive_probes(tp)))) {
>  			tcp_send_active_reset(sk, GFP_ATOMIC);
>  			tcp_write_err(sk);
>  			goto out;
>  		}
>  		if (tcp_write_wakeup(sk, LINUX_MIB_TCPKEEPALIVE) <= 0) {
> -			icsk->icsk_probes_out++;
>  			elapsed = keepalive_intvl_when(tp);
>  		} else {
>  			/* If keepalive was lost due to local congestion,
> @@ -732,8 +725,6 @@ static void tcp_keepalive_timer (struct timer_list *t)
>  	}
>  
>  	sk_mem_reclaim(sk);
> -
> -resched:
>  	inet_csk_reset_keepalive_timer (sk, elapsed);
>  	goto out;
>  
> -- 
> 2.29.2
>