On Mon, Apr 15, 2019 at 08:44:36PM +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
> @@ -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
