2005-09-13 20:48:48

by Markus F.X.J. Oberhumer

[permalink] [raw]
Subject: [PATCH] i386: fix stack alignment for signal handlers

/* test signal stack alignment (sigframe) */
/* Markus F.X.J. Oberhumer <[email protected]> */

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

volatile unsigned long signal_sp = 0;

#if defined(__x86_64__)
/* amd64: signal stack is properly aligned */
asm(
".text\n .align 16\n"
"my_sighandler:\n"
"lea 8(%rsp),%rax\n"
"movq %rax,(signal_sp)\n"
"retq\n"
);
#elif defined(__i386__)
/* i386: signal stack is always mis-aligned to 4-bytes */
asm(
".text\n .align 16\n"
"my_sighandler:\n"
"lea 4(%esp),%eax\n"
"movl %eax,(signal_sp)\n"
"ret\n"
);
#else
#error "arch not supported - please insert your code here"
#endif


#if defined(__cplusplus)
extern "C"
#endif
void my_sighandler(int);


int main()
{
int signo = SIGUSR1;

#if 1
/* randomize stack */
void * volatile dummy = NULL;
srand((unsigned)clock() % RAND_MAX);
dummy = __builtin_alloca(rand() % 2048ul);
#endif

signal_sp = 0;
signal(signo, my_sighandler);
raise(signo);
printf("sp = 0x%lx alignment = %lu\n", signal_sp, signal_sp & 15);
if (signal_sp & 15)
printf(" error: signal stack not aligned\n");

return 0;
}


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



Attachments:
test_sigframe_alignment.c (1.19 kB)
i386-align_sigframe.patch (2.39 kB)
Download all attachments

2005-09-13 22:54:01

by Linus Torvalds

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



On Tue, 13 Sep 2005, Markus F.X.J. Oberhumer wrote:
>
> It seems that the current signal code always sets up a stack frame so that
> signal handlers are run with a somewhat mis-aligned stack, i.e. (esp % 8 == 4).

Actually, not really.

They get entered with the stack pointer aligned, at least in my tests.

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

extern void handler(int);
void *saved_esp;

asm("handler: movl %esp,saved_esp; ret");

int main(int argc, char **argv)
{
signal(SIGALRM, handler);
alarm(1);
pause();
printf("%p\n", saved_esp);
return 0;
}

always prints out an aligned address for me.

You seem to be expecting that the address be aligned "before the return
address push", which is a totally different thing. Quite frankly, I don't
know which one gcc prefers or whether there's an ABI specifying any
preferences.

Linus

2005-09-13 23:24:08

by Markus F.X.J. Oberhumer

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

Linus Torvalds wrote:
>
> On Tue, 13 Sep 2005, Markus F.X.J. Oberhumer wrote:
>
>>It seems that the current signal code always sets up a stack frame so that
>>signal handlers are run with a somewhat mis-aligned stack, i.e. (esp % 8 == 4).
>
> Actually, not really.
>
> They get entered with the stack pointer aligned, at least in my tests.
>
> #include <stdio.h>
> #include <signal.h>
> #include <unistd.h>
>
> extern void handler(int);
> void *saved_esp;
>
> asm("handler: movl %esp,saved_esp; ret");
>
> int main(int argc, char **argv)
> {
> signal(SIGALRM, handler);
> alarm(1);
> pause();
> printf("%p\n", saved_esp);
> return 0;
> }
>
> always prints out an aligned address for me.
>
> You seem to be expecting that the address be aligned "before the return
> address push", which is a totally different thing. Quite frankly, I don't
> know which one gcc prefers or whether there's an ABI specifying any
> preferences.

I'm pretty sure that on both amd64 and i386 the alignment has to be
_before_ the address push from the call, though I cannot find any exact ABI
specs at the moment. Experts please advise.

What do you get when running this slightly modified version of your test
program? My patch would fix the alignment of Aligned16 here.

And for amd64, please also see arch/x86_64/kernel/signal.c where 8 is
subtracted from the get_stack() result. Actually I wonder if other archs
might be affected as well...

~Markus


