Re: [PATCH 3.18 055/104] arm64: futex: Fix FUTEX_WAKE_OP atomic ops with non-zero result value

[Nathan Chancellor]

 Hi Greg,

On Wed, Apr 24, 2019 at 07:09:12PM +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
> @@ -26,8 +26,8 @@
>  	asm volatile(							\
>  "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"					\
> @@ -50,7 +50,7 @@ futex_atomic_op_inuser(unsigned int enco
>  	int cmp = (encoded_op >> 24) & 15;
>  	int oparg = (int)(encoded_op << 8) >> 20;
>  	int cmparg = (int)(encoded_op << 20) >> 20;
> -	int oldval = 0, ret, tmp;
> +	int oldval, ret, tmp;

Please ensure the follow up fix gets queued up for 3.18 as well
(backport attached).

Thanks,
Nathan

>  
>  	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
>  		oparg = 1U << (oparg & 0x1f);
> @@ -62,23 +62,23 @@ futex_atomic_op_inuser(unsigned int enco
>  
>  	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:
> 
>