Hi Greg,
On Wed, Apr 24, 2019 at 07:09:12PM +0200, Greg Kroah-Hartman wrote:
> From: Will Deacon <[email protected]>
>
> 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: <[email protected]>
> Fixes: 6170a97460db ("arm64: Atomic operations")
> Signed-off-by: Will Deacon <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> ---
> 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:
>
>
>From 334d683655a0a4a68792ab1de6b20f4b559b0fcd Mon Sep 17 00:00:00 2001
From: Nathan Chancellor <[email protected]>
Date: Wed, 17 Apr 2019 00:21:21 -0700
Subject: [PATCH] arm64: futex: Restore oldval initialization to work around
buggy compilers
commit ff8acf929014b7f87315588e0daf8597c8aa9d1c upstream.
Commit 045afc24124d ("arm64: futex: Fix FUTEX_WAKE_OP atomic ops with
non-zero result value") removed oldval's zero initialization in
arch_futex_atomic_op_inuser because it is not necessary. Unfortunately,
Android's arm64 GCC 4.9.4 [1] does not agree:
../kernel/futex.c: In function 'do_futex':
../kernel/futex.c:1658:17: warning: 'oldval' may be used uninitialized
in this function [-Wmaybe-uninitialized]
return oldval == cmparg;
^
In file included from ../kernel/futex.c:73:0:
../arch/arm64/include/asm/futex.h:53:6: note: 'oldval' was declared here
int oldval, ret, tmp;
^
GCC fails to follow that when ret is non-zero, futex_atomic_op_inuser
returns right away, avoiding the uninitialized use that it claims.
Restoring the zero initialization works around this issue.
[1]:
https://android.googlesource.com/platform/prebuilts/gcc/linux-x86/aarch64/aarch64-linux-android-4.9/
Cc: [email protected]
Fixes: 045afc24124d ("arm64: futex: Fix FUTEX_WAKE_OP atomic ops with non-zero
result value")
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
Signed-off-by: Catalin Marinas <[email protected]>
---
arch/arm64/include/asm/futex.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
index 66bc1ef344f9..f7fda38d7183 100644
--- a/arch/arm64/include/asm/futex.h
+++ b/arch/arm64/include/asm/futex.h
@@ -50,7 +50,7 @@ futex_atomic_op_inuser(unsigned int encoded_op, u32 __user
*uaddr)
int cmp = (encoded_op >> 24) & 15;
int oparg = (int)(encoded_op << 8) >> 20;
int cmparg = (int)(encoded_op << 20) >> 20;
- int oldval, ret, tmp;
+ int oldval = 0, ret, tmp;
if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
oparg = 1U << (oparg & 0x1f);
--
2.21.0
