Skip to contentSkip to conversation

x86: fix ESP corruption CPU bug

PATCH In review Stas Sergeev 13 participants 31 messages Mar 13, 2005 – Mar 22, 2005 Char and misc drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 x86: fix ESP corruption CPU bug

Hi Alan.

Attached patch works around the corruption of the high word of the ESP register, which is the official bug of x86 CPUs. The bug triggers only when the one is using the 16bit stack segment, and is described here:
http://www.intel.com/design/intarch/specupdt/27287402.PDF

Patch helps running many apps under dosemu, and, according to the comments found in Wine sources, also helps Wine.

Also the patch closes the "information leak", which is that the half of the kernel's %esp value gets leaked to user-space.

The initial discussion about the problem can be found here:
http://lkml.org/lkml/2004/9/16/254
On a later stages the development of that patch was driven mainly by Linus, but it was in private.

This patch adds only 6 instructions for the fast kernel-->user return path, 6 instructions on an exception path and 5 instructions on an NMI path. All the rest of the patch gets executed only for the two apps in that world, namely, dosemu and wine (and VMWare?).
That's why it should be relatevely easy to make sure the patch doesn't do any harm for the normal apps, and so it is safe, and probably fits into an -ac tree.

How it works:
- Allocates the per-cpu data for 16bit stacks and sets the per-cpu GDT entries for them.
- On a return to userspace, checks whether the SS is from LDT and is 16 bit.
- If it is, the iret frame gets copied to the 16bit per-cpu stack and stack gets switched to the 16bit one, while the higher word of %esp gets preloaded with the proper value.
- On an exceptions the check is performed to see if we are on a 16bit stack, and if we are - switch to the 32bit one.

Alan, would it be possible to apply that patch to an -ac tree?

arch/i386/kernel/cpu/common.c +12 -0
diff -ur linux-2.6.11-ac2/arch/i386/kernel/cpu/common.c linux-2.6.11-ac2-stk/arch/i386/kernel/cpu/common.c
@@ -21,6 +21,10 @@
 DEFINE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 EXPORT_PER_CPU_SYMBOL(cpu_gdt_table);
 
+unsigned char cpu_16bit_stack[CPU_16BIT_STACK_SIZE];
+DEFINE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]);
+EXPORT_PER_CPU_SYMBOL(cpu_16bit_stack);
+
 static int cachesize_override __initdata = -1;
 static int disable_x86_fxsr __initdata = 0;
 static int disable_x86_serial_nr __initdata = 1;
@@ -557,6 +561,7 @@
 	int cpu = smp_processor_id();
 	struct tss_struct * t = &per_cpu(init_tss, cpu);
 	struct thread_struct *thread = &current->thread;
