RE: [BUG] in i386 semaphores.

[Richard B. Johnson]

On Mon, 18 Oct 2004, Aleksey Gorelov wrote:

> > -----Original Message-----
> > From: Richard B. Johnson [mailto:root@xxxxxxxxxxxxxxxxxx]
> > Sent: Monday, October 18, 2004 12:49 PM
> > __up is declared 'asmlinkage', which means that conventional
> > 'C' calling convention is used, that eax and/or edx/eax pair
> > is used for return values and the input values are pushed on
> >  the stack. The last parameter passed is a pointer in %ecx.
>
> Can not disagree here, except that %ecx is the only parameter.
> eax/edx are just saved in the stack, and then restored (see comments
> before the assembly in semaphore.c).
> > Therefore, the called 'C' function, that 'knows' only about
> > the first parameter passed, will get the correct pointer value.
> > The 'C' function also never changes the value of that pointer,
> > only something that it points to.
>
> This is an assumption. AFAIK, according to C standard, formal parameters
> are
> COPIED from actual parameters. Formal parameters can not be used outside
> the function, and as far as they are not constant ones, C compiler
> is free to mofidy them after their last use, and use the memory for
> other purposes.
> >

So you are saying that the stack area that was used by the passed-
parameter (that was never modified) can be destroyed at the whim of
the compiler?

void funct(int param)
{
   printf("%d\n", param);
   compiler_destroy(param);  // Not coded, just done?
}


Some C-compiler expert has to answer that question. If the
answer is true, then you need this patch:


--- linux-2.6.8/arch/i386/kernel/semaphore.c.orig	2004-08-14 01:36:56.000000000 -0400
+++ linux-2.6.8/arch/i386/kernel/semaphore.c	2004-10-19 08:06:15.000000000 -0400
@@ -198,9 +198,11 @@
 #endif
 	"pushl %eax\n\t"
 	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"	// Register to save
+	"pushl %ecx\n\t"	// Passed parameter
 	"call __down\n\t"
-	"popl %ecx\n\t"
+	"addl $0x04, %esp\t\n"	// Bypass corrupted parameter
+	"popl %ecx\n\t"		// Restore original
 	"popl %edx\n\t"
 	"popl %eax\n\t"
 #if defined(CONFIG_FRAME_POINTER)
@@ -220,9 +222,11 @@
 	"movl  %esp,%ebp\n\t"
 #endif
 	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"	// Save register
+	"pushl %ecx\n\t"	// Passed parameter
 	"call __down_interruptible\n\t"
-	"popl %ecx\n\t"
+	"addl $0x04, %esp\n\t"	// Bypass corrupted parameter
+	"popl %ecx\n\t"		// Restore register
 	"popl %edx\n\t"
 #if defined(CONFIG_FRAME_POINTER)
 	"movl %ebp,%esp\n\t"
@@ -241,9 +245,11 @@
 	"movl  %esp,%ebp\n\t"
 #endif
 	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"		// Save register
+	"pushl %ecx\n\t"		// Passed parameter
 	"call __down_trylock\n\t"
-	"popl %ecx\n\t"
+	"addl $0x04, %esp\n\t"		// Bypass corrupted parameter
+	"popl %ecx\n\t"			// Restore register
 	"popl %edx\n\t"
 #if defined(CONFIG_FRAME_POINTER)
 	"movl %ebp,%esp\n\t"
@@ -259,9 +265,11 @@
 "__up_wakeup:\n\t"
 	"pushl %eax\n\t"
 	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"	// Save register
+	"pushl %ecx\n\t"	// Passed parameter
 	"call __up\n\t"
-	"popl %ecx\n\t"
+	"addl $0x04, %esp\n\t"	// Bypass corrupted parameter
+	"popl %ecx\n\t"		// Restore register
 	"popl %edx\n\t"
 	"popl %eax\n\t"
 	"ret"



...and if you need this patch, then there is no reason for
the 'helper' functions at all and the code should be rewritten
to get rid of them.

> > Now, wake_up() gets a pointer to the variable sem->wait. wake_up()
> > never modifies 'sem', only sem->wait.
>
> Well, build the kernel with DEBUG_KERNEL disabled with gcc 3.2 for
> instance,
> objdump it and check __up() code. You'll be surprised. And this is NOT
> gcc issue.

I'm not sure about that. I don't think the 'C' compiler is allowed
to whack at things that were never touched by the code. For instance,
we have "main(int argc, char *argv[], char *env[]);" Normally main()
is only declared to have two arguments, but the environment strings
are really there also. Can the 'C' compiler destroy them? I think
not.

> > >  As one can see, actual parameter in %ecx is not only being copied in
> > > formal parameter sem (which is correct), but also being
> > restored from it
> > > after function call via %ecx (which is incorrect). Since formal
> > > parameter is not a constant one, it may be overwritten inside C
> > > function, or gcc may (and in fact does that in some cases) use it for
> > > something else.
> > > If we want to keep %ecx, correct behavior would be
> >
> >
> > > 	pushl %ecx
> > > 	pushl %ecx
> > > 	call _up
> > > 	add $4, %ecx
>              ^^^^
>              %esp
>
> Sorry, there was a typo as specified above. Of cause, you still need
> push/pop eax & edx, that was ommited for brevity, cause it is not really
> relevant.
>
> > > 	popl %ecx
> >
> > Very wrong. In fact, it would unbalance the stack. The register
> > value, %ecx must be restored exactly as the code does it. One
> > can't assume that %ecx magically got changed and needs to be
> > 'corrected'.
> >
> > Maybe you meant:
> >
> > 	pushl	%eax
> > 	pushl	%edx
>      pushl %ecx   # <- you still need to keep ecx
> > 	pushl	%ecx
> > 	call	__up
> > 	addl	$0x04, %esp	# Bypass ecx on stack
>      popl  %ecx   # <- this one as well
> > 	popl	%edx
> > 	popl	%eax
>
> I meant as indicated above.
>
> Aleks.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.8 on an i686 machine (5537.79 BogoMips).
            Note 96.31% of all statistics are fiction.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/