espfix code cleanup more
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 espfix code cleanup more
Hi Andrew.
Attached is a micro-optimization on top of the previous cleanup patch. It removes the redundant arguments from the C functions. No functionality changes.
arch/i386/kernel/traps.c +4 -5
@@ -1018,13 +1018,13 @@#endif } -fastcall unsigned long patch_espfix_gdt (struct pt_regs *regs, +fastcall unsigned long patch_espfix_base (unsigned long uesp,unsigned long kesp) {int cpu =smp_processor_id ();struct Xgt_desc_struct *cpu_gdt_descr = &per_cpu (cpu_gdt_descr, cpu);struct desc_struct *gdt = (struct desc_struct *)cpu_gdt_descr->address; -unsigned long base = (kesp - regs->esp) & -THREAD_SIZE ; +unsigned long base = (kesp - uesp) & -THREAD_SIZE ;__u64 desc = *(__u64 *)&gdt[GDT_ENTRY_ESPFIX_SS ];/* Set up base for espfix segment */ desc &=0x00ffff000000ffffULL ;
@@ -1034,14 +1034,13 @@return kesp - base; } -fastcall unsigned long get_orig_kesp (unsigned long kesp,unsigned long cpu) +fastcall unsigned long get_espfix_base (unsigned long cpu) {/* Since we are on a wrong stack, the smp_processor_id() cannot * be used. So the cpu number is passed from an assembly. */ struct Xgt_desc_struct *cpu_gdt_descr = &per_cpu (cpu_gdt_descr, cpu);struct desc_struct *gdt = (struct desc_struct *)cpu_gdt_descr->address; -unsigned long base =get_desc_base (&gdt[GDT_ENTRY_ESPFIX_SS ].a); -return base + kesp; +return get_desc_base (&gdt[GDT_ENTRY_ESPFIX_SS ].a); }/*
arch/i386/kernel/entry.S +5 -5
@@ -401,9 +401,9 @@ * This is an "official" bug of all the x86-compatible * CPUs, which we can try to work around to make * dosemu and wine happy. */ - movl %esp, %eax # pt_regs pointer + movl OLDESP(%esp), %eax movl %esp, %edx - call patch_espfix_gdt + call patch_espfix_base pushl $__ESPFIX_SS CFI_ADJUST_CFA_OFFSET 4 pushl %eax
@@ -502,10 +502,10 @@ CFI_ENDPROC #define FIXUP_ESPFIX_STACK \ - movl %esp, %eax; \ GET_THREAD_INFO(%ebp); \ - movl TI_cpu(%ebp), %edx; \ - call get_orig_kesp; \ + movl TI_cpu(%ebp), %eax; \ + call get_espfix_base; \ + addl %esp, %eax; \ pushl $__KERNEL_DS; \ pushl %eax; \ lss (%esp), %esp;
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: + espfix-code-cleanup.patch added to -mm tree
Hi Zach and thanks for you really great review!
Quoted 1 line · Zachary Amsden
Overall, this looks like a great cleanup. But I think there are some details with the patch that need fixing. It is extremely hard to follow the
entry.Schanges based on patches alone - which tree does this apply cleanly to?
This should cleanly apply to 2.6.18-rc2-mm1.
Quoted 1 line
I would like to see the final result in
entry.S, there are
I'll send it to you privately.
Quoted 1 line
still some details (dealing with eliminating the push/pop of %eax as a temporary for the segment comparison) which I could not follow exactly in patch format.
The reason for removal was that I also removed the xorl %eax,%eax, decl %eax stuff from error_code (replaced with $-1, as you've seen), and so preserving %eax became unnecessary I think.
Quoted 8 lines
Review
Quoted 7 lines
iret_exc:(quoted diff) +0
-2- TRACE_IRQS_ON - sti pushl $0 # no error code pushl $do_iret_error jmp error_codeWhy did you remove the sti here?
Because I simply broke my head over that code. If you say sti should stay - I beleive you. But please, explain me how it works, before I went crazy! :)
I read the iret pseudo-code in the Intel manuals carefully. It first pops the iret frame into temporaries, then checks the values for validity, and then either faults or writes the values to the real registers. Now, if we have a bad user's iret frame, the iret will pop it, raise an exception and push a kernel's iret frame, while the popped user's frame at that point should be completely lost, as far as I can understand. Now we go into GPF handler, then to a fixup, push $0 as an error code, but what iret frame is above that error code? Where does it come from? Doesn't the iret loses the user's iret frame completely? Certainly I am missing something obvious here, but I just can't follow that magic...
Quoted 1 line
Did you test this with deliberately faulting IRETs? I think this is a bug.
Yes, I tested that and it works (I have a rather extensive test-suite for that problem - faulting iret is always tested). But since I don't understand the magic, I can't say why it works.
Quoted 4 lines
Review
Quoted 3 lines
pushl %eax; \ <----
(quoted diff) +1
-1- CFI_ADJUST_CFA_OFFSET 4; \ + lss (%esp), %esp;<---- CFI adjustment here is missing.
Well, someone used that macro in a .fixup section, where the CFI adjustments do not seem to work. But since I don't know why this was done (Jan?), I reverted that to my original code and added the adjustments now.
Quoted 1 line
Bigger problem! (*!) It is _unsafe_ to call C-code here.
Ow, holly shit!!! What a disaster... But you are right. Well, this will make my new patch really big...
Quoted 1 line
You did not introduce this bug, I just spotted it now, and the old code has the same problem.
You are wrong here - I introduced that bug as the old code is also mine. Linus attributed it to someone else though, probably by mistake (or did he just wanted to rescue me from the rant about that code? :). But it was mine.
Quoted 2 lines
There are two options I see to fixing this problem. One is to move the GDT fixup code into assembler. The other is to compile the GDT fixup code in a separate .o file with exactly specified CFLAGS to make sure -fomit-frame-pointer is not passed to it.
Well, unsafe is unsafe. If we can't explicitly tell gcc to prefix the %ebp accesses with %ds, then I'd better go for an asm version.
Quoted 4 lines
Review
Quoted 3 lines
FIXUP_ESPFIX_STACK # %eax == %esp(quoted diff) +1
-1- CFI_ADJUST_CFA_OFFSET -20 # the frame has now moved + CFI_ADJUST_CFA_OFFSET -20Is this CFI adjustment still correct?
Hmm, I guess it wasn't correct also before, so I just left it as is. Should now be fixed.
Quoted 3 lines
Review
Quoted 2 lines
(quoted diff) +1
-1- .quad 0x0000920000000000 /* 0xd0 - ESPFIX 16-bit SS */ + .quad 0x00cf92000000ffff /* 0xd0 - ESPFIX SS */Seems a bit dangerous to allow access to full 4GB through this. Can you tighten the limit any? I suppose not, because the high bits in %esp really could be anything. But it might be nice to try setting the limit to
regs->esp+THREAD_SIZE. Of course, this is not strictly necessary, just an extra paranoid protection mechanism.
Since, when calculating the base, I do &-THREAD_SIZE, I guess the minimal safe limit is regs->esp + THREAD_SIZE*2... Well, may just I not do that please? :) For what, btw? There are no such a things for __KERNEL_DS or anything, so I just don't see the necessity.
The new patch is attached. Hopefully I addressed all of your concerns. Thanks!
percpu.h +21 -0
@@ -1,6 +1,27 @@#ifndef __ARCH_I386_PERCPU__#define __ARCH_I386_PERCPU__ +#ifndef __ASSEMBLY__#include <asm-generic/percpu.h> +#else + +/* +* PER_CPU finds an address of a per-cpu variable. +* +* Args: +* var - variable name +* cpu - 32bit register containing the current CPU number +* +* The resulting address is stored in the "cpu" argument. +* +* Example: +* PER_CPU(cpu_gdt_descr, %ebx) +*/ +#define PER_CPU (var, cpu) \ + shll $2, cpu; \ + movl __per_cpu_offset(cpu), cpu; \ + addl $per_cpu__/**/ var, cpu; + +#endif /* !__ASSEMBLY__ */ #endif /* __ARCH_I386_PERCPU__ */
include/asm-i386/desc.h +23 -4
@@ -4,8 +4,6 @@#include <asm/ldt.h> #include <asm/segment.h> -#define CPU_16BIT_STACK_SIZE 1024 -#ifndef __ASSEMBLY__#include <linux/preempt.h>
@@ -16,8 +14,6 @@extern struct desc_struct cpu_gdt_table[GDT_ENTRIES ]; -DECLARE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]); -struct Xgt_desc_struct {unsigned short size;unsigned long address __attribute__((packed));
@@ -162,6 +158,29 @@return base; } +#else /* __ASSEMBLY__ */ + +/* +* GET_DESC_BASE reads the descriptor base of the specified segment. +* +* Args: +* idx - descriptor index +* gdt - GDT pointer +* base - 32bit register to which the base will be written +* lo_w - lo word of the "base" register +* lo_b - lo byte of the "base" register +* hi_b - hi byte of the low word of the "base" register +* +* Example: +* GET_DESC_BASE(GDT_ENTRY_ESPFIX_SS, %ebx, %eax, %ax, %al, %ah) +* Will read the base address of GDT_ENTRY_ESPFIX_SS and put it into %eax. +*/ +#define GET_DESC_BASE (idx, gdt, base, lo_w, lo_b, hi_b) \ + movb idx*8+4(gdt), lo_b; \ + movb idx*8+7(gdt), hi_b; \ + shll $16, base; \ + movw idx*8+2(gdt), lo_w; +#endif /* !__ASSEMBLY__ */ #endif
arch/i386/kernel/head.S +1 -1
@@ -590,7 +590,7 @@ .quad 0x00009a000000ffff /* 0xc0 APM CS 16 code (16 bit) */ .quad 0x004092000000ffff /* 0xc8 APM DS data */ - .quad 0x0000920000000000 /* 0xd0 - ESPFIX 16-bit SS */ + .quad 0x00cf92000000ffff /* 0xd0 - ESPFIX SS */ .quad 0x0000000000000000 /* 0xd8 - unused */ .quad 0x0000000000000000 /* 0xe0 - unused */ .quad 0x0000000000000000 /* 0xe8 - unused */
arch/i386/kernel/traps.c +12 -41
61 lines not on this page · raw message
arch/i386/kernel/entry.S +33 -40
145 lines not on this page · raw message
arch/i386/kernel/asm-offsets.c +5 -0
@@ -58,6 +58,11 @@OFFSET (TI_sysenter_return, thread_info, sysenter_return);BLANK (); +OFFSET (GDS_size, Xgt_desc_struct, size); +OFFSET (GDS_address, Xgt_desc_struct, address); +OFFSET (GDS_pad, Xgt_desc_struct, pad); +BLANK (); +OFFSET (EXEC_DOMAIN_handler, exec_domain, handler);OFFSET (RT_SIGFRAME_sigcontext, rt_sigframe, uc.uc_mcontext);BLANK ();
arch/i386/kernel/cpu/common.c +0 -11
@@ -24,9 +24,6 @@ DEFINE_PER_CPU(struct Xgt_desc_struct, cpu_gdt_descr);EXPORT_PER_CPU_SYMBOL (cpu_gdt_descr); -DEFINE_PER_CPU(unsigned char, cpu_16bit_stack[CPU_16BIT_STACK_SIZE]); -EXPORT_PER_CPU_SYMBOL (cpu_16bit_stack); -static int cachesize_override__cpuinitdata =-1 ;static int disable_x86_fxsr__cpuinitdata ;static int disable_x86_serial_nr__cpuinitdata =1 ;
@@ -594,7 +591,6 @@struct tss_struct * t = &per_cpu (init_tss, cpu);struct thread_struct *thread = ¤t->thread;struct desc_struct *gdt; -__u32 stk16_off = (__u32)&per_cpu (cpu_16bit_stack, cpu);struct Xgt_desc_struct *cpu_gdt_descr = &per_cpu (cpu_gdt_descr, cpu);if (cpu_test_and_set (cpu, cpu_initialized)) {
@@ -642,13 +638,6 @@* and set up the GDT descriptor: */ memcpy (gdt, cpu_gdt_table,GDT_SIZE ); - -/* Set up GDT entry for 16bit stack */ - *(__u64 *)(&gdt[GDT_ENTRY_ESPFIX_SS ]) |= - ((((__u64 )stk16_off) <<16 ) &0x000000ffffff0000ULL ) | - ((((__u64 )stk16_off) <<32 ) &0xff00000000000000ULL ) | - (CPU_16BIT_STACK_SIZE -1 ); - cpu_gdt_descr->size =GDT_SIZE -1 ; cpu_gdt_descr->address = (unsigned long )gdt;
Re: + espfix-code-cleanup.patch added to -mm tree
Quoted 8 lines
Quoted 4 lines
Review
Quoted 3 lines
pushl %eax; \ <----
(quoted diff) +1
-1- CFI_ADJUST_CFA_OFFSET 4; \ + lss (%esp), %esp;<---- CFI adjustment here is missing.
Well, someone used that macro in a .fixup section, where the CFI adjustments do not seem to work. But since I don't know why this was done (Jan?), I reverted that to my original code and added the adjustments now.
The macro in question is UNWIND_ESPFIX_STACK, which is used in exactly one place (in normal .text). Even more, the macro itself switches to .fixup,
so it would be rather odd if it was used in a .fixup section by itself. Note
that FIXUP_ESPFIX_STACK doesn't have any annotations, and hence can freely be used in .fixup.
Quoted 6 lines
Quoted 4 lines
Review
Quoted 3 lines
FIXUP_ESPFIX_STACK # %eax == %esp(quoted diff) +1
-1- CFI_ADJUST_CFA_OFFSET -20 # the frame has now moved + CFI_ADJUST_CFA_OFFSET -20Is this CFI adjustment still correct?
Hmm, I guess it wasn't correct also before, so I just left it as is. Should now be fixed.
I would think it was correct, but surely annotating these pieces of code
wasn't something that anybody but the original author (you) should have
done, as it wasn't too difficult to get lost.
Jan
Re: + espfix-code-cleanup.patch added to -mm tree
Hi.
Quoted 5 lines · Jan Beulich
Quoted 4 lines
Well, someone used that macro in a .fixup section, where the CFI adjustments do not seem to work. But since I don't know why this was done (Jan?), I reverted that to my original code and added the adjustments now.
The macro in question is
UNWIND_ESPFIX_STACK, which is used in exactly
No, that was about FIXUP_ESPFIX_STACK in fact.
Quoted 1 line
Even more, the macro itself switches to .fixup,
... where it uses FIXUP_ESPFIX_STACK. I haven't done that. Someone else added the .fixup section to UNWIND_ESPFIX_STACK, and so the FIXUP_ESPFIX_STACK became used in that section. I removed that now with my patch, unless someone can tell me why it was needed.
Quoted 3 lines
Note
thatFIXUP_ESPFIX_STACKdoesn't have any annotations, and hence can freely be used in .fixup.
Yes, but now the annotations had to be added, and so was the problem.
Quoted 3 lines
Quoted 2 lines
Hmm, I guess it wasn't correct also before, so I just left it as is. Should now be fixed.
I would think it was correct,
But why? FIXUP_ESPFIX_STACK doesn't pop up 20 bytes or something, it just used to copy the entire stack frame from 16bit to 32bit stack. It is much more than 20 bytes, but at the end, %ss:%esp points to the very same data. So where exactly did 20 come from? Well, historical interest mainly, as that code is going to die when we agree on the patch, and the new code is much cleaner and won't raise such a questions.
Quoted 3 lines
but surely annotating these pieces of
code wasn't something that anybody but the original author (you) should have done, as it wasn't too difficult to get lost.
When it was written, such an annotations were not needed yet, so I couldn't do that.
Re: + espfix-code-cleanup.patch added to -mm tree
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Well, someone used that macro in a .fixup section, where the CFI adjustments do not seem to work. But since I don't know why this was done (Jan?), I reverted that to my original code and added the adjustments now.
The macro in question is
UNWIND_ESPFIX_STACK, which is used in
exactly
Quoted 8 lines
No, that was about
FIXUP_ESPFIX_STACKin fact.Quoted 1 line
Even more, the macro itself switches to .fixup,
... where it uses
FIXUP_ESPFIX_STACK. I haven't done that. Someone else added the .fixup section toUNWIND_ESPFIX_STACK, and so theFIXUP_ESPFIX_STACKbecame used in that section. I removed that now with my patch, unless someone can tell me why it was needed.
That was me, in order to get the unwind annotations right without complicating the code too much. Again, FIXUP_ESPFIX_STACK doesn't use any unwind directives so can be used anywhere, including the .fixup section UNWIND_ESPFIX_STACK switches to.
Jan
Re: + espfix-code-cleanup.patch added to -mm tree
Hi.
Quoted 4 lines · Jan Beulich
That was me, in order to get the unwind annotations right without complicating the code too much. Again,
FIXUP_ESPFIX_STACKdoesn't use any unwind directives so can be used anywhere, including the .fixup sectionUNWIND_ESPFIX_STACKswitches to.
Yes, but the new version of FIXUP_ESPFIX_STACK _will_ use the annotations (that was Zach's complain), and therefore it can't be used in a .fixup, and so the new UNWIND_ESPFIX_STACK _will not_ use the .fixup, just like it was before your change. I am not saying that your change was wrong, but now I had to undo it. Let me know if that makes a problem.
Re: + espfix-code-cleanup.patch added to -mm tree
In reply to Stas SergeevQuoted 38 lines · Stas Sergeev
Hi Zach and thanks for you really great review!
Quoted 1 line · Zachary Amsden
Overall, this looks like a great cleanup. But I think there are some details with the patch that need fixing. It is extremely hard to follow the
entry.Schanges based on patches alone - which tree does this apply cleanly to?This should cleanly apply to 2.6.18-rc2-mm1.
Quoted 1 line
I would like to see the final result in
entry.S, there areI'll send it to you privately.
Quoted 1 line
still some details (dealing with eliminating the push/pop of %eax as a temporary for the segment comparison) which I could not follow exactly in patch format.
The reason for removal was that I also removed the xorl %eax,%eax, decl %eax stuff from
error_code(replaced with $-1, as you've seen), and so preserving %eax became unnecessary I think.Quoted 8 lines
Review
Quoted 7 lines
iret_exc:(quoted diff) +0
-2- TRACE_IRQS_ON - sti pushl $0 # no error code pushl $do_iret_error jmp error_codeWhy did you remove the sti here?
Because I simply broke my head over that code. If you say sti should stay - I beleive you. But please, explain me how it works, before I went crazy! :)
I read the iret pseudo-code in the Intel manuals carefully. It first pops the iret frame into temporaries, then checks the values for validity, and then either faults or writes the values to the real registers. Now, if we have a bad user's iret frame, the iret will pop it, raise an exception and push a kernel's iret frame, while the popped user's frame at that point should be completely lost, as far as I can understand. Now we go into GPF handler, then to a fixup, push $0 as an error code, but what iret frame is above that error code? Where does it come from? Doesn't the iret loses the user's iret frame completely? Certainly I am missing something obvious here, but I just can't follow that magic...
Quoted 4 lines
Quoted 1 line
Did you test this with deliberately faulting IRETs? I think this is a bug.
Yes, I tested that and it works (I have a rather extensive test-suite for that problem - faulting iret is always tested). But since I don't understand the magic, I can't say why it works.
You need to get a #GP or #NP on the faulting iret. Several ways to do that -
1) Issue int $0x80 from last two bytes of an LDT code segment with truncated limit (#GP)
2) Move %ss to LDT, set %esp high. Issue write LDT system call via int $0x80 that truncates the limit below %esp (#GP)
3) Move %cs / %ss to LDT. Issue write LDT system call and make cs / ss segment not present (#NP)
Note that on the path back to the iret, interrupts are disabled. The iret faults, but doesn't pop the user return frame. The check for the fixup section will happen, and fixup_exception will return to the STI instruction in the fixup, with the user frame still above on the stack (but note the return pops EFLAGS, which disables interrupts as they were before for the iret). The fixup returns, pushes a fake error code onto the stack, followed by a push of the exception handle, do_iret_error. It then re-enters the common error_code path, with saves registers and dispatches to do_iret_error, which follows the notify path, either killing the process with a SIGSEGV or delivering a signal. The whole time the user return frame is sitting up there on the stack, so taking this exception:
DO_ERROR_INFO(32, SIGSEGV, "iret exception", iret_error, ILL_BADSTK, 0)
makes it appear as if the user instruction to which the kernel was trying to return caused a SIGSEGV / ILL_BADSTK.
I think you actually want interrupts enabled in that path, since if you look at the #GP handler, it is a trap gate, not an interrupt gate - so #GPs from userspace that take a very similar signal notifier path enter with interrupts enabled. I don't think very much would go wrong if interrupts were left disabled, but it makes me nervous to change that semantic.
Quoted 9 lines
Quoted 1 line
Bigger problem! (*!) It is _unsafe_ to call C-code here.
Ow, holly shit!!! What a disaster... But you are right. Well, this will make my new patch really big...
Quoted 1 line
You did not introduce this bug, I just spotted it now, and the old code has the same problem.
You are wrong here - I introduced that bug as the old code is also mine. Linus attributed it to someone else though, probably by mistake (or did he just wanted to rescue me from the rant about that code? :). But it was mine.
Ah, ok.
Quoted 3 lines
Quoted 1 line
There are two options I see to fixing this problem. One is to move the GDT fixup code into assembler. The other is to compile the GDT fixup code in a separate .o file with exactly specified CFLAGS to make sure -fomit-frame-pointer is not passed to it.
Well, unsafe is unsafe. If we can't explicitly tell gcc to prefix the %ebp accesses with %ds, then I'd better go for an asm version.
Sounds like a plan.
Quoted 8 lines
Quoted 4 lines
arch/i386/kernel/head.S line 593
Quoted 3 lines
arch/i386/kernel/head.S +1
-1- .quad 0x0000920000000000 /* 0xd0 - ESPFIX 16-bit SS */ + .quad 0x00cf92000000ffff /* 0xd0 - ESPFIX SS */Seems a bit dangerous to allow access to full 4GB through this. Can you tighten the limit any? I suppose not, because the high bits in %esp really could be anything. But it might be nice to try setting the limit to
regs->esp+THREAD_SIZE. Of course, this is not strictly necessary, just an extra paranoid protection mechanism.Since, when calculating the base, I do &-THREAD_SIZE, I guess the minimal safe limit is
regs->esp+ THREAD_SIZE*2... Well, may just I not do that please? :) For what, btw? There are no such a things for__KERNEL_DSor anything, so I just don't see the necessity.
It helps track down any bugs that could leak through otherwise and corrupt random memory.
Re: + espfix-code-cleanup.patch added to -mm tree
Hi,
Quoted 1 line · Zachary Amsden
You need to get a #GP or #NP on the faulting iret. Several ways to do that -
I do that much simpler - I provoke a SIGSEGV and in a signal handler I put the wrong value to scp->cs or scp->ss, and that makes iret to fault.
Quoted 1 line
iret faults, but doesn't pop the user return frame.
But does it push the kernel frame after it or not? If not - I don't understand how we go to a fixup. If yes - I don't understand how the user's frame gets accessed later, as it is above the kernel's frame.
Quoted 4 lines
Quoted 3 lines
safe limit is
regs->esp+ THREAD_SIZE*2... Well, may just I not do that please? :) For what, btw? There are no such a things for__KERNEL_DSor anything, so I just don't see the necessity.It helps track down any bugs that could leak through otherwise and corrupt random memory.
I think regs->esp + THREAD_SIZE*2 is already very permissive, and I'd like to avoid messing with granularity. So unless you really insist, I'll better not do that. :)
Re: + espfix-code-cleanup.patch added to -mm tree
Quoted 6 lines · Stas Sergeev
Hi,
Quoted 1 line · Zachary Amsden
You need to get a #GP or #NP on the faulting iret. Several ways to do that -
I do that much simpler - I provoke a SIGSEGV and in a signal handler I put the wrong value to
scp->csorscp->ss, and that makes iret to fault.
Ok, that's a new trick ;)
Quoted 6 lines
Quoted 2 lines
iret faults, but doesn't pop the user return frame.
But does it push the kernel frame after it or not? If not - I don't understand how we go to a fixup. If yes - I don't understand how the user's frame gets accessed later, as it is above the kernel's frame.
Yes. The iret faults, the fault pushes a new kernel frame - and the fault handler's iret returns, removing the kernel frame. So the kernel frame is gone by the time the fixup runs.
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
safe limit is
regs->esp+ THREAD_SIZE*2... Well, may just I not do that please? :) For what, btw? There are no such a things for__KERNEL_DSor anything, so I just don't see the necessity.It helps track down any bugs that could leak through otherwise and corrupt random memory.
I think
regs->esp+ THREAD_SIZE*2 is already very permissive, and I'd like to avoid messing with granularity. So unless you really insist, I'll better not do that. :)
It's really hard to catch bugs that could otherwise happen when a non-zero based stack gets used (for example, C code which uses %ebp with -fomit-frame-pointer). Setting the limit to THREAD_SIZE should guarantee that the non-zero based stack never is used to access anything but the stack and current thread.
Zach
Re: + espfix-code-cleanup.patch added to -mm tree
Hi.
Quoted 1 line · Zachary Amsden
Yes. The iret faults, the fault pushes a new kernel frame - and the fault handler's iret returns, removing the kernel frame. So the kernel frame is gone by the time the fixup runs.
OK, thanks! I wasn't completely realizing that the fixup runs after an exception handler is already returned. Now it all looks pretty obvious. :)
Quoted 3 lines
It's really hard to catch bugs that could otherwise happen when a non-zero based stack gets used (for example, C code which uses %ebp with -fomit-frame-pointer). Setting the limit to
THREAD_SIZEshould
guarantee that the non-zero based stack never is used to access anything but the stack and current thread.
Yes, be there a possibility the set the *constant* limit (THREAD_SIZE), I'd certainly do that, no questions. But as long as we are talking about the nasty non-constant limit like regs->esp+THREAD_SIZE*2, is it really worth an efforts? This limit is very unpredictable. I'll have to add the code to deal with granularity. And its still very, very permissive. Not even nearly something like just THREAD_SIZE. Do you really, really think it is worth all the headache?