+	__u32 stk16_off = (__u32)&per_cpu(cpu_16bit_stack, cpu);
 
 	if (cpu_test_and_set(cpu, cpu_initialized)) {
 		printk(KERN_WARNING "CPU#%d already initialized!\n", cpu);
@@ -579,6 +584,13 @@
 	 */
 	memcpy(&per_cpu(cpu_gdt_table, cpu), cpu_gdt_table,
 	       GDT_SIZE);
+
+	/* Set up GDT entry for 16bit stack */
+	*(__u64 *)&(per_cpu(cpu_gdt_table, cpu)[GDT_ENTRY_ESPFIX_SS]) |=
+		((((__u64)stk16_off) << 16) & 0x000000ffffff0000ULL) |
+		((((__u64)stk16_off) << 32) & 0xff00000000000000ULL) |
+		(CPU_16BIT_STACK_SIZE - 1);
+
 	cpu_gdt_descr[cpu].size = GDT_SIZE - 1;
 	cpu_gdt_descr[cpu].address =
 	    (unsigned long)&per_cpu(cpu_gdt_table, cpu);
arch/i386/kernel/entry.S +106 -24

203 lines not on this page · raw message

arch/i386/kernel/head.S +1 -1
diff -ur linux-2.6.11-ac2/arch/i386/kernel/head.S linux-2.6.11-ac2-stk/arch/i386/kernel/head.S
@@ -512,7 +512,7 @@
 	.quad 0x00009a0000000000	/* 0xc0 APM CS 16 code (16 bit) */
 	.quad 0x0040920000000000	/* 0xc8 APM DS    data */
 
-	.quad 0x0000000000000000	/* 0xd0 - unused */
+	.quad 0x0000920000000000	/* 0xd0 - ESPFIX 16-bit SS */
 	.quad 0x0000000000000000	/* 0xd8 - unused */
 	.quad 0x0000000000000000	/* 0xe0 - unused */
 	.quad 0x0000000000000000	/* 0xe8 - unused */
arch/i386/kernel/traps.c +45 -0

51 lines not on this page · raw message

include/asm-i386/desc.h +5 -0
diff -ur linux-2.6.11-ac2/include/asm-i386/desc.h linux-2.6.11-ac2-stk/include/asm-i386/desc.h
@@ -4,6 +4,8 @@
 #include <asm/ldt.h>
 #include <asm/segment.h>
 
+#define CPU_16BIT_STACK_SIZE 1024
+
 #ifndef __ASSEMBLY__
 
 #include <linux/preempt.h>
@@ -15,6 +17,9 @@
 extern struct desc_struct cpu_gdt_table[GDT_ENTRIES];
 DECLARE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 
+extern unsigned char cpu_16bit_stack[CPU_16BIT_STACK_SIZE];
+DECLARE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]);
+
 struct Xgt_desc_struct {
 	unsigned short size;
 	unsigned long address __attribute__((packed));
include/asm-i386/segment.h +4 -1
diff -ur linux-2.6.11-ac2/include/asm-i386/segment.h linux-2.6.11-ac2-stk/include/asm-i386/segment.h
@@ -38,7 +38,7 @@
  *  24 - APM BIOS support
  *  25 - APM BIOS support 
  *
- *  26 - unused
+ *  26 - ESPFIX small SS
  *  27 - unused
  *  28 - unused
  *  29 - unused
@@ -71,6 +71,9 @@
 #define GDT_ENTRY_PNPBIOS_BASE		(GDT_ENTRY_KERNEL_BASE + 6)
 #define GDT_ENTRY_APMBIOS_BASE		(GDT_ENTRY_KERNEL_BASE + 11)
 
+#define GDT_ENTRY_ESPFIX_SS		(GDT_ENTRY_KERNEL_BASE + 14)
+#define __ESPFIX_SS (GDT_ENTRY_ESPFIX_SS * 8)
+
 #define GDT_ENTRY_DOUBLEFAULT_TSS	31
 
 /*
#1 lore raw
Quoted 8 lines · Stas Sergeev

Hi Alan.

Attached patch works around the corruption of the high word of the ESP register, which is the official bug of x86 CPUs. The bug triggers only when the one is using the 16bit stack segment, and is described here:
http://www.intel.com/design/intarch/specupdt/27287402.PDF

Does the bug also egsist on AMD CPU's? Does the patch add anything to kernels compiled for AMD CPU's?

Thanks,

Grzegorz Kulewski

#2 lore raw

Hello.

Quoted 1 line · Grzegorz Kulewski

Does the bug also egsist on AMD CPU's?

Yes. As well as the ones of a Transmeta etc. I just haven't tested the old Cyrixes, that AFAIK were trying to ignore some Intel bugs. The test-case for the bug is here:
http://www.ussg.iu.edu/hypermail/linux/kernel/0409.2/0690.html

Quoted 1 line

Does the patch add anything to kernels compiled for AMD CPU's?

Same as for the Intel ones - unless you are a dosemu or, in a lesser extent, Wine user - nothing except for fixing the small "information leak".
If you are the dosemu user however, then this patch adds a lot. Whether or not it adds something for the VMWare users I can't say, since I am not the one of them, but my guess is that it can help with the DOS programs under it.

#3 lore raw
Quoted 11 lines · Stas Sergeev

Hello.

Quoted 1 line · Grzegorz Kulewski

Does the bug also egsist on AMD CPU's?

Yes. As well as the ones of a Transmeta etc.
I just haven't tested the old Cyrixes, that
AFAIK were trying to ignore some Intel bugs.
The test-case for the bug is here:
http://www.ussg.iu.edu/hypermail/linux/kernel/0409.2/0690.html

I've just ran that on my Cyrix MII PR300 and the bug is present:
old_ss=0x7b new_ss=0x7f
In sighandler: esp=bffff780
old_esp=0xbffff780 new_esp=0xc1a6f780
BUG!

I have also an older Cyrix CPU - 6x86 PR166 - but can't test it now as it's sitting in a plastic box on the shelf :-)

UMC U5SX/33 in my router - also present:
old_ss=0x7b new_ss=0x7f
In sighandler: esp=bffff820
old_esp=0xbffff820 new_esp=0xc003f820
BUG!

Signature · 1 line
Ondrej Zary
#4 lore raw

Hello.

Quoted 2 lines · Ondrej Zary

I've just ran that on my Cyrix MII PR300 and the bug is present:<> UMC U5SX/33 in my router - also present:

Thanks, now I know for sure that it exist
everywhere.
Now you can apply the patch and make sure
the bug goes away:)

#5 lore raw
In reply to Grzegorz Kulewski

Hi!

Quoted 9 lines
Quoted 6 lines

Attached patch works around the corruption of the high word of the ESP register, which is the official bug of x86 CPUs. The bug triggers only when the one is using the 16bit stack segment, and is described here:
http://www.intel.com/design/intarch/specupdt/27287402.PDF

Does the bug also egsist on AMD CPU's? Does the patch add anything to kernels compiled for AMD CPU's?

Yes, same workaround is needed on AMDs, Cyrixes, ...

Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#6 lore raw
In reply to Stas Sergeev

Hi!

arch/i386/kernel/entry.S line 275

Quoted 13 lines
arch/i386/kernel/entry.S +7 -1
@@ -257,8 +265,31 @@
 	movl TI_flags(%ebp), %ecx
 	testw $_TIF_ALLWORK_MASK, %cx	# current->work
 	jne syscall_exit_work
+
 restore_all:
-	RESTORE_ALL
+	movl EFLAGS(%esp), %eax		# mix EFLAGS, SS and CS
+	movb OLDSS(%esp), %ah
+	movb CS(%esp), %al
+	andl $(VM_MASK | (4 << 8) | 3), %eax
+	cmpl $((4 << 8) | 3), %eax
+	je ldt_ss			# returning to user-space with LDT SS

All common linux apps use same %ss, no? Perhaps it would be more efficient to just check if %ss == 0x7b, and proceed directly to restore_nocheck if not?

Or perhaps we could only enable this code after application loads custom ldt?

Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#7 lore raw

Hi.

Quoted 6 lines · Pavel Machek

arch/i386/kernel/entry.S line 275

Quoted 3 lines
arch/i386/kernel/entry.S +3 -0
+	andl $(VM_MASK | (4 << 8) | 3), %eax
+	cmpl $((4 << 8) | 3), %eax
+	je ldt_ss			# returning to user-space with LDT SS

All common linux apps use same %ss, no? Perhaps it would be more efficient to just check if %ss == 0x7b, and proceed directly to restore_nocheck if not?

Such an optimization will cost three more instructions, one of which is a "taken" jump. It seems like the "taken" jump on a fast path is not good, while now it is only 5 instructions with a not-taken jump. I am not sure here, but I think the current solution is better (depends on how bad the "taken" jump is, and how bad it is to have the three extra insns for that optimization purpose).

Quoted 2 lines

Or perhaps we could only enable this code
after application loads custom ldt?

The good thing here is that the code actually does what you say, i.e. it jumps to ldt_ss only when the app has loaded the custom ldt and loaded that selector to %ss. The way it is implemented, is probably different from what you mean,
I assume you mean the new per-thread flag? But I don't see how it can be more optimal, i.e. you propose to check whether or not the app altered the ldt (which can just be the old glibc I think), while the current solution is to also check whether it was loaded to %ss (so the glibc case is avoided, IIRC glibc used to load %gs with LDT selector). I.e. since right now we jump to ldt_ss only when the %ss is loaded with an LDT selector, I think the extra checks are not needed.

#8 lore raw
Quoted 4 lines · Stas Sergeev

Such an optimization will cost three more
instructions, one of which is a "taken"
jump.

I think Pavel missed the fact that you have to check the VM86 bit in eflags before you check SS, since otherwise SS doesn't mean anything special at all (ie checking for just the normal SS isn't correct: you could have a 16-bit SS that looks normal, but is actually a vm86 segment).

Pavel: for the same reason you have to check the low two bits of CS too, since if they are zero, then SS hasn't been saved on the stack at all, so comparing it against some normal value is meaningless.

That said, the "ldt_ss" case should be moved _after_ the conditional tests, since most CPU's out there will do static prediction based on forward/backwards direction if the branch predictor isn't primed. And so since it's an unusual case, the branch should be a forward branch, which is usually the not-taken one (this depends on the core, of course, and you could also add the prefix byte to mark it explicitly predict-not-taken for the newer cores that support it).

Linus

#9 lore raw

PATCH 1/1 x86: fix ESP corruption CPU bug (take 2)

Hello.

Quoted 1 line · Linus Torvalds

I think Pavel missed the fact that you have to check the VM86 bit in

Ah, thanks, I must have forgotten the essentials of the own patch :(

Quoted 3 lines

That said, the "ldt_ss" case should be moved _after_ the conditional tests, since most CPU's out there will do static prediction based on<> forward/backwards direction if the branch predictor isn't primed.

OK, I moved the "ldt_ss" below the check. The new patch is attached.

Alan, can you please apply that to an -ac tree?

----
Summary:
Attached patch works around the corruption of the high word of the ESP register, which is the official bug of x86 CPUs. The bug triggers only when the one is using the 16bit stack segment, and is described here:
http://www.intel.com/design/intarch/specupdt/27287402.PDF

arch/i386/kernel/cpu/common.c +12 -0
diff -ur linux-2.6.11-ac2/arch/i386/kernel/cpu/common.c linux-2.6.11-ac2-stk/arch/i386/kernel/cpu/common.c
@@ -21,6 +21,10 @@
 DEFINE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 EXPORT_PER_CPU_SYMBOL(cpu_gdt_table);
 
+unsigned char cpu_16bit_stack[CPU_16BIT_STACK_SIZE];
+DEFINE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]);
+EXPORT_PER_CPU_SYMBOL(cpu_16bit_stack);
+
 static int cachesize_override __initdata = -1;
 static int disable_x86_fxsr __initdata = 0;
 static int disable_x86_serial_nr __initdata = 1;
@@ -557,6 +561,7 @@
 	int cpu = smp_processor_id();
 	struct tss_struct * t = &per_cpu(init_tss, cpu);
 	struct thread_struct *thread = &current->thread;
+	__u32 stk16_off = (__u32)&per_cpu(cpu_16bit_stack, cpu);
 
 	if (cpu_test_and_set(cpu, cpu_initialized)) {
 		printk(KERN_WARNING "CPU#%d already initialized!\n", cpu);
@@ -579,6 +584,13 @@
 	 */
 	memcpy(&per_cpu(cpu_gdt_table, cpu), cpu_gdt_table,
 	       GDT_SIZE);
+
+	/* Set up GDT entry for 16bit stack */
+	*(__u64 *)&(per_cpu(cpu_gdt_table, cpu)[GDT_ENTRY_ESPFIX_SS]) |=
+		((((__u64)stk16_off) << 16) & 0x000000ffffff0000ULL) |
+		((((__u64)stk16_off) << 32) & 0xff00000000000000ULL) |
+		(CPU_16BIT_STACK_SIZE - 1);
+
 	cpu_gdt_descr[cpu].size = GDT_SIZE - 1;
 	cpu_gdt_descr[cpu].address =
 	    (unsigned long)&per_cpu(cpu_gdt_table, cpu);
arch/i386/kernel/entry.S +106 -24

197 lines not on this page · raw message

arch/i386/kernel/head.S +1 -1
diff -ur linux-2.6.11-ac2/arch/i386/kernel/head.S linux-2.6.11-ac2-stk/arch/i386/kernel/head.S
@@ -512,7 +512,7 @@
 	.quad 0x00009a0000000000	/* 0xc0 APM CS 16 code (16 bit) */
 	.quad 0x0040920000000000	/* 0xc8 APM DS    data */
 
-	.quad 0x0000000000000000	/* 0xd0 - unused */
+	.quad 0x0000920000000000	/* 0xd0 - ESPFIX 16-bit SS */
 	.quad 0x0000000000000000	/* 0xd8 - unused */
 	.quad 0x0000000000000000	/* 0xe0 - unused */
 	.quad 0x0000000000000000	/* 0xe8 - unused */
arch/i386/kernel/traps.c +45 -0

51 lines not on this page · raw message

include/asm-i386/desc.h +5 -0
diff -ur linux-2.6.11-ac2/include/asm-i386/desc.h linux-2.6.11-ac2-stk/include/asm-i386/desc.h
@@ -4,6 +4,8 @@
 #include <asm/ldt.h>
 #include <asm/segment.h>
 
+#define CPU_16BIT_STACK_SIZE 1024
+
 #ifndef __ASSEMBLY__
 
 #include <linux/preempt.h>
@@ -15,6 +17,9 @@
 extern struct desc_struct cpu_gdt_table[GDT_ENTRIES];
 DECLARE_PER_CPU(struct desc_struct, cpu_gdt_table[GDT_ENTRIES]);
 
+extern unsigned char cpu_16bit_stack[CPU_16BIT_STACK_SIZE];
+DECLARE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]);
+
 struct Xgt_desc_struct {
 	unsigned short size;
 	unsigned long address __attribute__((packed));
include/asm-i386/segment.h +4 -1
diff -ur linux-2.6.11-ac2/include/asm-i386/segment.h linux-2.6.11-ac2-stk/include/asm-i386/segment.h
@@ -38,7 +38,7 @@
  *  24 - APM BIOS support
  *  25 - APM BIOS support 
  *
- *  26 - unused
+ *  26 - ESPFIX small SS
  *  27 - unused
  *  28 - unused
  *  29 - unused
@@ -71,6 +71,9 @@
 #define GDT_ENTRY_PNPBIOS_BASE		(GDT_ENTRY_KERNEL_BASE + 6)
 #define GDT_ENTRY_APMBIOS_BASE		(GDT_ENTRY_KERNEL_BASE + 11)
 
+#define GDT_ENTRY_ESPFIX_SS		(GDT_ENTRY_KERNEL_BASE + 14)
+#define __ESPFIX_SS (GDT_ENTRY_ESPFIX_SS * 8)
+
 #define GDT_ENTRY_DOUBLEFAULT_TSS	31
 
 /*
#10 lore raw

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

Quoted 2 lines · Stas Sergeev

Alan, can you please apply that to an -ac
tree?

Ask Andrew Morton as it belongs in the -mm tree

#11 lore raw

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

Hi!

Quoted 3 lines · Alan Cox
Quoted 2 lines

Alan, can you please apply that to an -ac
tree?

Ask Andrew Morton as it belongs in the -mm tree

Actually I tried that already. Andrew had nothing against that patch personally, as well as Linus, but after all that didn't work:
http://lkml.org/lkml/2005/1/3/260

So it can't be applied to -mm, and not depending on the kgdb-ga patch allowed for some extra optimization.
So I have to try the -ac as the last resort. I realized you won't be pleased with that too much. If you are confident it doesn't fit the -ac tree by whatever means, I can understand that. (In that case I'll much appreciate the suggestion what other tree should I try.)

#12 lore raw

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

Quoted 6 lines · Stas Sergeev
Quoted 3 lines · Alan Cox
Quoted 2 lines

Alan, can you please apply that to an -ac
tree?

Ask Andrew Morton as it belongs in the -mm tree

Actually I tried that already.

I added this patch to -mm.

Quoted 9 lines

Andrew
had nothing against that patch personally,
as well as Linus, but after all that didn't
work:
http://lkml.org/lkml/2005/1/3/260

So it can't be applied to -mm, and not
depending on the kgdb-ga patch allowed for
some extra optimization.

The rule is:

- If the patch patches something which is in Linus's kernel, prepare a diff against Linus's latest kernel.

- If the patch patches something which is only in -mm, prepare a patch against -mm.

In this case, I merged the patch prior to the kgdb patch and then fixed up the fallout.

(If that causes kgdb to break in non-obvious-to-me ways then I might come calling "help". We'll see)

#13 lore raw

x86: spin_unlock(), spin_unlock_irq() & others are out of line ?

Hi all

I noticed that in current linux kernel versions (2.6.11), some basic functions are out of line (not inlined)

Example of a call to spin_unlock(&somelock)

c01069fa:   b8 e8 7b 35 c0          mov    $0xc0357be8,%eax
c01069ff:   e8 3c e4 1f 00          call   c0304e40 <_spin_unlock>

c0304e40 <_spin_unlock>:

c0304e40:   c6 00 01                movb   $0x1,(%eax)
c0304e43:   c3                      ret

Same problem for _write_unlock(), _read_unlock(), _spin_unlock_irq(), ...

That seems odd, and I fail to see the reason for that. (It's OK for complex functions, but not for very short ones...)

Is it a regression, or is it needed ?

configuration :
- SMP
- Processor family (Pentium-4/Celeron(P4-based)/Pentium-4 M/Xeon)
- No "Generic x86 support"

Thank you
Eric Dumazet

#14 lore raw

Re: x86: spin_unlock(), spin_unlock_irq() & others are out of line ?

Quoted 2 lines · Eric Dumazet

Is it a regression, or is it needed ?

Please see the "Completely out of line spinlocks" thread from about a month ago.

Lee

#15 lore raw

Re: [patch] x86: fix ESP corruption CPU bug (take 2)

In reply to Andrew Morton

Hello.

Quoted 1 line · Andrew Morton

I added this patch to -mm.

Many thanks!

Alan, sorry for bothering you with that.

Quoted 8 lines

- If the patch patches something which is in Linus's kernel, prepare a diff against Linus's latest kernel.
- If the patch patches something which is only in -mm, prepare a patch against -mm.
In this case, I merged the patch prior to the kgdb patch and then fixed up the fallout.
(If that causes kgdb to break in non-obvious-to-me ways then I might come calling "help".

Thanks for explanations. That's what I
tried to find in your "The perfect patch"
guidelines many times, but it is not there
in such details.

#16 lore raw
In reply to Linus Torvalds
Quoted 8 lines · Linus Torvalds
Quoted 4 lines · Stas Sergeev

Such an optimization will cost three more
instructions, one of which is a "taken"
jump.

Quoted 8 lines

I think Pavel missed the fact that you have to check the VM86 bit in eflags before you check SS, since otherwise SS doesn't mean anything special at all (ie checking for just the normal SS isn't correct: you could have a 16-bit SS that looks normal, but is actually a vm86 segment).

Pavel: for the same reason you have to check the low two bits of CS too, since if they are zero, then SS hasn't been saved on the stack at all, so comparing it against some normal value is meaningless.

Yes, I missed that one, thanks.

What about flag similar to _TIF_SYSCALL_TRACE (call it _TIF_THIS_BEAST_USES_V86 or something), and only do the tests in the slowpath if it is set? As normal applications do not use v86, we could make this 0 instructions in syscall fast path...
Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#17 lore raw
Quoted 5 lines · Pavel Machek

What about flag similar to _TIF_SYSCALL_TRACE (call it _TIF_THIS_BEAST_USES_V86 or something), and only do the tests in the slowpath if it is set? As normal applications do not use v86, we could make this 0 instructions in syscall fast path...

It wouldn't help you. You'd need to mix in two of the values anyway, so at most you'd save one instruction. And the cost would be that anything that has ever used vm86 mode (can you say "X server"?) would be slower. Not a good trade-off.

Oh, I guess you could clear the flag when you know there's no vm86 state anywhere (easy enough, those things never nest), but then it still comes back to "extra complexity that you can get wrong, just to save a single "mov" instruction - that "mov" may have partial-register-stall issues, but still..).

Linus

#18 lore raw
In reply to Linus Torvalds
Quoted 4 lines · Linus Torvalds

That said, the "ldt_ss" case should be moved _after_ the conditional tests, since most CPU's out there will do static prediction based on forward/backwards direction

Btw, Stas, one thing I'd really like to see is even a partial list of anything that actually cares about this. Ie, if there is some known Windows app where Wine works better or something like that, just adding that information to the comments would be hugely appreciated.

Another way of saying the same thing: I absolutely hate seeing patches that fix some theoretical issue that no Linux apps will ever care about. So I'd like to have a bit more of a case for this patch, since I know what the case against it is ;)

Linus

#19 lore raw

Hi,

Quoted 1 line · Linus Torvalds

Btw, Stas, one thing I'd really like to see is even a partial list of anything that actually cares about this. Ie, if there is some known Windows app where Wine works better or something like that, just adding

I am not using Wine too much, but I've
found this:
http://cvs.winehq.org/cvsweb/~checkout~/wine/dlls/winedos/int31.c?rev=1.41&content-type=text/plain
---
/* due to a flaw in some CPUs (at least mine), it is best to mark stack segments as 32-bit if they
can be used in 32-bit code. Otherwise, these CPUs may not set the high word of esp during a ring transition (from kernel code) to the 16-bit stack, and this causes trouble if executing 32-bit code using this stack. */
---
I added win-devel to CC, maybe people there
can tell if that patch has any value for them
or not.
The reference to the original patch:
http://www.uwsg.iu.edu/hypermail/linux/kernel/0503.1/1794.html

Dosemu looks a little better on that, the whole chapter of the docs is dedicated to that problem:
http://dosemu.sourceforge.net/docs/EMUfailure/t1.html#AEN55
There you can find a (relatively small) list of the programs that are affected, but I personally have the old Microsoft linker that crashes, and a few more DOS games.

Quoted 1 line

Another way of saying the same thing: I absolutely hate seeing patches that fix some theoretical issue that no Linux apps will ever care about.

No, it is not theoretical, but it is mainly about a DOS games and an MS linker, as for me. The things I'd like to get working, but the ones you may not care too much about:) The particular game I want to get working, is "Master of Orion 2" for DOS.

Quoted 1 line

So I'd like to have a bit more of a case for this patch, since I know what the case against it is ;)

Yep, and the informational leak it closes, looks also rather minor.
So it is only a matter of how do you care about the dosemu and the DOS games under linux. Considering the amount of the dosemu-related code in vm86.c, I guess you care:)
And uhm, adding the list of the DOS games to the comments of the Linux kernel code, doesn't sound like a good idea to me:)

