Looking at the oops report from this bug:
[bugzilla] https://bugzilla.redhat.com/show_bug.cgi?id=429412
[oops] https://bugzilla.redhat.com/attachment.cgi?id=292260
It was an unhandled page fault (protection violation.)
I tracked it down to the cmpxchg in this code:
include/asm-x86/futex_32.h::futex_atomic_cmpxchg_inatomic()
__asm__ __volatile__(
"1: " LOCK_PREFIX "cmpxchgl %3, %1 \n"
"2: .section .fixup, \"ax\" \n"
"3: mov %2, %0 \n"
" jmp 2b \n"
" .previous \n"
" .section __ex_table, \"a\" \n"
" .align 8 \n"
" .long 1b,3b \n"
" .previous \n"
There is a fixup, so this should never happen. But the lock instruction
was replaced with a nop by the altinstruction code, and that makes the fixup
address wrong. AFAICT we don't fix up the exception table when we replace
a lock with a nop, which makes the fixup table point to the nop instead
of the cmpxchg instruction and causes us to miss the fixup.
On 01/21/2008 03:47 PM, Chuck Ebbert wrote:
> Looking at the oops report from this bug:
>
> [bugzilla] https://bugzilla.redhat.com/show_bug.cgi?id=429412
> [oops] https://bugzilla.redhat.com/attachment.cgi?id=292260
>
> It was an unhandled page fault (protection violation.)
>
> I tracked it down to the cmpxchg in this code:
>
> include/asm-x86/futex_32.h::futex_atomic_cmpxchg_inatomic()
> __asm__ __volatile__(
> "1: " LOCK_PREFIX "cmpxchgl %3, %1 \n"
> "2: .section .fixup, \"ax\" \n"
> "3: mov %2, %0 \n"
> " jmp 2b \n"
> " .previous \n"
> " .section __ex_table, \"a\" \n"
> " .align 8 \n"
> " .long 1b,3b \n"
> " .previous \n"
>
> There is a fixup, so this should never happen. But the lock instruction
> was replaced with a nop by the altinstruction code, and that makes the fixup
> address wrong. AFAICT we don't fix up the exception table when we replace
> a lock with a nop, which makes the fixup table point to the nop instead
> of the cmpxchg instruction and causes us to miss the fixup.
>
The bug reporter has confirmed that booting with "noreplace-smp" prevents
the kernel oops.
Chuck Ebbert <[email protected]> writes:
>
> There is a fixup, so this should never happen. But the lock instruction
> was replaced with a nop by the altinstruction code, and that makes the fixup
> address wrong. AFAICT we don't fix up the exception table when we replace
> a lock with a nop, which makes the fixup table point to the nop instead
> of the cmpxchg instruction and causes us to miss the fixup.
Indeed. Nasty issue.
A quick fix would be to add another fixup to handle both cases
I checked the other LOCK_PREFIX users and they look ok.
Does this fix it?
-Andi
(untested)
---
Add exception handlers for both the LOCK and no LOCK prefix
case in futex.
Hopefully fixes https://bugzilla.redhat.com/show_bug.cgi?id=429412
Signed-off-by: Andi Kleen <[email protected]>
Index: linux/include/asm-x86/futex.h
===================================================================
--- linux.orig/include/asm-x86/futex.h
+++ linux/include/asm-x86/futex.h
@@ -30,7 +30,7 @@
"1: movl %2, %0\n \
movl %0, %3\n" \
insn "\n" \
-"2: " LOCK_PREFIX "cmpxchgl %3, %2\n \
+"2: " LOCK_PREFIX "\n5: cmpxchgl %3, %2\n \
jnz 1b\n \
3: .section .fixup,\"ax\"\n \
4: mov %5, %1\n \
@@ -38,7 +38,7 @@
.previous\n \
.section __ex_table,\"a\"\n \
.align 8\n" \
- _ASM_PTR "1b,4b,2b,4b\n \
+ _ASM_PTR "1b,4b,2b,4b,5b,4b\n \
.previous" \
: "=&a" (oldval), "=&r" (ret), "+m" (*uaddr), \
"=&r" (tem) \
On Tue, 22 Jan 2008, Andi Kleen wrote:
> Chuck Ebbert <[email protected]> writes:
> >
> > There is a fixup, so this should never happen. But the lock instruction
> > was replaced with a nop by the altinstruction code, and that makes the fixup
> > address wrong. AFAICT we don't fix up the exception table when we replace
> > a lock with a nop, which makes the fixup table point to the nop instead
> > of the cmpxchg instruction and causes us to miss the fixup.
>
> Indeed. Nasty issue.
>
> A quick fix would be to add another fixup to handle both cases
Agreed.
> I checked the other LOCK_PREFIX users and they look ok.
Really ?
> Does this fix it?
No it does not. Chuck tracked it down to
include/asm-x86/futex_32.h::futex_atomic_cmpxchg_inatomic()
And your patch is changing: __futex_atomic_op2(). That's fine, because
we need the fixup for both places.
Also your patch is against x86.git and not against mainline. Corrected
version below. x86.git needs a different fixup once this hits
mainline, as it unifies the 32/64bit versions.
That's a long standing bug in both the PI futex and the standard futex
code. Needs to go to stable as well.
Thanks,
tglx
------------->
Subject: x86: fix missing exception entry for SMP alternatives in futex macros
From: Thomas Gleixner <[email protected]>
The exception fixup for the futex macros __futex_atomic_op2 and
futex_atomic_cmpxchg_inatomic() is missing an entry when the lock
prefix is replaced by a NOP via SMP alternatives.
Chuck Ebert tracked this down from the information provided in:
https://bugzilla.redhat.com/show_bug.cgi?id=429412
The solution is to add another fixup after the LOCK_PREFIX, so both
the LOCK and NOP case have their own entry in the exception table.
The solution was pointed out by Andi Kleen.
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
---
include/asm-x86/futex_32.h | 8 ++++----
include/asm-x86/futex_64.h | 8 ++++----
2 files changed, 8 insertions(+), 8 deletions(-)
Index: linux-2.6/include/asm-x86/futex_32.h
===================================================================
--- linux-2.6.orig/include/asm-x86/futex_32.h 2008-01-22 13:13:10.000000000 +0100
+++ linux-2.6/include/asm-x86/futex_32.h 2008-01-22 13:13:49.000000000 +0100
@@ -28,7 +28,7 @@
"1: movl %2, %0\n\
movl %0, %3\n" \
insn "\n" \
-"2: " LOCK_PREFIX "cmpxchgl %3, %2\n\
+"2: " LOCK_PREFIX "\n 5: cmpxchgl %3, %2\n\
jnz 1b\n\
3: .section .fixup,\"ax\"\n\
4: mov %5, %1\n\
@@ -36,7 +36,7 @@
.previous\n\
.section __ex_table,\"a\"\n\
.align 8\n\
- .long 1b,4b,2b,4b\n\
+ .long 1b,4b,2b,4b,5b,4b\n\
.previous" \
: "=&a" (oldval), "=&r" (ret), "+m" (*uaddr), \
"=&r" (tem) \
@@ -111,7 +111,7 @@ futex_atomic_cmpxchg_inatomic(int __user
return -EFAULT;
__asm__ __volatile__(
- "1: " LOCK_PREFIX "cmpxchgl %3, %1 \n"
+ "1: " LOCK_PREFIX "\n 4: cmpxchgl %3, %1 \n"
"2: .section .fixup, \"ax\" \n"
"3: mov %2, %0 \n"
@@ -120,7 +120,7 @@ futex_atomic_cmpxchg_inatomic(int __user
" .section __ex_table, \"a\" \n"
" .align 8 \n"
- " .long 1b,3b \n"
+ " .long 1b,3b,4b,3b \n"
" .previous \n"
: "=a" (oldval), "+m" (*uaddr)
Index: linux-2.6/include/asm-x86/futex_64.h
===================================================================
--- linux-2.6.orig/include/asm-x86/futex_64.h 2008-01-22 13:13:10.000000000 +0100
+++ linux-2.6/include/asm-x86/futex_64.h 2008-01-22 13:13:49.000000000 +0100
@@ -27,7 +27,7 @@
"1: movl %2, %0\n\
movl %0, %3\n" \
insn "\n" \
-"2: " LOCK_PREFIX "cmpxchgl %3, %2\n\
+"2: " LOCK_PREFIX "\n 5: cmpxchgl %3, %2\n\
jnz 1b\n\
3: .section .fixup,\"ax\"\n\
4: mov %5, %1\n\
@@ -35,7 +35,7 @@
.previous\n\
.section __ex_table,\"a\"\n\
.align 8\n\
- .quad 1b,4b,2b,4b\n\
+ .quad 1b,4b,2b,4b,5b,4b\n\
.previous" \
: "=&a" (oldval), "=&r" (ret), "=m" (*uaddr), \
"=&r" (tem) \
@@ -101,7 +101,7 @@ futex_atomic_cmpxchg_inatomic(int __user
return -EFAULT;
__asm__ __volatile__(
- "1: " LOCK_PREFIX "cmpxchgl %3, %1 \n"
+ "1: " LOCK_PREFIX "\n 4: cmpxchgl %3, %1 \n"
"2: .section .fixup, \"ax\" \n"
"3: mov %2, %0 \n"
@@ -110,7 +110,7 @@ futex_atomic_cmpxchg_inatomic(int __user
" .section __ex_table, \"a\" \n"
" .align 8 \n"
- " .quad 1b,3b \n"
+ " .quad 1b,3b,4b,3b \n"
" .previous \n"
: "=a" (oldval), "=m" (*uaddr)
On 01/22/2008 07:35 AM, Thomas Gleixner wrote:
>
> That's a long standing bug in both the PI futex and the standard futex
> code. Needs to go to stable as well.
>
Here's the 2.6.23 version:
Subject: x86: fix missing exception entry for SMP alternatives in futex macros
From: Thomas Gleixner <[email protected]>
The exception fixup for the futex macros __futex_atomic_op2 and
futex_atomic_cmpxchg_inatomic() is missing an entry when the lock
prefix is replaced by a NOP via SMP alternatives.
Chuck Ebert tracked this down from the information provided in:
https://bugzilla.redhat.com/show_bug.cgi?id=429412
The solution is to add another fixup after the LOCK_PREFIX, so both
the LOCK and NOP case have their own entry in the exception table.
The solution was pointed out by Andi Kleen.
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
---
include/asm-i386/futex.h | 8 ++++----
include/asm-x86_64/futex.h | 8 ++++----
2 files changed, 8 insertions(+), 8 deletions(-)
Index: linux-2.6/include/asm-i386/futex.h
===================================================================
--- linux-2.6.orig/include/asm-i386/futex.h 2008-01-22 13:13:10.000000000 +0100
+++ linux-2.6/include/asm-i386/futex.h 2008-01-22 13:13:49.000000000 +0100
@@ -28,7 +28,7 @@
"1: movl %2, %0\n\
movl %0, %3\n" \
insn "\n" \
-"2: " LOCK_PREFIX "cmpxchgl %3, %2\n\
+"2: " LOCK_PREFIX "\n 5: cmpxchgl %3, %2\n\
jnz 1b\n\
3: .section .fixup,\"ax\"\n\
4: mov %5, %1\n\
@@ -36,7 +36,7 @@
.previous\n\
.section __ex_table,\"a\"\n\
.align 8\n\
- .long 1b,4b,2b,4b\n\
+ .long 1b,4b,2b,4b,5b,4b\n\
.previous" \
: "=&a" (oldval), "=&r" (ret), "+m" (*uaddr), \
"=&r" (tem) \
@@ -111,7 +111,7 @@ futex_atomic_cmpxchg_inatomic(int __user
return -EFAULT;
__asm__ __volatile__(
- "1: " LOCK_PREFIX "cmpxchgl %3, %1 \n"
+ "1: " LOCK_PREFIX "\n 4: cmpxchgl %3, %1 \n"
"2: .section .fixup, \"ax\" \n"
"3: mov %2, %0 \n"
@@ -120,7 +120,7 @@ futex_atomic_cmpxchg_inatomic(int __user
" .section __ex_table, \"a\" \n"
" .align 8 \n"
- " .long 1b,3b \n"
+ " .long 1b,3b,4b,3b \n"
" .previous \n"
: "=a" (oldval), "+m" (*uaddr)
Index: linux-2.6/include/asm-x86_64/futex.h
===================================================================
--- linux-2.6.orig/include/asm-x86_64/futex.h 2008-01-22 13:13:10.000000000 +0100
+++ linux-2.6/include/asm-x86_64/futex.h 2008-01-22 13:13:49.000000000 +0100
@@ -27,7 +27,7 @@
"1: movl %2, %0\n\
movl %0, %3\n" \
insn "\n" \
-"2: " LOCK_PREFIX "cmpxchgl %3, %2\n\
+"2: " LOCK_PREFIX "\n 5: cmpxchgl %3, %2\n\
jnz 1b\n\
3: .section .fixup,\"ax\"\n\
4: mov %5, %1\n\
@@ -35,7 +35,7 @@
.previous\n\
.section __ex_table,\"a\"\n\
.align 8\n\
- .quad 1b,4b,2b,4b\n\
+ .quad 1b,4b,2b,4b,5b,4b\n\
.previous" \
: "=&a" (oldval), "=&r" (ret), "=m" (*uaddr), \
"=&r" (tem) \
@@ -101,7 +101,7 @@ futex_atomic_cmpxchg_inatomic(int __user
return -EFAULT;
__asm__ __volatile__(
- "1: " LOCK_PREFIX "cmpxchgl %3, %1 \n"
+ "1: " LOCK_PREFIX "\n 4: cmpxchgl %3, %1 \n"
"2: .section .fixup, \"ax\" \n"
"3: mov %2, %0 \n"
@@ -110,7 +110,7 @@ futex_atomic_cmpxchg_inatomic(int __user
" .section __ex_table, \"a\" \n"
" .align 8 \n"
- " .quad 1b,3b \n"
+ " .quad 1b,3b,4b,3b \n"
" .previous \n"
: "=a" (oldval), "=m" (*uaddr)
On Tue, Jan 22, 2008 at 03:19:33PM -0500, Chuck Ebbert wrote:
> On 01/22/2008 07:35 AM, Thomas Gleixner wrote:
> >
> > That's a long standing bug in both the PI futex and the standard futex
> > code. Needs to go to stable as well.
> >
>
> Here's the 2.6.23 version:
>
>
> Subject: x86: fix missing exception entry for SMP alternatives in futex macros
> From: Thomas Gleixner <[email protected]>
I don't see this in Linus's tree, am I just missing it? Do you have a
git commit id?
thanks,
greg k-h
On Wed, 6 Feb 2008, Greg KH wrote:
>
> I don't see this in Linus's tree, am I just missing it? Do you have a
> git commit id?
Isn't this 9d55b9923a1b7ea8193b8875c57ec940dc2ff027 (possibly with
2532ec6d178abc55681d049097d3dc577eaa266c on top)?
Linus
* Linus Torvalds <[email protected]> wrote:
> > I don't see this in Linus's tree, am I just missing it? Do you have
> > a git commit id?
>
> Isn't this 9d55b9923a1b7ea8193b8875c57ec940dc2ff027 (possibly with
> 2532ec6d178abc55681d049097d3dc577eaa266c on top)?
yeah.
Greg: note that this is against post-unification asm-x86/futex.h so it
wont apply to .24.
I've backported the fix to .24 - find it below. (untested but obvious)
Ingo
----------------------->
Subject: x86: replace LOCK_PREFIX in futex.h
From: Thomas Gleixner <[email protected]>
The exception fixup for the futex macros __futex_atomic_op1/2 and
futex_atomic_cmpxchg_inatomic() is missing an entry when the lock
prefix is replaced by a NOP via SMP alternatives.
Chuck Ebert tracked this down from the information provided in:
https://bugzilla.redhat.com/show_bug.cgi?id=429412
A possible solution would be to add another fixup after the
LOCK_PREFIX, so both the LOCK and NOP case have their own entry in the
exception table, but it's not really worth the trouble.
Simply replace LOCK_PREFIX with lock and keep those untouched by SMP
alternatives.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
include/asm-x86/futex_32.h | 6 +++---
include/asm-x86/futex_64.h | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
Index: linux-2.6.24/include/asm-x86/futex_32.h
===================================================================
--- linux-2.6.24.orig/include/asm-x86/futex_32.h
+++ linux-2.6.24/include/asm-x86/futex_32.h
@@ -28,7 +28,7 @@
"1: movl %2, %0\n\
movl %0, %3\n" \
insn "\n" \
-"2: " LOCK_PREFIX "cmpxchgl %3, %2\n\
+"2: lock; cmpxchgl %3, %2\n \
jnz 1b\n\
3: .section .fixup,\"ax\"\n\
4: mov %5, %1\n\
@@ -68,7 +68,7 @@ futex_atomic_op_inuser (int encoded_op,
#endif
switch (op) {
case FUTEX_OP_ADD:
- __futex_atomic_op1(LOCK_PREFIX "xaddl %0, %2", ret,
+ __futex_atomic_op1("lock; xaddl %0, %2", ret, oldval,
oldval, uaddr, oparg);
break;
case FUTEX_OP_OR:
@@ -111,8 +111,8 @@ futex_atomic_cmpxchg_inatomic(int __user
return -EFAULT;
__asm__ __volatile__(
- "1: " LOCK_PREFIX "cmpxchgl %3, %1 \n"
+ "1: lock; cmpxchgl %3, %1 \n"
"2: .section .fixup, \"ax\" \n"
"3: mov %2, %0 \n"
" jmp 2b \n"
Index: linux-2.6.24/include/asm-x86/futex_64.h
===================================================================
--- linux-2.6.24.orig/include/asm-x86/futex_64.h
+++ linux-2.6.24/include/asm-x86/futex_64.h
@@ -27,7 +27,7 @@
"1: movl %2, %0\n\
movl %0, %3\n" \
insn "\n" \
-"2: " LOCK_PREFIX "cmpxchgl %3, %2\n\
+"2: lock; cmpxchgl %3, %2\n \
jnz 1b\n\
3: .section .fixup,\"ax\"\n\
4: mov %5, %1\n\
@@ -62,7 +62,7 @@ futex_atomic_op_inuser (int encoded_op,
__futex_atomic_op1("xchgl %0, %2", ret, oldval, uaddr, oparg);
break;
case FUTEX_OP_ADD:
- __futex_atomic_op1(LOCK_PREFIX "xaddl %0, %2", ret, oldval,
+ __futex_atomic_op1("lock; xaddl %0, %2", ret, oldval,
uaddr, oparg);
break;
case FUTEX_OP_OR:
@@ -101,8 +101,8 @@ futex_atomic_cmpxchg_inatomic(int __user
return -EFAULT;
__asm__ __volatile__(
- "1: " LOCK_PREFIX "cmpxchgl %3, %1 \n"
+ "1: lock; cmpxchgl %3, %1 \n"
"2: .section .fixup, \"ax\" \n"
"3: mov %2, %0 \n"
" jmp 2b \n"
On Thu, Feb 07, 2008 at 12:43:09AM +0100, Ingo Molnar wrote:
>
> * Linus Torvalds <[email protected]> wrote:
>
> > > I don't see this in Linus's tree, am I just missing it? Do you have
> > > a git commit id?
> >
> > Isn't this 9d55b9923a1b7ea8193b8875c57ec940dc2ff027 (possibly with
> > 2532ec6d178abc55681d049097d3dc577eaa266c on top)?
>
> yeah.
>
> Greg: note that this is against post-unification asm-x86/futex.h so it
> wont apply to .24.
>
> I've backported the fix to .24 - find it below. (untested but obvious)
Thanks for the fix, I'm guessing that this should only go into
.24-stable and nothing prior?
thanks,
greg k-h
On Thu, Feb 07, 2008 at 12:43:09AM +0100, Ingo Molnar wrote:
>
> * Linus Torvalds <[email protected]> wrote:
>
> > > I don't see this in Linus's tree, am I just missing it? Do you have
> > > a git commit id?
> >
> > Isn't this 9d55b9923a1b7ea8193b8875c57ec940dc2ff027 (possibly with
> > 2532ec6d178abc55681d049097d3dc577eaa266c on top)?
>
> yeah.
>
> Greg: note that this is against post-unification asm-x86/futex.h so it
> wont apply to .24.
>
> I've backported the fix to .24 - find it below. (untested but obvious)
unfortunatly it doesn't even compile:
distcc[30740] ERROR: compile (null) on localhost failed
In file included from include/asm/futex.h:2,
from kernel/futex.c:59:
include/asm/futex_32.h:72:29: error: macro "__futex_atomic_op1" passed 6 arguments, but takes just 5
In file included from include/asm/futex.h:2,
from kernel/futex.c:59:
include/asm/futex_32.h: In function 'futex_atomic_op_inuser':
include/asm/futex_32.h:71: error: '__futex_atomic_op1' undeclared (first use in this function)
include/asm/futex_32.h:71: error: (Each undeclared identifier is reported only once
include/asm/futex_32.h:71: error: for each function it appears in.)
distcc[30739] ERROR: compile kernel/futex.c on localhost failed
make[1]: *** [kernel/futex.o] Error 1
Care to try again? :)
thanks,
greg k-h
On Wed, 6 Feb 2008, Greg KH wrote:
> On Tue, Jan 22, 2008 at 03:19:33PM -0500, Chuck Ebbert wrote:
> > On 01/22/2008 07:35 AM, Thomas Gleixner wrote:
> > >
> > > That's a long standing bug in both the PI futex and the standard futex
> > > code. Needs to go to stable as well.
> > >
> >
> > Here's the 2.6.23 version:
> >
> >
> > Subject: x86: fix missing exception entry for SMP alternatives in futex macros
> > From: Thomas Gleixner <[email protected]>
>
> I don't see this in Linus's tree, am I just missing it? Do you have a
> git commit id?
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=9d55b9923a1b7ea8193b8875c57ec940dc2ff027
I did a simpler version by just replacing LOCK_PREFIX with lock. The
reason is, that we would need a separate implementation for
__futex_atomic_op1() as well, due to:
case FUTEX_OP_ADD:
__futex_atomic_op1(LOCK_PREFIX "xaddl %0, %2", ret, oldval,
And it's not really worth the trouble.
Backport to 2.6.24 and 2.6.23 attached.
Thanks,
tglx