#include <stdio.h>
#include <signal.h>
#include <unistd.h>
#include <assert.h>

typedef struct { double x,y; } Aligned16 __attribute__((__aligned__(16)));

void *saved_esp;
void handler(int unused) {
Aligned16 a;
assert(__alignof(a) >= 16),
saved_esp = (void *) &a;
}

int main()
{
Aligned16 a;
assert(__alignof(a) >= 16),
printf("%p\n", &a);
signal(SIGALRM, handler);
alarm(1);
pause();
printf("%p\n", saved_esp);
return 0;
}


--
Markus Oberhumer, <[email protected]>, http://www.oberhumer.com/

2005-09-13 23:52:42

by Linus Torvalds

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



On Wed, 14 Sep 2005, Markus F.X.J. Oberhumer wrote:
>
> > You seem to be expecting that the address be aligned "before the return
> > address push", which is a totally different thing. Quite frankly, I don't
> > know which one gcc prefers or whether there's an ABI specifying any
> > preferences.
>
> I'm pretty sure that on both amd64 and i386 the alignment has to be
> _before_ the address push from the call, though I cannot find any exact ABI
> specs at the moment. Experts please advise.
>
> What do you get when running this slightly modified version of your test
> program? My patch would fix the alignment of Aligned16 here.

Your test program does seems to imply that gcc wants the alignment before
the return address (ie it prints out an address that is 4 bytes offset),
but on the other hand I'm not even sure how careful gcc is about this
alignment thing at all.

In the "main()" function, gcc will actually generate a "andl $-16,%esp" to
force the alignment, but ot in the handler function. Just a gcc special
case? Random luck?

Andi - you know the gcc people, is there some documented rules somewhere?
How does gcc itself try to align the stack when it generates the calls?

Linus

2005-09-14 01:32:56

by Markus F.X.J. Oberhumer

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

Linus Torvalds wrote:
>
> On Wed, 14 Sep 2005, Markus F.X.J. Oberhumer wrote:
>
>>>You seem to be expecting that the address be aligned "before the return
>>>address push", which is a totally different thing. Quite frankly, I don't
>>>know which one gcc prefers or whether there's an ABI specifying any
>>>preferences.
>>
>>I'm pretty sure that on both amd64 and i386 the alignment has to be
>>_before_ the address push from the call, though I cannot find any exact ABI
>>specs at the moment. Experts please advise.
>>
>>What do you get when running this slightly modified version of your test
>>program? My patch would fix the alignment of Aligned16 here.
>
>
> Your test program does seems to imply that gcc wants the alignment before
> the return address (ie it prints out an address that is 4 bytes offset),
> but on the other hand I'm not even sure how careful gcc is about this
> alignment thing at all.
>
> In the "main()" function, gcc will actually generate a "andl $-16,%esp" to
> force the alignment, but ot in the handler function. Just a gcc special
> case? Random luck?

I think that main() is a known name and therefore gets a special treatment
- if you rename main() to foo() and then compare the disassembly you will
see that the "andl $-16,%esp" has vanished.

OTOS the "andl" in main() exactly does show how gcc wants the stack to be
aligned, i.e. _before_ the call-address push.

Another argument would be the 16-byte aligned stack-setup of glibc - please
try runing this tiny program under gdb and look at "info reg":

asm(".globl main\n main:\n int $3\n");

All of this would indicate that the kernel should get fixed.

~Markus

>
> Andi - you know the gcc people, is there some documented rules somewhere?
> How does gcc itself try to align the stack when it generates the calls?
>
> Linus
>

--
Markus Oberhumer, <[email protected]>, http://www.oberhumer.com/

2005-09-14 04:54:08

by Andi Kleen

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

> Andi - you know the gcc people, is there some documented rules somewhere?
> How does gcc itself try to align the stack when it generates the calls?

The x86-64 ABI says

>>
The end of the input argument area shall be aligned on a 16 byte boundary.
In other words, the value (%rsp - 8) is always a multiple of 16 when control is
transferred to the function entry point. The stack pointer, %rsp, always points to
the end of the latest allocated stack frame. 7
<<