#20 lore raw
Quoted 10 lines · Stas Sergeev
Quoted 4 lines

Another way of saying the same thing: I absolutely hate seeing patches that fix some theoretical issue that no Linux apps will ever care about.

No, it is not theoretical, but it is mainly about a DOS games and an MS linker, as for me. The things I'd like to get working, but the ones you may not care too much about:) The particular game I want to get working, is "Master of Orion 2" for DOS.

How about you just run it in dosbox instead of dosemu ?

-Andi

#21 lore raw

[BUG?] x86_64 : Can not read /dev/kmem ?

Hi Andi

I tried to read /dev/kmem on x86_64 (linux-2.6.11) and got no success.

read() or pread() returns EINVAL

I tried mmap() too : mmap() calls succeed, but as soon the user process dereference memory, we get :

tinfo: Corrupted page table at address 2aaaaaabf800 PGD 8a983067 PUD c7e5a067 PMD 91588067 PTE ffffffff8048a025 Bad pagetable: 000d [1] SMP
CPU 0

Modules linked in: ipt_REJECT
Pid: 10892, comm: tinfo Not tainted 2.6.11
RIP: 0033:[<0000000000100562>] [<0000000000100562>]
RSP: 002b:00007ffffffff790  EFLAGS: 00010217
RAX: 00002aaaaaabf000 RBX: 00002aaaaabbe000 RCX: 00002aaaaac8fc0c
RDX: 0000000000000001 RSI: 0000000000001000 RDI: 0000000000000000
RBP: 00007ffffffff7f8 R08: 0000000000000003 R09: ffffffff8048a000
R10: 0000000000000001 R11: 0000000000000206 R12: 00000000001005b0
R13: 0000000000000001 R14: 00002aaaaadfdfe8 R15: 0000000000100530
FS:  00002aaaaabcb970(0000) GS:ffffffff804866c0(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00002aaaaaabf800 CR3: 0000000090368000 CR4: 00000000000006e0

Process tinfo (pid: 10892, threadinfo ffff8100901b0000, task ffff8100c7d976c0)

RIP [<0000000000100562>] RSP <00007ffffffff790>

Thank you

Eric Dumazet
----------------------------------------------------------------

# cat tinfo.c

#define _XOPEN_SOURCE 500
#include <unistd.h>
#include <stdio.h>
#include <fcntl.h>

struct tcp_hashinfo {
        struct tcp_ehash_bucket *__tcp_ehash;
        struct tcp_bind_hashbucket *__tcp_bhash;

        int __tcp_bhash_size;
        int __tcp_ehash_size;

} tcp_hashinfo;

#define TCPINFO_ADDR 0xffffffff8048a000 /* tcp_hashinfo */

int main()
{
int fd = open("/dev/kmem", O_RDONLY) ;

if (pread(fd, &tcp_hashinfo, sizeof(tcp_hashinfo), TCPINFO_ADDR) == -1) {
        lseek(fd, TCPINFO_ADDR, 0) ;
        if (read(fd, &tcp_hashinfo, sizeof(tcp_hashinfo)) == -1) {
                perror("Can not read /dev/kmem ?") ;
                return 1 ;
                }
        }
printf("ehash=%p esize=%d bhash=%p bsize=%d\n",
        tcp_hashinfo.__tcp_ehash,
        tcp_hashinfo.__tcp_ehash_size,
        tcp_hashinfo.__tcp_bhash,
        tcp_hashinfo.__tcp_bhash_size) ;

return 0 ;
}

#22 lore raw

Re: [BUG?] x86_64 : Can not read /dev/kmem ?

Sorry for the late answer.

Quoted 6 lines · Eric Dumazet

Hi Andi

I tried to read /dev/kmem on x86_64 (linux-2.6.11) and got no success.

read() or pread() returns EINVAL

Yes. I fixed this once for 2.4, but somehow the changes never made it into 2.6. The VFS code doesn't like negative offsets, and the kernel addresses are negative.

Quoted 3 lines

I tried mmap() too : mmap() calls succeed, but as soon the user process dereference memory, we get :

Hmm, looks like a bug yes. I can reproduce it. Thanks for the report. Will investigate later.

-Andi

#23 lore raw

Re: [BUG?] x86_64 : Can not read /dev/kmem ?

In reply to Eric Dumazet
Quoted 5 lines · Eric Dumazet

Hi Andi

I tried to mmap /dev/kmem on x86_64 (linux-2.6.11) and got no success.

Here's a patch that fixes the problem to me.

Fix mmap of /dev/kmem. It cannot ever have worked before.

vmalloc is still not supported because that would be more complicated.

drivers/char/mem.c renamed from drivers/char/mem.c-o +17 -1
diff -u linux-2.6.11/drivers/char/mem.c-o linux-2.6.11/drivers/char/mem.c
@@ -211,6 +211,23 @@
 	return 0;
 }
 
