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;
Date:   Mon, 15 Apr 2019 15:01:51 -0700
From:   Nathan Chancellor <natechancellor@gmail.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        stable@kernel.org, Will Deacon <will.deacon@arm.com>
Subject: Re: [PATCH 4.9 72/76] arm64: futex: Fix FUTEX_WAKE_OP atomic ops
 with non-zero result value
Message-ID: <20190415220151.GA23056@archlinux-i9>
References: <20190415183707.712011689@linuxfoundation.org>
 <20190415183729.170980546@linuxfoundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190415183729.170980546@linuxfoundation.org>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Apr 15, 2019 at 08:44:36PM +0200, Greg Kroah-Hartman wrote:
> From: Will Deacon <will.deacon@arm.com>
> 
> commit 045afc24124d80c6998d9c770844c67912083506 upstream.
> 
> Rather embarrassingly, our futex() FUTEX_WAKE_OP implementation doesn't
> explicitly set the return value on the non-faulting path and instead
> leaves it holding the result of the underlying atomic operation. This
> means that any FUTEX_WAKE_OP atomic operation which computes a non-zero
> value will be reported as having failed. Regrettably, I wrote the buggy
> code back in 2011 and it was upstreamed as part of the initial arm64
> support in 2012.
> 
> The reasons we appear to get away with this are:
> 
>   1. FUTEX_WAKE_OP is rarely used and therefore doesn't appear to get
>      exercised by futex() test applications
> 
>   2. If the result of the atomic operation is zero, the system call
>      behaves correctly
> 
>   3. Prior to version 2.25, the only operation used by GLIBC set the
>      futex to zero, and therefore worked as expected. From 2.25 onwards,
>      FUTEX_WAKE_OP is not used by GLIBC at all.
> 
> Fix the implementation by ensuring that the return value is either 0
> to indicate that the atomic operation completed successfully, or -EFAULT
> if we encountered a fault when accessing the user mapping.
> 
> Cc: <stable@kernel.org>
> Fixes: 6170a97460db ("arm64: Atomic operations")
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> ---
>  arch/arm64/include/asm/futex.h |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> --- a/arch/arm64/include/asm/futex.h
> +++ b/arch/arm64/include/asm/futex.h
> @@ -33,8 +33,8 @@
>  "	prfm	pstl1strm, %2\n"					\
>  "1:	ldxr	%w1, %2\n"						\
>  	insn "\n"							\
> -"2:	stlxr	%w3, %w0, %2\n"						\
> -"	cbnz	%w3, 1b\n"						\
> +"2:	stlxr	%w0, %w3, %2\n"						\
> +"	cbnz	%w0, 1b\n"						\
>  "	dmb	ish\n"							\
>  "3:\n"									\
>  "	.pushsection .fixup,\"ax\"\n"					\
> @@ -53,29 +53,29 @@
>  static inline int
>  arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  {
> -	int oldval = 0, ret, tmp;
> +	int oldval, ret, tmp;
>  
>  	pagefault_disable();
>  
>  	switch (op) {
>  	case FUTEX_OP_SET:
> -		__futex_atomic_op("mov	%w0, %w4",
> +		__futex_atomic_op("mov	%w3, %w4",
>  				  ret, oldval, uaddr, tmp, oparg);
>  		break;
>  	case FUTEX_OP_ADD:
> -		__futex_atomic_op("add	%w0, %w1, %w4",
> +		__futex_atomic_op("add	%w3, %w1, %w4",
>  				  ret, oldval, uaddr, tmp, oparg);
>  		break;
>  	case FUTEX_OP_OR:
> -		__futex_atomic_op("orr	%w0, %w1, %w4",
> +		__futex_atomic_op("orr	%w3, %w1, %w4",
>  				  ret, oldval, uaddr, tmp, oparg);
>  		break;
>  	case FUTEX_OP_ANDN:
> -		__futex_atomic_op("and	%w0, %w1, %w4",
> +		__futex_atomic_op("and	%w3, %w1, %w4",
>  				  ret, oldval, uaddr, tmp, ~oparg);
>  		break;
>  	case FUTEX_OP_XOR:
> -		__futex_atomic_op("eor	%w0, %w1, %w4",
> +		__futex_atomic_op("eor	%w3, %w1, %w4",
>  				  ret, oldval, uaddr, tmp, oparg);
>  		break;
>  	default:
> 
>

This causes a (false) build warning with AOSP's GCC 4.9.4 (which is
used to build nearly all arm64 Android kernels before 4.14):

  CC      kernel/futex.o
../kernel/futex.c: In function 'do_futex':
../kernel/futex.c:1492:17: warning: 'oldval' may be used uninitialized in this function [-Wmaybe-uninitialized]
   return oldval == cmparg;
                 ^
In file included from ../kernel/futex.c:69:0:
../arch/arm64/include/asm/futex.h:56:6: note: 'oldval' was declared here
  int oldval, ret, tmp;
      ^

The only reason I bring this up is Qualcomm based kernels have a Python
script that emulates -Werror, meaning this will be fatal for a large
number of kernels, when this eventually gets merged into them.

Nathan