Re: [PATCH 4.9 72/76] arm64: futex: Fix FUTEX_WAKE_OP atomic ops with non-zero result value

[Nathan Chancellor]

On Mon, Apr 15, 2019 at 08:44:36PM +0200, Greg Kroah-Hartman wrote:
> From: Will Deacon <will.deacon@xxxxxxx>
> 
> 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@xxxxxxxxxx>
> Fixes: 6170a97460db ("arm64: Atomic operations")
> Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> 
> ---
>  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