+static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
+{
+        unsigned long long val;
+	/* 
+	 * RED-PEN: on some architectures there is more mapped memory
+	 * than available in mem_map which pfn_valid checks
+	 * for. Perhaps should add a new macro here.
+	 * 
+	 * RED-PEN: vmalloc is not supported right now. 
+	 */
+	if (!pfn_valid(vma->vm_pgoff))
+		return -EIO;
+	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
+	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT; 
+	return mmap_mem(file, vma);
+}
+
 extern long vread(char *buf, char *addr, unsigned long count);
 extern long vwrite(char *buf, char *addr, unsigned long count);
 
@@ -567,7 +584,6 @@
 	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
 }
 
-#define mmap_kmem	mmap_mem
 #define zero_lseek	null_lseek
 #define full_lseek      null_lseek
 #define write_zero	write_null
#24 lore raw
In reply to Andi Kleen
Quoted 17 lines · Andi Kleen
Quoted 5 lines · Stas Sergeev
Quoted 5 lines

Another way of saying the same thing: I absolutely hate seeing patches that fix some theoretical issue that no Linux apps will ever care about.

Quoted 8 lines

No, it is not theoretical, but it is mainly about a DOS games and an MS linker, as for me. The things I'd like to get working, but the ones you may not care too much about:) The particular game I want to get working, is "Master of Orion 2" for DOS.

