On Tue, Apr 16, 2019 at 09:47:51AM -0700, Nathan Chancellor wrote:
On Tue, Apr 16, 2019 at 11:00:52AM +0200, Greg Kroah-Hartman wrote:
On Mon, Apr 15, 2019 at 03:01:51PM -0700, Nathan Chancellor wrote:
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:
FUTEX_WAKE_OP is rarely used and therefore doesn't appear to get exercised by futex() test applications
If the result of the atomic operation is zero, the system call behaves correctly
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);
__futex_atomic_op("add %w0, %w1, %w4",
__futex_atomic_op("add %w3, %w1, %w4", ret, oldval, uaddr, tmp, oparg);
__futex_atomic_op("orr %w0, %w1, %w4",
__futex_atomic_op("orr %w3, %w1, %w4", ret, oldval, uaddr, tmp, oparg);
__futex_atomic_op("and %w0, %w1, %w4",
__futex_atomic_op("and %w3, %w1, %w4", ret, oldval, uaddr, tmp, ~oparg);
__futex_atomic_op("eor %w0, %w1, %w4",
__futex_atomic_op("eor %w3, %w1, %w4", ret, oldval, uaddr, tmp, oparg);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.
Argh, really? That's a buggy compiler that you have there, as oldval will be set correctly if all is good, and if not, ret will be and the code will error out.
Correct.
Working around broken compilers is not something I really like doing :(
Indeed, I wouldn't have brought it up if it wasn't the compiler for all Android 4.9 kernels aside from the Pixel 3 (XL).
That being said, does this also show up in the 4.19.y and 5.0.y tree right now? If not, why not?
It does.
$ make ARCH=arm64 CROSS_COMPILE=<path>/bin/aarch64-linux-gnu- defconfig kernel/futex.o
Great, so it seems this needs to be fixed in Linus's tree first, before I can backport it everywhere.
Want me to send a patch for this or can you?
thanks,
greg k-h