[PATCH] ARM: futex: fix clobbering oldval

[PATCH] ARM: futex: fix clobbering oldval

[mhban]

This patch fixes clobbering oldval bug. oldval should be preserved for next
compare operation.

Change-Id: I2a63bc1bdb8de330eb9e1ac02d7da1f77e6e8c3c
Signed-off-by: Minho Ban <mhban@samsung.com>
---
 arch/arm/include/asm/futex.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
index d2d733c..b0f2e8e 100644
--- a/arch/arm/include/asm/futex.h
+++ b/arch/arm/include/asm/futex.h
@@ -30,14 +30,14 @@
 	__asm__ __volatile__(					\
 	"1:	ldrex	%1, [%2]\n"				\
 	"	" insn "\n"					\
-	"2:	strex	%1, %0, [%2]\n"				\
-	"	teq	%1, #0\n"				\
+	"2:	strex	r5, %0, [%2]\n"				\
+	"	teq	r5, #0\n"				\
 	"	bne	1b\n"					\
 	"	mov	%0, #0\n"				\
 	__futex_atomic_ex_table("%4")				\
 	: "=&r" (ret), "=&r" (oldval)				\
 	: "r" (uaddr), "r" (oparg), "Ir" (-EFAULT)		\
-	: "cc", "memory")
+	: "r5", "cc", "memory")
 
 static inline int
 futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
-- 
1.7.0.4

[PATCH] ARM: futex: fix clobbering oldval

[Will Deacon]

Hi,

On Thu, Sep 22, 2011 at 03:53:35AM +0100, mhban wrote:
> This patch fixes clobbering oldval bug. oldval should be preserved for next
> compare operation.
> 
> Change-Id: I2a63bc1bdb8de330eb9e1ac02d7da1f77e6e8c3c
> Signed-off-by: Minho Ban <mhban@samsung.com>

It would have been nice to have been CC'd on this...

I ran LTP tests on this, so I'm surprised that this was broken (the tests
passed). Well spotted anyway!

> ---
>  arch/arm/include/asm/futex.h |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
> index d2d733c..b0f2e8e 100644
> --- a/arch/arm/include/asm/futex.h
> +++ b/arch/arm/include/asm/futex.h
> @@ -30,14 +30,14 @@
>  	__asm__ __volatile__(					\
>  	"1:	ldrex	%1, [%2]\n"				\
>  	"	" insn "\n"					\
> -	"2:	strex	%1, %0, [%2]\n"				\
> -	"	teq	%1, #0\n"				\
> +	"2:	strex	r5, %0, [%2]\n"				\
> +	"	teq	r5, #0\n"				\
>  	"	bne	1b\n"					\
>  	"	mov	%0, #0\n"				\
>  	__futex_atomic_ex_table("%4")				\
>  	: "=&r" (ret), "=&r" (oldval)				\
>  	: "r" (uaddr), "r" (oparg), "Ir" (-EFAULT)		\
> -	: "cc", "memory")
> +	: "r5", "cc", "memory")

You shouldn't reference r5 directly here, but due to the way the futex code
is laid out, you can't add an extra output operand without converting the
code to use named arguments.

I'll post a patch to do that.

Will

[PATCH] ARM: futex: fix clobbering oldval

[mhban]

On Thu, 2011-09-22 at 18:26 +0100, Will Deacon wrote:
> Hi,
> 
> On Thu, Sep 22, 2011 at 03:53:35AM +0100, mhban wrote:
> > This patch fixes clobbering oldval bug. oldval should be preserved for next
> > compare operation.
> > 
> > Change-Id: I2a63bc1bdb8de330eb9e1ac02d7da1f77e6e8c3c
> > Signed-off-by: Minho Ban <mhban@samsung.com>
> 
> It would have been nice to have been CC'd on this...

Will not miss next time. Thanks.

> 
> I ran LTP tests on this, so I'm surprised that this was broken (the tests
> passed). Well spotted anyway!
> 
> > ---
> >  arch/arm/include/asm/futex.h |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
> > index d2d733c..b0f2e8e 100644
> > --- a/arch/arm/include/asm/futex.h
> > +++ b/arch/arm/include/asm/futex.h
> > @@ -30,14 +30,14 @@
> >  	__asm__ __volatile__(					\
> >  	"1:	ldrex	%1, [%2]\n"				\
> >  	"	" insn "\n"					\
> > -	"2:	strex	%1, %0, [%2]\n"				\
> > -	"	teq	%1, #0\n"				\
> > +	"2:	strex	r5, %0, [%2]\n"				\
> > +	"	teq	r5, #0\n"				\
> >  	"	bne	1b\n"					\
> >  	"	mov	%0, #0\n"				\
> >  	__futex_atomic_ex_table("%4")				\
> >  	: "=&r" (ret), "=&r" (oldval)				\
> >  	: "r" (uaddr), "r" (oparg), "Ir" (-EFAULT)		\
> > -	: "cc", "memory")
> > +	: "r5", "cc", "memory")
> 
> You shouldn't reference r5 directly here, but due to the way the futex code
> is laid out, you can't add an extra output operand without converting the
> code to use named arguments.
> 
> I'll post a patch to do that.
> 
> Will

I'm not familiar with gcc inline, thanks for pointing it out.

BTW, my last name is Ban not Ben.


Minho

[PATCH] ARM: futex: fix clobbering oldval

[Will Deacon]

On Fri, Sep 23, 2011 at 01:18:49AM +0100, mhban wrote:
> On Thu, 2011-09-22 at 18:26 +0100, Will Deacon wrote:
> > 
> > It would have been nice to have been CC'd on this...
> 
> Will not miss next time. Thanks.

Thanks. You can use scripts/get_maintainer.pl to get a list of people to
email.