Quoted 3 lines

How about you just run it in dosbox instead of dosemu ?

Yes, that's a solution of course, but it is a bit like saying why not use Open Office instead of MS Word.

A long term goal of wine is to support DOS apps to. Of course it's not a priority, but it's there.

regards,
Jakob

#25 lore raw
Quoted 19 lines · Jakob Eriksson
Quoted 14 lines · Andi Kleen
Quoted 11 lines · Stas Sergeev
Quoted 4 lines

Another way of saying the same thing: I absolutely hate seeing patches that fix some theoretical issue that no Linux apps will ever care about.

No, it is not theoretical, but it is mainly about a DOS games and an MS linker, as for me. The things I'd like to get working, but the ones you may not care too much about:) The particular game I want to get working, is "Master of Orion 2" for DOS.

Quoted 2 lines

How about you just run it in dosbox instead of dosemu ?

Quoted 9 lines

Yes, that's a solution of course, but it is a bit like saying why not use Open Office instead of MS Word.

A long term goal of wine is to support DOS apps to. Of course it's not a priority, but it's there.

regards,
Jakob

Can you tell me how the invisible high-word (invisible in VM-86, and in real mode) could possibly harm something running in VM-86 or read-mode ??? I don't even think it's a BUG. If the transition into and out of VM-86 doesn't handle the fact that the high-word of the stack hasn't been used in VM-86, then that piece of code is bad (the SP isn't even the same stack, BTW).

Cheers,
Dick Johnson
Penguin : Linux version 2.6.11 on an i686 machine (5537.79 BogoMips).
Notice : All mail here is now cached for review by Dictator Bush.
98.36% of all statistics are fiction.

#26 lore raw

Hi!

Quoted 3 lines

Can you tell me how the invisible high-word (invisible in VM-86, and in real mode) could possibly harm something running in VM-86 or read-mode ??? I don't even think it's a BUG. If the transition

You can have protected-mode application running in dosemu with 16-bit stack segment.
Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#27 lore raw
In reply to linux-os
Quoted 31 lines · linux-os
Quoted 19 lines · Jakob Eriksson
Quoted 14 lines · Andi Kleen
Quoted 11 lines · Stas Sergeev
Quoted 4 lines

Another way of saying the same thing: I absolutely hate seeing patches that fix some theoretical issue that no Linux apps will ever care about.

No, it is not theoretical, but it is mainly about a DOS games and an MS linker, as for me. The things I'd like to get working, but the ones you may not care too much about:) The particular game I want to get working, is "Master of Orion 2" for DOS.

Quoted 2 lines

How about you just run it in dosbox instead of dosemu ?

Quoted 9 lines

Yes, that's a solution of course, but it is a bit like saying why not use Open Office instead of MS Word.

A long term goal of wine is to support DOS apps to. Of course it's not a priority, but it's there.

regards,
Jakob

Quoted 6 lines

Can you tell me how the invisible high-word (invisible in VM-86, and in real mode) could possibly harm something running in VM-86 or read-mode ??? I don't even think it's a BUG. If the transition into and out of VM-86 doesn't handle the fact that the high-word of the stack hasn't been used in VM-86, then that piece of code is bad (the SP isn't even the same stack, BTW).

Because even in 16-bit mode (real, vm86 or 16-bit protected) you can use 32-bit instructions, with an operand and/or address size override prefix. Of course this only works on 386 or later.

--
Brian Gerst

#28 lore raw

Hello.

Quoted 7 lines · Brian Gerst
Quoted 6 lines

Can you tell me how the invisible high-word (invisible in VM-86, and in real mode) could possibly harm something running in VM-86 or read-mode ??? I don't even think it's a BUG. If the transition into and out of VM-86 doesn't handle the fact that the high-word of the stack hasn't been used in VM-86, then that piece of code is bad (the SP isn't even the same stack, BTW).

Because even in 16-bit mode (real, vm86 or 16-bit protected) you can use 32-bit instructions, with an operand and/or address size override prefix.

And the real problem is when the pure 32bit code is starting to use the 16bit stack for some strange reasons. Looks like the common technique for the early dos4gw -based apps...

#29 lore raw
In reply to Jakob Eriksson

Hi,

Quoted 2 lines · Jakob Eriksson

A long term goal of wine is to support DOS apps to. Of course it's not a priority, but it's there.

Yes, that's exactly what I was hoping for, thanks!
Even if no Windows apps do such a thing (which wasn't confirmed yet), Wine may still need that fix for the DOS support in the future, and dosemu seems to be in need of that fix already and for long (that's where I am getting the use of it). And we haven't heard a word for a VMWare yet, and I think they may appreciate that too.
Also, since the first version of the path, I've been contacted by a few people asking me to provide an updated version for 2.6.9 and 2.6.10. I don't know the reason, but I know it was used (I think they were more concerned about an aforementioned "information leak", rather than about the %esp corruption). So if the last problem with that patch was that it is not really needed, I think it no longer stays:)

#30 lore raw
In reply to Andi Kleen

Hello.

Quoted 3 lines · Andi Kleen
Quoted 2 lines

The particular game I want to get working,
is "Master of Orion 2" for DOS.

How about you just run it in dosbox instead of dosemu ?

Way too slow, and there are other reasons too, like the better networking support on dosemu side (not for Orion, but for other games is important), but that might be an off-topic here:)