I presume acts i386 (it's likely undocumented because the ABI documents
predate this), but Jan surely can confirm?

It makes sense because when long double is passed on the stack
you really want them to be aligned there.

This would mean Markus' patch is correct for x86-64 and likely for i386
too.

-Andi

2005-09-14 14:22:11

by Daniel Jacobowitz

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

On Tue, Sep 13, 2005 at 04:52:30PM -0700, Linus Torvalds wrote:
> Your test program does seems to imply that gcc wants the alignment before
> the return address (ie it prints out an address that is 4 bytes offset),
> but on the other hand I'm not even sure how careful gcc is about this
> alignment thing at all.

Very, on architectures where the ABI requires alignment. E.G. for
vector register loads that require 16-byte alignment to avoid a trap.

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.

> In the "main()" function, gcc will actually generate a "andl $-16,%esp" to
> force the alignment, but ot in the handler function. Just a gcc special
> case? Random luck?

Special case. This is only done for main().

--
Daniel Jacobowitz
CodeSourcery, LLC

2005-09-14 14:56:09

by Linus Torvalds

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



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.

Linus

2005-09-14 15:44:32

by Andi Kleen

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

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

2005-09-14 20:11:32

by J.A. Magallon

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


On 09.14, Markus F.X.J. Oberhumer wrote:
...
>
> #include <stdio.h>
> #include <signal.h>
> #include <unistd.h>
> #include <assert.h>
>
> typedef struct { double x,y; } Aligned16 __attribute__((__aligned__(16)));
>
> void *saved_esp;
> void handler(int unused) {
> Aligned16 a;
> assert(__alignof(a) >= 16),

errr... assert(__alignof(a) < 16), ???


--
J.A. Magallon <jamagallon()able!es> \ Software is like sex:
werewolf!able!es \ It's better when it's free
Mandriva Linux release 2006.0 (Cooker) for i586
Linux 2.6.13-jam4 (gcc 4.0.1 (4.0.1-5mdk for Mandriva Linux release 2006.0))


2005-10-09 16:53:22

by Markus F.X.J. Oberhumer

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

/* 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 <[email protected]>
*/

#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: */


Attachments:
i386-align_sigframe.patch (1.77 kB)
test_sigframe_alignment.c (1.52 kB)
Download all attachments

2005-10-09 16:56:50

by Andi Kleen

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

On Sunday 09 October 2005 18:54, Markus F.X.J. Oberhumer wrote:

> 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.

I already have a version of the patch in my queue, but it's not a strict
bugfix so it's only for after 2.6.14.

-Andi

ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt-current/patches/sigframe-alignment


2005-10-09 17:05:40

by Markus F.X.J. Oberhumer

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

Andi Kleen wrote:
> On Sunday 09 October 2005 18:54, Markus F.X.J. Oberhumer wrote:
>
>
>>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.
>
>
> I already have a version of the patch in my queue, but it's not a strict
> bugfix so it's only for after 2.6.14.

I see, many thanks.

Please note that your current version could waste 16-bytes for unneeded
alignment, while my new version does not. Not a real problem, but still
things like these should be done right.

~Markus

>
> -Andi
>
> ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt-current/patches/sigframe-alignment
>
>

--
Markus Oberhumer, <[email protected]>, http://www.oberhumer.com/

2005-10-11 00:22:22

by Markus F.X.J. Oberhumer

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

I've just seen that Linus has merged my second patch, so here is one more
missing piece to fix ia64 in ia32 emulation as well.

~Markus

p.s. this patch has not been tested due to lack of hardware


Andi Kleen wrote:
> On Sunday 09 October 2005 18:54, Markus F.X.J. Oberhumer wrote:
>
>
>>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.
>
>
> I already have a version of the patch in my queue, but it's not a strict
> bugfix so it's only for after 2.6.14.
>
> -Andi
>
> ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt-current/patches/sigframe-alignment
>
>

--
Markus Oberhumer, <[email protected]>, http://www.oberhumer.com/


Attachments:
i386-align_sigframe-ia64.patch (1.19 kB)