diff mbox series

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

Message ID 20190424170902.623665551@linuxfoundation.org
State New
Headers show
Series None | expand

Commit Message

Greg Kroah-Hartman April 24, 2019, 5:09 p.m. UTC 
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:

  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@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(-)

Comments

Nathan Chancellor April 24, 2019, 5:35 p.m. UTC | #1 
Hi Greg,

On Wed, Apr 24, 2019 at 07:09:12PM +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:

> 

>   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@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

> @@ -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 <natechancellor@gmail.com>

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: stable@vger.kernel.org
Fixes: 045afc24124d ("arm64: futex: Fix FUTEX_WAKE_OP atomic ops with non-zero result value")
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>

---
 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
Greg Kroah-Hartman April 25, 2019, 7:51 a.m. UTC | #2 
On Wed, Apr 24, 2019 at 10:35:20AM -0700, Nathan Chancellor wrote:
> Hi Greg,

> 

> On Wed, Apr 24, 2019 at 07:09:12PM +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:

> > 

> >   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@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

> > @@ -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).


Ugh, I forgot it, thanks for the backport, now queued up.

greg k-h
diff mbox series

Patch

--- 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;
 
 	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: