[RFC PATCH] x86 alternatives : fix LOCK_PREFIX race withpreemptible kernel and CPU hotplug

[Mathieu Desnoyers]

If a kernel thread is preempted in single-cpu mode right after the NOP (nop
about to be turned into a lock prefix), then we CPU hotplug a CPU, and then the
thread is scheduled back again, a SMP-unsafe atomic operation will be used on
shared SMP variables, leading to corruption. No corruption would happen in the
reverse case : going from SMP to UP is ok because we split a bit instruction
into tiny pieces, which does not present this condition.

Changing the 0x90 (single-byte nop) currently used into a 0x3E DS segment
override prefix should fix this issue. Since the default of the atomic
instructions is to use the DS segment anyway, it should not affect the
behavior.

** BIG FAT WARNING (tm) ** : this patch assumes that the 0x3E prefix will
leave atomic operations as-is (thus assuming they normally touch data in the DS
segment). Is there an obnoxious corner-case I would have missed ?

This source :
AMD64 Architecture Programmer's Manual Volume 3: General-Purpose and System
Instructions
States
"Instructions that Reference a Non-Stack SegmentâIf an instruction encoding
references any base register other than rBP or rSP, or if an instruction
contains an immediate offset, the default segment is the data segment (DS).
These instructions can use the segment-override prefix to select one of the
non-default segments, as shown in Table 1-5."

Therefore, forcing the DS segment on the atomic operations, which already use
the DS segment, should not change.

This source :
http://wiki.osdev.org/X86_Instruction_Encoding
States
"In 64-bit the CS, SS, DS and ES segment overrides are ignored."
(since it's a wiki, it would have to be confirmed, but at least the worse that
happens is that the DS override is ignored)

This patch applies to 2.6.27-rc2, but would also have to be applied to earlier
kernels (2.6.26, 2.6.25, ...).

Performance impact of the fix : tests done on "xaddq" and "xaddl" shows it
actually improves performances on Intel Xeon, AMD64, Pentium M. It does not
change the performance on Pentium 3 and Pentium 4.

Xeon E5405 2.0GHz :
NR_TESTS                                    10000000
test empty cycles :                        162207948
test test 1-byte nop xadd cycles :         170755422
test test DS override prefix xadd cycles : 170000118 *
test test LOCK xadd cycles :               472012134

AMD64 2.0GHz :
NR_TESTS                                    10000000
test empty cycles :                        146674549
test test 1-byte nop xadd cycles :         150273860
test test DS override prefix xadd cycles : 149982382 *
test test LOCK xadd cycles :               270000690

Pentium 4 3.0GHz
NR_TESTS                                    10000000
test empty cycles :                        290001195
test test 1-byte nop xadd cycles :         310000560
test test DS override prefix xadd cycles : 310000575 *
test test LOCK xadd cycles :              1050103740

Pentium M 2.0GHz
NR_TESTS 10000000
test empty cycles :                        180000523
test test 1-byte nop xadd cycles :         320000345
test test DS override prefix xadd cycles : 310000374 *
test test LOCK xadd cycles :               480000357

Pentium 3 550MHz
NR_TESTS                                    10000000
test empty cycles :                        510000231
test test 1-byte nop xadd cycles :         620000128
test test DS override prefix xadd cycles : 620000110 *
test test LOCK xadd cycles :               800000088

Speed test modules can be found at 
http://ltt.polymtl.ca/svn/trunk/tests/kernel/test-prefix-speed-32.c
http://ltt.polymtl.ca/svn/trunk/tests/kernel/test-prefix-speed.c

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxx>
CC: Andi Kleen <andi@xxxxxxxxxxxxxx>
CC: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
CC: Steven Rostedt <srostedt@xxxxxxxxxx>
CC: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
CC: Ingo Molnar <mingo@xxxxxxx>
CC: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
CC: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
CC: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
CC: David Miller <davem@xxxxxxxxxxxxx>
CC: Roland McGrath <roland@xxxxxxxxxx>
CC: Ulrich Drepper <drepper@xxxxxxxxxx>
CC: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
CC: Gregory Haskins <ghaskins@xxxxxxxxxx>
CC: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
CC: "Luis Claudio R. Goncalves" <lclaudio@xxxxxxxx>
CC: Clark Williams <williams@xxxxxxxxxx>
CC: Christoph Lameter <cl@xxxxxxxxxxxxxxxxxxxx>
---
 arch/x86/kernel/alternative.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6-lttng/arch/x86/kernel/alternative.c
===================================================================
--- linux-2.6-lttng.orig/arch/x86/kernel/alternative.c	2008-08-13 18:42:47.000000000 -0400
+++ linux-2.6-lttng/arch/x86/kernel/alternative.c	2008-08-13 18:54:36.000000000 -0400
@@ -241,25 +241,25 @@ static void alternatives_smp_lock(u8 **s
 			continue;
 		if (*ptr > text_end)
 			continue;
-		text_poke(*ptr, ((unsigned char []){0xf0}), 1); /* add lock prefix */
+		/* turn DS segment override prefix into lock prefix */
+		text_poke(*ptr, ((unsigned char []){0xf0}), 1);
 	};
 }
 
 static void alternatives_smp_unlock(u8 **start, u8 **end, u8 *text, u8 *text_end)
 {
 	u8 **ptr;
-	char insn[1];
 
 	if (noreplace_smp)
 		return;
 
-	add_nops(insn, 1);
 	for (ptr = start; ptr < end; ptr++) {
 		if (*ptr < text)
 			continue;
 		if (*ptr > text_end)
 			continue;
-		text_poke(*ptr, insn, 1);
+		/* turn lock prefix into DS segment override prefix */
+		text_poke(*ptr, ((unsigned char []){0x3E}), 1);
 	};
 }
 
-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/