Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1522358626.2654.39.camel@codethink.co.uk>
Subject: Re: [PATCH 4.4 033/134] tcp: sysctl: Fix a race to avoid unexpected
 0 window from space
From:   Ben Hutchings <ben.hutchings@codethink.co.uk>
To:     Gao Feng <fgao@ikuai8.com>, "David S. Miller" <davem@davemloft.net>
Cc:     stable@vger.kernel.org,
        Sasha Levin <alexander.levin@microsoft.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
Date:   Thu, 29 Mar 2018 22:23:46 +0100
In-Reply-To: <20180319171854.104088447@linuxfoundation.org>
References: <20180319171849.024066323@linuxfoundation.org>
         <20180319171854.104088447@linuxfoundation.org>
Organization: Codethink Ltd.
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 2018-03-19 at 19:05 +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Gao Feng <fgao@ikuai8.com>
> 
> 
> [ Upstream commit c48367427a39ea0b85c7cf018fe4256627abfd9e ]
> 
> Because sysctl_tcp_adv_win_scale could be changed any time, so there
> is one race in tcp_win_from_space.
> For example,
> 1.sysctl_tcp_adv_win_scale<=0 (sysctl_tcp_adv_win_scale is negative now)
> 2.space>>(-sysctl_tcp_adv_win_scale) (sysctl_tcp_adv_win_scale is postive now)
> 
> As a result, tcp_win_from_space returns 0. It is unexpected.
> 
> Certainly if the compiler put the sysctl_tcp_adv_win_scale into one
> register firstly, then use the register directly, it would be ok.
> But we could not depend on the compiler behavior.

This is true, but the compiler can also decide that this local variable
is just an alias for the global variable and still read it twice.  It
is necessary to use READ_ONCE() to prevent that.

Ben.

> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  include/net/tcp.h |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1199,9 +1199,11 @@ void tcp_select_initial_window(int __spa
>  
>  static inline int tcp_win_from_space(int space)
>  {
> -	return sysctl_tcp_adv_win_scale<=0 ?
> -		(space>>(-sysctl_tcp_adv_win_scale)) :
> -		space - (space>>sysctl_tcp_adv_win_scale);
> +	int tcp_adv_win_scale = sysctl_tcp_adv_win_scale;
> +
> +	return tcp_adv_win_scale <= 0 ?
> +		(space>>(-tcp_adv_win_scale)) :
> +		space - (space>>tcp_adv_win_scale);
>  }
>  
>  /* Note: caller must be prepared to deal with negative returns */

-- 
Ben Hutchings
Software Developer, Codethink Ltd.