Re: [PATCH] i386: fix stack alignment for signal handlers

["Markus F.X.J. Oberhumer" <markus@oberhumer.com>]

Andi Kleen wrote:
> On Wed, Sep 14, 2005 at 07:55:53AM -0700, Linus Torvalds wrote:
> > On Wed, 14 Sep 2005, Daniel Jacobowitz wrote:
> >
> > > The comment for the relevant bits of the GCC configuration says it won't
> > > assume this for x86, but I believe that comment is out of date. I think
> > > it'll assume 16-byte alignment on entrance to non-main() functions.
> >
> > Well, that's kind of the point. We _do_ have the stack aligned on
> > entrance, but it looks like gcc wants it _non-aligned_. It seems to want
> > it offset by the "return address push" - ie it seems to expect that it was
> > aligned before the "call", but entry into the next function will thus
> > _never_ be aligned.
> >
> > So the kernel actually seems to have it _too_ aligned right now. 
>
>
> Yes it's wrong. I would recommend to apply Markus' patch for i386
> and x86-64.
>
> -Andi

Here is a somewhat simplified version of my previous patch with
updated comments.

Attached is also a new small user-space test program which does not
depend on any special gcc features and should trigger the problem on all 
machines.

~Markus

P.S. I have not been involved in lkml back since 1999, so I currently don't 
know whom to bug to get this patch applied, esp. as there seems to be no 
official i386 maintainer.

--
Markus Oberhumer, <markus@oberhumer.com>, http://www.oberhumer.com/

[PATCH] i386: fix stack alignment for signal handlers

This patches fixes the setup of the alignment of the signal frame, so
that all signal handlers are run with a properly aligned stack frame.

The current code "over-aligns" the stack pointer so that the stack
frame is effectively always mis-aligned by 4 bytes. But what we really
want is that on function entry ((sp + 4) & 15) == 0.


Signed-off-by: Markus F.X.J. Oberhumer <markus@oberhumer.com>


 arch/i386/kernel/signal.c      |    6 +++++-
 arch/x86_64/ia32/ia32_signal.c |    6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

Index: linux-2.6.git/arch/i386/kernel/signal.c
===================================================================
--- linux-2.6.git.orig/arch/i386/kernel/signal.c
+++ linux-2.6.git/arch/i386/kernel/signal.c
@@ -338,7 +338,11 @@
 		esp = (unsigned long) ka->sa.sa_restorer;
 	}
 
-	return (void __user *)((esp - frame_size) & -8ul);
+	esp -= frame_size;
+	/* Align the stack pointer according to the i386 ABI,
+	 * i.e. so that on function entry ((sp + 4) & 15) == 0. */
+	esp = ((esp + 4) & -16ul) - 4;
+	return (void __user *) esp;
 }
 
 /* These symbols are defined with the addresses in the vsyscall page.
Index: linux-2.6.git/arch/x86_64/ia32/ia32_signal.c
===================================================================
--- linux-2.6.git.orig/arch/x86_64/ia32/ia32_signal.c
+++ linux-2.6.git/arch/x86_64/ia32/ia32_signal.c
@@ -425,7 +425,11 @@
 		rsp = (unsigned long) ka->sa.sa_restorer;
 	}
 
-	return (void __user *)((rsp - frame_size) & -8UL);
+	rsp -= frame_size;
+	/* Align the stack pointer according to the i386 ABI,
+	 * i.e. so that on function entry ((sp + 4) & 15) == 0. */
+	rsp = ((rsp + 4) & -16ul) - 4;
+	return (void __user *) rsp;
 }
 
 int ia32_setup_frame(int sig, struct k_sigaction *ka,

/* test signal stack alignment (sigframe)
 *
 * a small user-space demo program to show that the signal stack
 * is currently mis-aligned on i386-linux
 *
 * Markus F.X.J. Oberhumer <markus@oberhumer.com>
 */

#include <signal.h>
#include <stdio.h>
#include <stdlib.h>

void sighandler(int);
void test(void);

volatile unsigned long sp = 0;

/* assembler prologue code that stores the stack pointer into 'sp'
 * and then jumps to the real function */
#if defined(__i386__)
asm(
    ".text\n"
"sighandler:\n"
    "lea    4(%esp), %eax\n"
    "mov    %eax, (sp)\n"
    "jmp    do_sighandler\n"
"test:\n"
    "lea    4(%esp), %eax\n"
    "mov    %eax, (sp)\n"
    "jmp    do_test\n"
".globl main\n"
"main:\n"
    "lea    4(%esp), %eax\n"
    "mov    %eax, (sp)\n"
    "jmp    do_main\n"
);
#elif defined(__x86_64__)
asm(
    ".text\n"
"sighandler:\n"
    "lea    8(%rsp), %rax\n"
    "mov    %rax, sp(%rip)\n"
    "jmp    do_sighandler\n"
"test:\n"
    "lea    8(%rsp), %rax\n"
    "mov    %rax, sp(%rip)\n"
    "jmp    do_test\n"
".globl main\n"
"main:\n"
    "lea    8(%rsp), %rax\n"
    "mov    %rax, sp(%rip)\n"
    "jmp    do_main\n"
);
#else
#error "arch not supported - please insert your code here"
#endif


void do_sighandler(void)
{
    printf("in sighandler: sp = 0x%lx\n", sp);
}

void do_test(void)
{
    printf("in test      : sp = 0x%lx\n", sp);
    signal(SIGUSR1, sighandler);
    raise(SIGUSR1);
}

int do_main(void)
{
    printf("in main      : sp = 0x%lx\n", sp);
    test();
    printf("All done.\n");
    return 0;
}


/* vim:set ts=4 et: */