Athlon Prefetch workaround for 2.6.0test6
PATCH 1/1 Athlon Prefetch workaround for 2.6.0test6
Here is a new version of the Athlon/Opteron prefetch issue workaround for 2.6.0test6. The issue was hit regularly by the 2.6 kernel while doing prefetches on NULL terminated hlists.
These CPUs sometimes generate illegal exception for prefetch instructions. The operating system can work around this by checking if the faulting instruction is a prefetch and ignoring it when it's the case.
The code is structured carefully to ensure that the page fault will never recurse more than once. Also unmapped EIPs are special cased to give more friendly oopses when the kernel jumps to unmapped addresses.
It removes the previous dumb in kernel workaround for this and shrinks the kernel by >10k.
Small behaviour change is that a SIGBUS fault for a *_user access will cause an EFAULT now, no SIGBUS.
This version addresses all criticism that I got for previous versions.
- Only checks on AMD K7+ CPUs.
- Computes linear address for VM86 mode or code segments with non zero base.
- Some cleanup
- No pointer comparisons
- More comments
Please consider applying,
-Andi
include/asm-i386/processor.h renamed from include/asm-i386/processor.h-PRE +0 -2
@@ -578,8 +589,6 @@#define ARCH_HAS_PREFETCH extern inline void prefetch (const void *x) { -if (cpu_data[0 ].x86_vendor ==X86_VENDOR_AMD ) -return ;/* Some athlons fault if the address is bad */ alternative_input(ASM_NOP4, "prefetchnta (%1)", X86_FEATURE_XMM,
arch/i386/mm/fault.c renamed from arch/i386/mm/fault.c-PRE +123 -9
170 lines not on this page · raw message
Quoted 5 lines · Andi Kleen
+/*
+ * Find an segment base in the LDT/GDT.
+ * Don't need to do any boundary checking because the CPU did that already
+ * when the instruction was executed
+ */
I'm not 100% sure, but I see some possible dangerous race conditions.
1. Userspace thread A has a page fault at CS:EIP == 0x000f:0xbffff000.
Simultaneously, userspace thread B calls modify_ldt() to change the LDT descriptor base to 0x40000000.
The LDT descriptor is changed after A enters the page fault handler, but before it takes mmap_sem. This can happen on SMP or on a pre-empt kernel.
__is_prefetch() calls __get_user(0xfffff000), or whatever address userspace wants to trick it into reading.
Result: unpriveleged userspace causes an MMIO read and crashes the system, or worse.
2. segment_base sets "desc = (u32 *)&cpu_gdt_table[smp_processor_id()]".
Pre-empt switches CPU.
desc now points to the _old_ CPU, where the GDT for this task is no longer valid, and that is read in the next few lines.
I think for completeness you should check the segment type and limit too (because they can be changed, see 1.). These are easier to check than they look (w.r.t. 16 bit segments etc.): you don't have to decode the descriptor; just use the "lar" and "lsl" instructions.
If you can't decode the instruction because of transient access problems, just return as if it was a prefetch, so that the faulting instruction will be retried, or access permissions will cause the program to trap in the normal ways.
-- Jamie
Quoted 1 line
I'm not 100% sure, but I see some possible dangerous race conditions.
Thanks for the review.
Quoted 10 lines
1. Userspace thread A has a page fault at CS:EIP == 0x000f:0xbffff000.
Simultaneously, userspace thread B calls
modify_ldt()to change the LDT descriptor base to0x40000000.The LDT descriptor is changed after A enters the page fault handler, but before it takes
mmap_sem. This can happen on SMP or on a pre-empt kernel.
Quoted 5 lines
__is_prefetch()calls__get_user(0xfffff000), or whatever address userspace wants to trick it into reading.Result: unpriveleged userspace causes an MMIO read and crashes the system, or worse.
Ok, I added access_ok() checks when the fault came from ring 3 code.
That should catch everything.
I guess they should be added to the AMD64 version too. It ignores all bases, but I'm not sure if the CPU catches the case where the linear address computation wraps.
Quoted 4 lines
2.
segment_basesets "desc = (u32 *)&cpu_gdt_table[smp_processor_id()]".Pre-empt switches CPU.
True. Fixed.
Quoted 8 lines
desc now points to the _old_ CPU, where the GDT for this task is no longer valid, and that is read in the next few lines.
I think for completeness you should check the segment type and limit too (because they can be changed, see 1.). These are easier to check than they look (w.r.t. 16 bit segments etc.): you don't have to decode the descriptor; just use the "lar" and "lsl" instructions.
I don't want to do that, the code is already too complicated and I don't plan to reimplement an x86 here (just dealing with this segmentation horror is bad enough)
If it gets any more complicated I would be inclined to just handle the in kernel prefetches using __ex_table entries and give up on user space.
The x86-64 version just ignores all bases, that should be fine too. Anybody who uses non zero code segments likely doesn't care about performance and won't use prefetch ;-)
-Andi
New patch with the fixes attached:
Linus, can you consider merging it?
------------------
Here is a new version of the Athlon/Opteron prefetch issue workaround for 2.6.0test6. The issue was hit regularly by the 2.6 kernel while doing prefetches on NULL terminated hlists.
These CPUs sometimes generate illegal exception for prefetch instructions. The operating system can work around this by checking if the faulting instruction is a prefetch and ignoring it when it's the case.
The code is structured carefully to ensure that the page fault will never recurse more than once. Also unmapped EIPs are special cased to give more friendly oopses when the kernel jumps to unmapped addresses.
It removes the previous dumb in kernel workaround for this and shrinks the kernel by >10k.
Small behaviour change is that a SIGBUS fault for a *_user access will cause an EFAULT now, no SIGBUS.
include/asm-i386/processor.h renamed from include/asm-i386/processor.h-PRE +0 -2
@@ -578,8 +589,6 @@#define ARCH_HAS_PREFETCH extern inline void prefetch (const void *x) { -if (cpu_data[0 ].x86_vendor ==X86_VENDOR_AMD ) -return ;/* Some athlons fault if the address is bad */ alternative_input(ASM_NOP4, "prefetchnta (%1)", X86_FEATURE_XMM,
arch/i386/mm/fault.c renamed from arch/i386/mm/fault.c-PRE +127 -9
174 lines not on this page · raw message
Quoted 1 line · Andi Kleen
Ok, I added
access_ok()checks when the fault came from ring 3 code.
That looks fine.
Quoted 5 lines
Quoted 3 lines
2.
segment_basesets "desc = (u32 *)&cpu_gdt_table[smp_processor_id()]".Pre-empt switches CPU.
True. Fixed.
Where is this fixed?
Quoted 8 lines
Quoted 4 lines
I think for completeness you should check the segment type and limit too (because they can be changed, see 1.). These are easier to check than they look (w.r.t. 16 bit segments etc.): you don't have to decode the descriptor; just use the "lar" and "lsl" instructions.
I don't want to do that, the code is already too complicated and I don't plan to reimplement an x86 here (just dealing with this segmentation horror is bad enough)
I sympathise with that aversion :)
My concern is that, by being complicated enough to decode segments when CS is not equal to either of those, you risk opening a loophole where userspace can trick, if not the kernel (with the access_ok checks), then surrounding userspace virtualisation due to this code in the kernel.
My thinking: Segments can be used by x86 virtualising programs which use a segment to move a "window" of address range accessible to the virtualised code. It should _never_ be possible for the virtualised code, which may be malicious, to trigger reads outside the protected address range by various combinations of threads and triggering LDT modifications in the virtualiser.
Btw, you assume that regs->xcs is a valid segment value. I think that the upper 16 bits are not guaranteed to be zero in general on the IA32, although they clearly are zero for the majority of IA-32 chips. Are they guaranteed to be zero on AMD's processors?
Quoted 3 lines
If it gets any more complicated I would be inclined to just handle the in kernel prefetches using
__ex_tableentries and give up on user space.
__ex_table entries would have been a less controversial fix all along :)
Perhaps it is better to just not decode at all when CS != __KERNEL_CS && CS != __USER_CS? Just fault.
Then you can drop the segment code, which you don't like anyway. Anything as sophisticated as x86 virtualisation in userspace can fixup spurious faults itself. (That isn't perfect, but it's imperfect in a safe way).
If you want to keep the segment base code, then I'll work out a patch on top of yours to do the right access checks. It is quite small, I nearly have it already.
Quoted 3 lines
The x86-64 version just ignores all bases, that should be fine too. Anybody who uses non zero code segments likely doesn't care about performance and won't use prefetch ;-)
Does the x86-64 version ignore bases in 32 bit programs?
-- Jamie
In article <20030929125629.GA1746@averell>, Andi Kleen <[email protected]> wrote:
| It removes the previous dumb in kernel workaround for this and shrinks the | kernel by >10k. | | Small behaviour change is that a SIGBUS fault for a *_user access will | cause an EFAULT now, no SIGBUS. | | This version addresses all criticism that I got for previous versions. | | - Only checks on AMD K7+ CPUs. | - Computes linear address for VM86 mode or code segments | with non zero base. | - Some cleanup | - No pointer comparisons | - More comments
I have to try this on a P4 and K7, but WRT "Only checks on AMD K7+ CPUs" I hope you meant "only generates code if AMD CPU is target" and not that the code size penalty is still there for CPUs which don't need it.
Will check Wednesday, life is very busy right now.
Signature · 3 lines
bill davidsen <[email protected]> CTO, TMR Associates, Inc Doing interesting things with little computers since 1979.
Quoted 3 lines · Andi Kleen
I guess they should be added to the AMD64 version too. It ignores all bases, but I'm not sure if the CPU catches the case where the linear address computation wraps.
The linear address is _allowed_ to wrap on x86, and there are real DOS-era programs which take advantage of this. It is used to create the illusion of access to high addresses, by making them wrap to low ones which can be mapped.
Some DOS extenders did this so that 32-bit programs could access BIOS and video memory in the same DS segment as normal code, despite having a large segment base address so that null pointers would be trapped by page protection.
Of course no linux programs do this :)
(Well, maybe some do under DOSEMU?)
So it seems quite likely that the AMD64 CPU simply allows wrapping in the linear address calculation.
-- Jamie
arch/i386/mm/fault.c line 97
Quoted 7 lines · Andi Kleen
arch/i386/mm/fault.c renamed from arch/i386/mm/fault.c-PRE +7
-0+/* +* Avoid recursive faults. This catches the kernel jumping to nirvana. +* More complicated races with unmapped EIP are handled elsewhere for +* user space. +*/ +if (regs->eip == addr) +return 0 ;
I'm curious - how does this help?
If the kernel jumps into nirvana, it will page fault. is_prefetch() will do a __get_user which will fault - recursion, ok. The inner fault handler will reach fixup_exception(), fixup the __get_user, and return without recursing further. is_prefetch() will simply return.
So how does the above test help?
arch/i386/mm/fault.c line 77
Quoted 8 lines
arch/i386/mm/fault.c renamed from arch/i386/mm/fault.c-PRE +8
-0+if (seg & (1 <<2 )) + desc = current->mm->context.ldt; +else + desc = (u32 *)&cpu_gdt_table[smp_processor_id ()]; + desc = (void *)desc + (seg & ~7 ); +return (desc[0 ] >>16 ) | + ((desc[1 ] &0xFF ) <<16 ) | + (desc[1 ] &0xFF000000 );
In addition to needing get_cpu() to protect the GDT access, this code needs to take down(¤t->mm->context.sem) for the LDT access.
Otherwise, context.ldt may have been vfree()'d by the time you use it, and the desc[0..1] accesses will panic.
Thanks,
-- Jamie
Quoted 12 lines · bill davidsen
In article <20030929125629.GA1746@averell>, Andi Kleen <[email protected]> wrote:
| It removes the previous dumb in kernel workaround for this and shrinks the | kernel by >10k. | | Small behaviour change is that a SIGBUS fault for a *_user access will | cause an EFAULT now, no SIGBUS. | | This version addresses all criticism that I got for previous versions. | | - Only checks on AMD K7+ CPUs. | - Computes linear address for VM86 mode or code segments | with non zero base. | - Some cleanup | - No pointer comparisons | - More comments
Quoted 6 lines
I have to try this on a P4 and K7, but WRT "Only checks on AMD K7+ CPUs" I hope you meant "only generates code if AMD CPU is target" and not that the code size penalty is still there for CPUs which don't need it.
Will check Wednesday, life is very busy right now.
No, the code is not conditionally compiled. That is a different issue to this patch though. The target CPU selection scheme doesn't work at all like you would expect and its impossible to compile this sort of code out (when on x86 arch). See Adrian's code to rationalise cpu selection.
Quoted 9 lines · Jamie Lokier
Quoted 3 lines · Andi Kleen
I guess they should be added to the AMD64 version too. It ignores all bases, but I'm not sure if the CPU catches the case where the linear address computation wraps.
The linear address is _allowed_ to wrap on x86, and there are real DOS-era programs which take advantage of this. It is used to create the illusion of access to high addresses, by making them wrap to low ones which can be mapped.
Only when the A20 gate is set (?), which it never is in Linux.
-Andi
Quoted 6 lines · Jamie Lokier
My thinking: Segments can be used by x86 virtualising programs which use a segment to move a "window" of address range accessible to the virtualised code. It should _never_ be possible for the virtualised code, which may be malicious, to trigger reads outside the protected address range by various combinations of threads and triggering LDT modifications in the virtualiser.
Agreed. But access_ok() takes care of that. It does not guarantee that the right instruction is checked, but whoever uses non zero code segment bases can add their own checker or more likely not use prefetch at all (it is likely ancient legacy code code anyways).
Quoted 4 lines
Btw, you assume that
regs->xcsis a valid segment value. I think that the upper 16 bits are not guaranteed to be zero in general on the IA32, although they clearly are zero for the majority of IA-32 chips. Are they guaranteed to be zero on AMD's processors?
That would be new to me. Can you quote a line that says that from the architecture manual?
Also remember the code only runs on AMD.
Quoted 8 lines
Quoted 3 lines
If it gets any more complicated I would be inclined to just handle the in kernel prefetches using
__ex_tableentries and give up on user space.
__ex_tableentries would have been a less controversial fix all along :)Perhaps it is better to just not decode at all when CS !=
__KERNEL_CS&& CS !=__USER_CS? Just fault.
Yep, I considered that too.
I think I will do that for x86-64 and 32bit can handle it all in user space as I suspect the patch is unmergeable anyways.
Quoted 5 lines
Quoted 3 lines
The x86-64 version just ignores all bases, that should be fine too. Anybody who uses non zero code segments likely doesn't care about performance and won't use prefetch ;-)
Does the x86-64 version ignore bases in 32 bit programs?
The CPU doesn't, the prefetch fix does.
I will add a check for __USER32_CS/__USER_CS and __KERNEL_CS to make sure it will never wrap.
I'm dropping the original segment checking/code checking proposal for 32bit. It obviously was misguided and Linus was right from the beginning to reject it.
-Andi
Quoted 4 lines · Jamie Lokier
Btw, you assume that
regs->xcsis a valid segment value. I think that the upper 16 bits are not guaranteed to be zero in general on the IA32, although they clearly are zero for the majority of IA-32 chips. Are they guaranteed to be zero on AMD's processors?
At least for pushes of segment registers a 486 decrements the stack pointer by 4 but only writes the 2 least significant bytes, leaving garbage in the upper half.
That's documented in the list of differences between 486 and Pentium. It may be different for the frame pushed on the stack during interrupt entry; my take on it is that it is inherently dangerous to rely on the upper 16 bits being zero.
Just my 2 cents,
Regards,
Gabriel
Quoted 17 lines · bill davidsen
In article <20030929125629.GA1746@averell>, Andi Kleen <[email protected]> wrote:
| It removes the previous dumb in kernel workaround for this and shrinks the | kernel by >10k. | | Small behaviour change is that a SIGBUS fault for a *_user access will | cause an EFAULT now, no SIGBUS. | | This version addresses all criticism that I got for previous versions. | | - Only checks on AMD K7+ CPUs. | - Computes linear address for VM86 mode or code segments | with non zero base. | - Some cleanup | - No pointer comparisons | - More comments
Quoted 3 lines
I have to try this on a P4 and K7, but WRT "Only checks on AMD K7+ CPUs" I hope you meant "only generates code if AMD CPU is target" and not that the code size penalty is still there for CPUs which don't need it.
NO NO NO NO NO.
This *has* to be there on a P4 kernel too, as we can now boot those on a K7 too. The 'code size penalty' you talk about is in the region of a few hundred bytes. Much less than a page. There are far more obvious bloat candidates.
Dave
Signature · 1 line
Dave Jones http://www.codemonkey.org.uk
Quoted 11 lines · Dave Jones
Quoted 5 lines · bill davidsen
In article <20030929125629.GA1746@averell>, Andi Kleen <[email protected]> wrote:
I have to try this on a P4 and K7, but WRT "Only checks on AMD K7+ CPUs" I hope you meant "only generates code if AMD CPU is target" and not that the code size penalty is still there for CPUs which don't need it.
NO NO NO NO NO.
This *has* to be there on a P4 kernel too, as we can now boot those on a K7 too. The 'code size penalty' you talk about is in the region of a few hundred bytes. Much less than a page. There are far more obvious bloat candidates.
Clearly you don't do embedded or small applications, and take the attitude that people should try to find hundreds of bytes elsewhere to compensate for the bytes you want to force into the kernel to support a bugfix for one vendor.
You can build a 386 kernel which will run fine on K7 if you are too limited in resources to build a proper matching kernel for the target system. The attitude that there is bloat in the kernel already so we can just add more as long as it doesn't inconvenience you is totally at odds with the long tradition of Linux being lean and mean.
Linus wouldn't let dump to disk in the kernel because it wasn't generally useful, I am amazed that he is letting 300 bytes of vendor bugfix which is totally without value to any other system go in without ifdef's to limit the memory penalty to the relevant CPUs.
Signature · 3 lines
bill davidsen <[email protected]> CTO, TMR Associates, Inc Doing interesting things with little computers since 1979.