Re: 2.6.12-rc3 OOPS in vanilla source (once more)

[Stas Sergeev]

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;