Re: Register saving and signal handling

[linux-os]

On Tue, 25 Jan 2005, Robert Szeleney wrote:

> Hi!
>
> This is my first time posting to the Linux kernel mailing-list, and I hope 
> someone can help me or at least explain following to me.
>
> When a task gets interrupted by a signal, the do_signal() function is called. 
> Now, when the signal is not handled by the task and the interrupted function 
> returned -EINTR, the syscall gets restarted by modifying the user mode EIP to 
> point to the int 0x80 again.
>
> When the task leaves the do_signal function, it will pop the saved registers 
> and return to the user mode and immediately do the syscall again.
>
> But now to the actual question:
>
> Let's make a new "test" system call function. Let's call it: sys_test
>
> asmlinkage int sys_test(int para1, int para2)
> {
>   para1++;
>   para2++;
>
>   kill( current->pid, SIGCHLD);
>   return -EINTR;
> }
>
> When compiling this function, GCC increments para1 and para2 by one. para1 
> and para2 are stored on the kernel stack. The system call entry assembler 
> function pushed the registers containing this values from the usermode on the 
> stack.
> But GCC actually modified the values on the stack here, the "live" data. 
> (which will be poped later again, right before returning to user mode)
>
> After returning from this function, the system call wrapper checks for a 
> signal and calls do_signal. The do_signal call will restart the system call 
> by modifying the user mode EIP. Then, the system call wrapper will pop the 
> saved registers from the stack. But here I see this problem. The already 
> modified values for para1 and para2 will be popped. When the system call is 
> then start again, para1 and para2 don't have to original value.
>
> One can say, why are you modifying para1 and para2 then?  Yes, this is 
> correct, but after compiling a few more test sources, I got following 
> problem:
>
> asmlinkage int sys_test(int iSize)
> {
>    printk("Size is: %d\n", iSize * sizeof(any_structure));
> }
>
> When compiling this, GCC produces following assembler:
>
> ....
> sall    $4, 8(%ebp)
> ....

But this is correct. The caller should pass a COPY of the parameter.
The called procedure can do anything it wants to this copy. Check
to see what asmlinkage is #defined to be ou your system. It should
be __attribute__((regparam(0))). If it got changed, all bets are
off with any interface code. Everything needs to match.

> which actually modifies the content of the stack holding the iSize again. It 
> is very difficult to keep track of such implicit stack argument 
> modifications.
> Thus, when a signal is waiting and the syscall is restarted, iSize contains a 
> different value already.
>
> So, does anyone else have such a problem? Is there any compiler flag missing? 
> Is this possible at all?
>
> Thank you very much!
> Robert!
>
> Btw,  please CC to mf204005@xxxxxxxxx  too.

Yes.

For instance:

void funct(int param)
{
    param++;
}
int main()
{
    int foo = 0;
    printf("%d\n", foo);
}

That code should pass a COPY of foo to funct(). Procedure
funct() should be able to do anything it wants with it
and, since it's a copy, main() should still print 0.
However, there is some kernel code that passes the
actual value, not a copy.

linux-2.6.9/arch/i386/kernel/semaphore.c is an example.
This has some hand-tweaked assembly that violates the
de-facto 'C' standard. There may be other such kernel
code. I submitted a patch, but it was rejected. Just
for kicks, I attached the patch so you can see what
problem(s) may still exist.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.10 on an i686 machine (5537.79 BogoMips).
 Notice : All mail here is now cached for review by Dictator Bush.
                 98.36% of all statistics are fiction.--- linux-2.6.9/arch/i386/kernel/semaphore.c.orig	2004-10-29 13:00:17.961579368 -0400
+++ linux-2.6.9/arch/i386/kernel/semaphore.c	2004-10-29 13:03:35.046617888 -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"
+	"leal 0x04(%esp), %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"
+	"leal 0x04(%esp), %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"
+	"leal 0x04(%esp), %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"
+	"leal 0x04(%esp), %esp\n\t"	// Bypass corrupted parameter
+	"popl %ecx\n\t"			// Restore register
 	"popl %edx\n\t"
 	"popl %eax\n\t"
 	"ret"