> > 
> > You shouldn't reference r5 directly here, but due to the way the futex code
> > is laid out, you can't add an extra output operand without converting the
> > code to use named arguments.
> > 
> > I'll post a patch to do that.
> > 
> > Will
> 
> I'm not familiar with gcc inline, thanks for pointing it out.

There's a good guide for ARM here:

http://www.ethernut.de/en/documents/arm-inline-asm.html

> BTW, my last name is Ban not Ben.

Oops, I'll update my reported-by to spell that correctly.

Cheers,

Will

[PATCH] ARM: futex: fix clobbering oldval

[Jon Masters]

On Thu, 2011-09-22 at 18:26 +0100, Will Deacon wrote:
> Hi,
> 
> On Thu, Sep 22, 2011 at 03:53:35AM +0100, mhban wrote:
> > This patch fixes clobbering oldval bug. oldval should be preserved for next
> > compare operation.
> > 
> > Change-Id: I2a63bc1bdb8de330eb9e1ac02d7da1f77e6e8c3c
> > Signed-off-by: Minho Ban <mhban@samsung.com>
> 
> It would have been nice to have been CC'd on this...
> 
> I ran LTP tests on this, so I'm surprised that this was broken (the tests
> passed). Well spotted anyway!

This appears to be the cause of our "stuck builder" problem in Fedora
building certain large packages, such as OpenJDK. Some of our guys are
still monitoring the situation in case there's a secondary issue.

Was there ever any additional followup on this topic?

Jon.

[PATCH] ARM: futex: fix clobbering oldval

[Will Deacon]

Hi Jon,

On Tue, Dec 06, 2011 at 06:58:34PM +0000, Jon Masters wrote:
> On Thu, 2011-09-22 at 18:26 +0100, Will Deacon wrote:
> > Hi,
> > 
> > On Thu, Sep 22, 2011 at 03:53:35AM +0100, mhban wrote:
> > > This patch fixes clobbering oldval bug. oldval should be preserved for next
> > > compare operation.
> > > 
> > > Change-Id: I2a63bc1bdb8de330eb9e1ac02d7da1f77e6e8c3c
> > > Signed-off-by: Minho Ban <mhban@samsung.com>
> > 
> > It would have been nice to have been CC'd on this...
> > 
> > I ran LTP tests on this, so I'm surprised that this was broken (the tests
> > passed). Well spotted anyway!
> 
> This appears to be the cause of our "stuck builder" problem in Fedora
> building certain large packages, such as OpenJDK. Some of our guys are
> still monitoring the situation in case there's a secondary issue.
> 
> Was there ever any additional followup on this topic?

Yup, check commit df77abca ("ARM: 7099/1: futex: preserve oldval in SMP
__futex_atomic_op"). It should be in stable for all affected kernels too.

Cheers,

Will

[PATCH] ARM: futex: fix clobbering oldval

[Jon Masters]

On Tue, 2011-12-06 at 19:11 +0000, Will Deacon wrote:
> Hi Jon,
> 
> On Tue, Dec 06, 2011 at 06:58:34PM +0000, Jon Masters wrote:
> > On Thu, 2011-09-22 at 18:26 +0100, Will Deacon wrote:
> > > Hi,
> > > 
> > > On Thu, Sep 22, 2011 at 03:53:35AM +0100, mhban wrote:
> > > > This patch fixes clobbering oldval bug. oldval should be preserved for next
> > > > compare operation.
> > > > 
> > > > Change-Id: I2a63bc1bdb8de330eb9e1ac02d7da1f77e6e8c3c
> > > > Signed-off-by: Minho Ban <mhban@samsung.com>
> > > 
> > > It would have been nice to have been CC'd on this...
> > > 
> > > I ran LTP tests on this, so I'm surprised that this was broken (the tests
> > > passed). Well spotted anyway!
> > 
> > This appears to be the cause of our "stuck builder" problem in Fedora
> > building certain large packages, such as OpenJDK. Some of our guys are
> > still monitoring the situation in case there's a secondary issue.
> > 
> > Was there ever any additional followup on this topic?
> 
> Yup, check commit df77abca ("ARM: 7099/1: futex: preserve oldval in SMP
> __futex_atomic_op"). It should be in stable for all affected kernels too.

Yea, Mark flagged that patch and it was included into the kernels on our
boards here. So far, it seems to be making some improvement, though we
still have stuck boards and weird signal issues sometimes. We are trying
to collect more data when this occurs (I gave the engineer concerned
some instructions around collecting data that will be of use). So I just
wondered if you saw any more futex issues, or if this is believe fixed.

Jon.

[PATCH] ARM: futex: fix clobbering oldval

[Will Deacon]

On Tue, Dec 06, 2011 at 07:38:48PM +0000, Jon Masters wrote:
> On Tue, 2011-12-06 at 19:11 +0000, Will Deacon wrote:
> > On Tue, Dec 06, 2011 at 06:58:34PM +0000, Jon Masters wrote:
> > > 
> > > Was there ever any additional followup on this topic?
> > 
> > Yup, check commit df77abca ("ARM: 7099/1: futex: preserve oldval in SMP
> > __futex_atomic_op"). It should be in stable for all affected kernels too.
> 
> Yea, Mark flagged that patch and it was included into the kernels on our
> boards here. So far, it seems to be making some improvement, though we
> still have stuck boards and weird signal issues sometimes. We are trying
> to collect more data when this occurs (I gave the engineer concerned
> some instructions around collecting data that will be of use). So I just
> wondered if you saw any more futex issues, or if this is believe fixed.

I think this is fixed, but that doesn't mean there aren't bugs lurking
elsewhere that I'm simply not seeing. If you do get any diagnostics or
concrete debug information, please CC me on any posts to the list.

Cheers,

Will