Messages in this thread Patch in this message | | | Date | Tue, 03 May 2005 21:20:44 +0400 | From | Stas Sergeev <> | Subject | Re: 2.6.12-rc3 OOPS in vanilla source (once more) |
| |
Hello.
Alexander Nyberg wrote: > So, my solution is to instead of just adjusting esp0 that creates an > inconsitent state I adjust where the user-space registers are saved with > -8 bytes. When I did that offending patch, I was thinking the following way: - Do we need to adjust that initial copy of child regs by the 8 bytes too? - Well, we need that 8 bytes only when the "struct pt_regs" is incomplete. Here we copy the *complete* "struct pt_regs", so shifting that here makes no sense.
And so I adjusted only esp0 and nothing else. I think this may actually still be valid.
> This gives us the wanted extra bytes on the start of the stack > and esp0 is now correct. Yes, it is now correct by the mean that it points to the top of the "struct pt_regs" on the thread startup. However, it is not *always* points to the top of the "struct pt_regs". This -8 means exactly that esp0 can also point 8 bytes below the top of the "struct pt_regs" - that's what we've seen on a sysenter path, and that's what used crash either. So I think using esp0 to locate the top of the "struct pt_regs" is wrong. It doesn't always point to the top of that struct. Sometimes it does, but sometimes points 8 bytes lower. IMHO the ptrace.c have to be fixed instead so to not use this wrong assumption any more. What do you think?
Btw, I attached the slightly "optimized" version of your patch (haven't tested). Just to avoid a few assignments/typecasts.
--- process.c.old 2005-05-03 20:15:39.000000000 +0400 +++ process.c 2005-05-03 20:41:25.000000000 +0400 @@ -399,12 +399,6 @@ struct task_struct *tsk; int err; - childregs = ((struct pt_regs *) (THREAD_SIZE + (unsigned long) p->thread_info)) - 1; - *childregs = *regs; - childregs->eax = 0; - childregs->esp = esp; - - p->thread.esp = (unsigned long) childregs; /* * The below -8 is to reserve 8 bytes on top of the ring0 stack. * This is necessary to guarantee that the entire "struct pt_regs" @@ -415,7 +409,13 @@ * "struct pt_regs" is possible, but they may contain the * completely wrong values. */ - p->thread.esp0 = (unsigned long) (childregs+1) - 8; + childregs = ((struct pt_regs *) (THREAD_SIZE - 8 + (unsigned long)p->thread_info)) - 1; + *childregs = *regs; + childregs->eax = 0; + childregs->esp = esp; + + p->thread.esp = (unsigned long) childregs; + p->thread.esp0 = (unsigned long) (childregs+1); p->thread.eip = (unsigned long) ret_from_fork;
| |