* [PATCH] i386/x86_64 signal handler arg fixes
@ 2006-09-14 8:34 Albert Cahalan
2006-09-14 10:11 ` Andi Kleen
0 siblings, 1 reply; 6+ messages in thread
From: Albert Cahalan @ 2006-09-14 8:34 UTC (permalink / raw)
To: linux-kernel, Andrew Morton, Andi Kleen, Linus Torvalds, hpa
[-- Attachment #1: Type: text/plain, Size: 4294 bytes --]
Some time ago, the i386 kernel was patched to support user code that
has signal handlers marked with __attribute__((regparm(3))) or compiled
with the -mregparm=3 gcc option. This is useful for klibc at least.
The code wasn't quite right, and it never got ported to x86_64.
For i386, the non-RT frame is wrong. It was using the raw sig value
instead of the translated value, and did not pass the semi-documented
extra parameters.
For x86-64, the regparm 3 calling convention was simply missing.
This patch should do the job, provided I'm right in guessing that a
struct passed by value is really passed as a pointer in this case.
(the non-RT signal handler's second arg is a pass-by-value struct)
Signed-off-by: Albert Cahalan <acahalan@gmail.com>
---
I'm sending this inline for review, and attached because
the whitespace will surely be mangled.
diff -Naurd old/arch/i386/kernel/signal.c new/arch/i386/kernel/signal.c
--- old/arch/i386/kernel/signal.c 2006-09-14 03:49:30.000000000 -0400
+++ new/arch/i386/kernel/signal.c 2006-09-14 03:52:54.000000000 -0400
@@ -375,9 +375,9 @@
/* Set up registers for signal handler */
regs->esp = (unsigned long) frame;
regs->eip = (unsigned long) ka->sa.sa_handler;
- regs->eax = (unsigned long) sig;
- regs->edx = (unsigned long) 0;
- regs->ecx = (unsigned long) 0;
+ regs->eax = (unsigned long) usig;
+ regs->edx = (unsigned long) &frame->sc;
+ regs->ecx = (unsigned long) &frame->fpstate;
set_fs(USER_DS);
regs->xds = __USER_DS;
diff -Naurd old/arch/x86_64/ia32/ia32_signal.c
new/arch/x86_64/ia32/ia32_signal.c
--- old/arch/x86_64/ia32/ia32_signal.c 2006-09-14 03:21:21.000000000 -0400
+++ new/arch/x86_64/ia32/ia32_signal.c 2006-09-14 03:47:32.000000000 -0400
@@ -433,6 +433,7 @@
{
struct sigframe __user *frame;
int err = 0;
+ int usig;
frame = get_sigframe(ka, regs, sizeof(*frame));
@@ -441,12 +442,10 @@
{
struct exec_domain *ed = current_thread_info()->exec_domain;
- err |= __put_user((ed
- && ed->signal_invmap
- && sig < 32
- ? ed->signal_invmap[sig]
- : sig),
- &frame->sig);
+ usig = ed && ed->signal_invmap && sig < 32
+ ? ed->signal_invmap[sig]
+ : sig;
+ err |= __put_user(usig,&frame->sig);
}
if (err)
goto give_sigsegv;
@@ -493,6 +492,9 @@
/* Set up registers for signal handler */
regs->rsp = (unsigned long) frame;
regs->rip = (unsigned long) ka->sa.sa_handler;
+ regs->rax = (unsigned long) usig;
+ regs->rdx = (unsigned long) &frame->sc;
+ regs->rcx = (unsigned long) &frame->fpstate;
asm volatile("movl %0,%%ds" :: "r" (__USER32_DS));
asm volatile("movl %0,%%es" :: "r" (__USER32_DS));
@@ -522,6 +524,7 @@
{
struct rt_sigframe __user *frame;
int err = 0;
+ int usig;
frame = get_sigframe(ka, regs, sizeof(*frame));
@@ -530,12 +533,10 @@
{
struct exec_domain *ed = current_thread_info()->exec_domain;
- err |= __put_user((ed
- && ed->signal_invmap
- && sig < 32
- ? ed->signal_invmap[sig]
- : sig),
- &frame->sig);
+ usig = ed && ed->signal_invmap && sig < 32
+ ? ed->signal_invmap[sig]
+ : sig;
+ err |= __put_user(usig,&frame->sig);
}
err |= __put_user(ptr_to_compat(&frame->info), &frame->pinfo);
err |= __put_user(ptr_to_compat(&frame->uc), &frame->puc);
@@ -589,6 +590,9 @@
/* Set up registers for signal handler */
regs->rsp = (unsigned long) frame;
regs->rip = (unsigned long) ka->sa.sa_handler;
+ regs->rax = (unsigned long) usig;
+ regs->rdx = (unsigned long) &frame->info;
+ regs->rcx = (unsigned long) &frame->uc;
asm volatile("movl %0,%%ds" :: "r" (__USER32_DS));
asm volatile("movl %0,%%es" :: "r" (__USER32_DS));
[-- Attachment #2: regparm3.patch --]
[-- Type: text/x-patch, Size: 2838 bytes --]
diff -Naurd old/arch/i386/kernel/signal.c new/arch/i386/kernel/signal.c
--- old/arch/i386/kernel/signal.c 2006-09-14 03:49:30.000000000 -0400
+++ new/arch/i386/kernel/signal.c 2006-09-14 03:52:54.000000000 -0400
@@ -375,9 +375,9 @@
/* Set up registers for signal handler */
regs->esp = (unsigned long) frame;
regs->eip = (unsigned long) ka->sa.sa_handler;
- regs->eax = (unsigned long) sig;
- regs->edx = (unsigned long) 0;
- regs->ecx = (unsigned long) 0;
+ regs->eax = (unsigned long) usig;
+ regs->edx = (unsigned long) &frame->sc;
+ regs->ecx = (unsigned long) &frame->fpstate;
set_fs(USER_DS);
regs->xds = __USER_DS;
diff -Naurd old/arch/x86_64/ia32/ia32_signal.c new/arch/x86_64/ia32/ia32_signal.c
--- old/arch/x86_64/ia32/ia32_signal.c 2006-09-14 03:21:21.000000000 -0400
+++ new/arch/x86_64/ia32/ia32_signal.c 2006-09-14 03:47:32.000000000 -0400
@@ -433,6 +433,7 @@
{
struct sigframe __user *frame;
int err = 0;
+ int usig;
frame = get_sigframe(ka, regs, sizeof(*frame));
@@ -441,12 +442,10 @@
{
struct exec_domain *ed = current_thread_info()->exec_domain;
- err |= __put_user((ed
- && ed->signal_invmap
- && sig < 32
- ? ed->signal_invmap[sig]
- : sig),
- &frame->sig);
+ usig = ed && ed->signal_invmap && sig < 32
+ ? ed->signal_invmap[sig]
+ : sig;
+ err |= __put_user(usig,&frame->sig);
}
if (err)
goto give_sigsegv;
@@ -493,6 +492,9 @@
/* Set up registers for signal handler */
regs->rsp = (unsigned long) frame;
regs->rip = (unsigned long) ka->sa.sa_handler;
+ regs->rax = (unsigned long) usig;
+ regs->rdx = (unsigned long) &frame->sc;
+ regs->rcx = (unsigned long) &frame->fpstate;
asm volatile("movl %0,%%ds" :: "r" (__USER32_DS));
asm volatile("movl %0,%%es" :: "r" (__USER32_DS));
@@ -522,6 +524,7 @@
{
struct rt_sigframe __user *frame;
int err = 0;
+ int usig;
frame = get_sigframe(ka, regs, sizeof(*frame));
@@ -530,12 +533,10 @@
{
struct exec_domain *ed = current_thread_info()->exec_domain;
- err |= __put_user((ed
- && ed->signal_invmap
- && sig < 32
- ? ed->signal_invmap[sig]
- : sig),
- &frame->sig);
+ usig = ed && ed->signal_invmap && sig < 32
+ ? ed->signal_invmap[sig]
+ : sig;
+ err |= __put_user(usig,&frame->sig);
}
err |= __put_user(ptr_to_compat(&frame->info), &frame->pinfo);
err |= __put_user(ptr_to_compat(&frame->uc), &frame->puc);
@@ -589,6 +590,9 @@
/* Set up registers for signal handler */
regs->rsp = (unsigned long) frame;
regs->rip = (unsigned long) ka->sa.sa_handler;
+ regs->rax = (unsigned long) usig;
+ regs->rdx = (unsigned long) &frame->info;
+ regs->rcx = (unsigned long) &frame->uc;
asm volatile("movl %0,%%ds" :: "r" (__USER32_DS));
asm volatile("movl %0,%%es" :: "r" (__USER32_DS));
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] i386/x86_64 signal handler arg fixes
2006-09-14 8:34 [PATCH] i386/x86_64 signal handler arg fixes Albert Cahalan
@ 2006-09-14 10:11 ` Andi Kleen
2006-09-14 15:01 ` Albert Cahalan
0 siblings, 1 reply; 6+ messages in thread
From: Andi Kleen @ 2006-09-14 10:11 UTC (permalink / raw)
To: Albert Cahalan; +Cc: linux-kernel, Andrew Morton, Linus Torvalds, hpa
On Thursday 14 September 2006 10:34, Albert Cahalan wrote:
> For i386, the non-RT frame is wrong. It was using the raw sig value
> instead of the translated value, and did not pass the semi-documented
> extra parameters.
The translation is not needed because x86-64 doesn't support iBCS at all
and afaik it was only used for that.
>
> For x86-64, the regparm 3 calling convention was simply missing.
Ok that should be added.
Can you please send a single patch that just does this?
-Andi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i386/x86_64 signal handler arg fixes
2006-09-14 10:11 ` Andi Kleen
@ 2006-09-14 15:01 ` Albert Cahalan
2006-09-14 13:40 ` Andi Kleen
0 siblings, 1 reply; 6+ messages in thread
From: Albert Cahalan @ 2006-09-14 15:01 UTC (permalink / raw)
To: Andi Kleen; +Cc: linux-kernel, Andrew Morton, Linus Torvalds, hpa
On 9/14/06, Andi Kleen <ak@suse.de> wrote:
> On Thursday 14 September 2006 10:34, Albert Cahalan wrote:
>
> > For i386, the non-RT frame is wrong. It was using the raw sig value
> > instead of the translated value, and did not pass the semi-documented
> > extra parameters.
>
> The translation is not needed because x86-64 doesn't support iBCS at all
> and afaik it was only used for that.
I figured, but you already have part of the code it seems.
(messing around with current_thread_info()->exec_domain
to get ->signal_invmap)
I guess that should be deleted then?
Currently you remap signals. Whatever you do this for
regparm(0) should also be done for regparm(3).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i386/x86_64 signal handler arg fixes
2006-09-14 15:01 ` Albert Cahalan
@ 2006-09-14 13:40 ` Andi Kleen
2006-09-14 16:01 ` Albert Cahalan
0 siblings, 1 reply; 6+ messages in thread
From: Andi Kleen @ 2006-09-14 13:40 UTC (permalink / raw)
To: Albert Cahalan; +Cc: linux-kernel, Andrew Morton, Linus Torvalds, hpa
> I guess that should be deleted then?
Yes. I will delete it right now. Thanks for the notice.
> Currently you remap signals. Whatever you do this for
> regparm(0) should also be done for regparm(3).
Not sure I parse you here. You're asking how to fix the regparm(3)
case?
-Andi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i386/x86_64 signal handler arg fixes
2006-09-14 13:40 ` Andi Kleen
@ 2006-09-14 16:01 ` Albert Cahalan
2006-09-14 16:23 ` H. Peter Anvin
0 siblings, 1 reply; 6+ messages in thread
From: Albert Cahalan @ 2006-09-14 16:01 UTC (permalink / raw)
To: Andi Kleen; +Cc: linux-kernel, Andrew Morton, Linus Torvalds, hpa
On 9/14/06, Andi Kleen <ak@suse.de> wrote:
>
> > I guess that should be deleted then?
>
> Yes. I will delete it right now. Thanks for the notice.
Er, OK. This means I can't patch it without conflict.
Mind just adding the six lines of code needed for
support of regparm(3) apps?
> > Currently you remap signals. Whatever you do this for
> > regparm(0) should also be done for regparm(3).
>
> Not sure I parse you here. You're asking how to fix the regparm(3)
> case?
No. I'd thought that the two cases should match.
The regparm(3) case should remap signals if and only if
the regparm(0) case remaps signals. Perhaps this
is not correct if the remapping is not needed for
native Linux apps; I doubt iBCS stuff would ever be
needing regparm(3) support.
Since you plan to delete the remapping cruft from
the regparm(0) case, then obviously it should not
be added to the regparm(3) case.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i386/x86_64 signal handler arg fixes
2006-09-14 16:01 ` Albert Cahalan
@ 2006-09-14 16:23 ` H. Peter Anvin
0 siblings, 0 replies; 6+ messages in thread
From: H. Peter Anvin @ 2006-09-14 16:23 UTC (permalink / raw)
To: Albert Cahalan; +Cc: Andi Kleen, linux-kernel, Andrew Morton, Linus Torvalds
Albert Cahalan wrote:
>
>> > Currently you remap signals. Whatever you do this for
>> > regparm(0) should also be done for regparm(3).
>>
>> Not sure I parse you here. You're asking how to fix the regparm(3)
>> case?
>
> No. I'd thought that the two cases should match.
> The regparm(3) case should remap signals if and only if
> the regparm(0) case remaps signals. Perhaps this
> is not correct if the remapping is not needed for
> native Linux apps; I doubt iBCS stuff would ever be
> needing regparm(3) support.
>
The two should definitely match, though. Otherwise, life will be confusing.
> Since you plan to delete the remapping cruft from
> the regparm(0) case, then obviously it should not
> be added to the regparm(3) case.
Indeed.
-hpa
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-09-14 16:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-09-14 8:34 [PATCH] i386/x86_64 signal handler arg fixes Albert Cahalan
2006-09-14 10:11 ` Andi Kleen
2006-09-14 15:01 ` Albert Cahalan
2006-09-14 13:40 ` Andi Kleen
2006-09-14 16:01 ` Albert Cahalan
2006-09-14 16:23 ` H. Peter Anvin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox