We need to check for stack overflow only when the signal is on stack.
So we can improve the patch "http://lkml.org/lkml/2007/11/27/101" as following.
Signed-off-by: Shi Weihua <[email protected]>
---
--- linux-2.6.25-rc2.orig/arch/x86/kernel/signal_32.c 2008-02-16 04:57:20.000000000 +0800
+++ linux-2.6.25-rc2/arch/x86/kernel/signal_32.c 2008-02-18 18:06:39.000000000 +0800
@@ -299,17 +299,21 @@ get_sigframe(struct k_sigaction *ka, str
/* Default to using normal stack */
sp = regs->sp;
- /*
- * If we are on the alternate signal stack and would overflow it, don't.
- * Return an always-bogus address instead so we will die with SIGSEGV.
- */
- if (on_sig_stack(sp) && !likely(on_sig_stack(sp - frame_size)))
- return (void __user *) -1L;
-
/* This is the X/Open sanctioned signal stack switching. */
if (ka->sa.sa_flags & SA_ONSTACK) {
- if (sas_ss_flags(sp) == 0)
+ int onstack = sas_ss_flags(sp);
+
+ if (onstack == 0)
sp = current->sas_ss_sp + current->sas_ss_size;
+ else if (onstack == SS_ONSTACK) {
+ /*
+ * If we are on the alternate signal stack and would
+ * overflow it, don't return an always-bogus address
+ * instead so we will die with SIGSEGV.
+ */
+ if (!likely(on_sig_stack(sp - frame_size)))
+ return (void __user *) -1L;
+ }
}
/* This is the legacy signal stack switching. */
* Shi Weihua <[email protected]> wrote:
> We need to check for stack overflow only when the signal is on stack.
> So we can improve the patch "http://lkml.org/lkml/2007/11/27/101" as
> following.
hm, does this address Roland's observations at:
http://lkml.org/lkml/2007/11/28/13
?
Ingo
On Mon, 18 Feb 2008 18:22:05 +0800, Shi Weihua said:
> - /*
> - * If we are on the alternate signal stack and would overflow it, don't.
notice this ^
> - * Return an always-bogus address instead so we will die with SIGSEGV.
> + * If we are on the alternate signal stack and would
> + * overflow it, don't return an always-bogus address
missing here ^
> + * instead so we will die with SIGSEGV.
"don't. Return" is a vastly different semantic than "don't return".
I think the same comment error was cut-n-pasted into all 5 patches...
Ingo Molnar wrote::
> * Shi Weihua <[email protected]> wrote:
>
>> We need to check for stack overflow only when the signal is on stack.
>> So we can improve the patch "http://lkml.org/lkml/2007/11/27/101" as
>> following.
>
> hm, does this address Roland's observations at:
>
> http://lkml.org/lkml/2007/11/28/13
>
> ?
Yes. please check it.
>
> Ingo
>
>
>
[email protected] wrote::
> On Mon, 18 Feb 2008 18:22:05 +0800, Shi Weihua said:
>
>> - /*
>> - * If we are on the alternate signal stack and would overflow it, don't.
> notice this ^
>> - * Return an always-bogus address instead so we will die with SIGSEGV.
>
>> + * If we are on the alternate signal stack and would
>> + * overflow it, don't return an always-bogus address
> missing here ^
>> + * instead so we will die with SIGSEGV.
>
> "don't. Return" is a vastly different semantic than "don't return".
>
> I think the same comment error was cut-n-pasted into all 5 patches...
>
Sorry, it's my mistake.
I will correct the all 5 patches. Thanks.
We need to check for stack overflow only when the signal is on stack.
So we can improve the patch "http://lkml.org/lkml/2007/11/27/101" as following.
Signed-off-by: Shi Weihua <[email protected]>
---
The previous patch has a comment mistake. Now I correct it.
---
--- linux-2.6.25-rc2.orig/arch/x86/kernel/signal_32.c 2008-02-16 04:57:20.000000000 +0800
+++ linux-2.6.25-rc2/arch/x86/kernel/signal_32.c 2008-02-19 09:55:59.000000000 +0800
@@ -299,17 +299,21 @@ get_sigframe(struct k_sigaction *ka, str
/* Default to using normal stack */
sp = regs->sp;
- /*
- * If we are on the alternate signal stack and would overflow it, don't.
- * Return an always-bogus address instead so we will die with SIGSEGV.
- */
- if (on_sig_stack(sp) && !likely(on_sig_stack(sp - frame_size)))
- return (void __user *) -1L;
-
/* This is the X/Open sanctioned signal stack switching. */
if (ka->sa.sa_flags & SA_ONSTACK) {
- if (sas_ss_flags(sp) == 0)
+ int onstack = sas_ss_flags(sp);
+
+ if (onstack == 0)
sp = current->sas_ss_sp + current->sas_ss_size;
+ else if (onstack == SS_ONSTACK) {
+ /*
+ * If we are on the alternate signal stack and would
+ * overflow it, don't. Return an always-bogus address
+ * instead so we will die with SIGSEGV.
+ */
+ if (!likely(on_sig_stack(sp - frame_size)))
+ return (void __user *) -1L;
+ }
}
/* This is the legacy signal stack switching. */
* Shi Weihua <[email protected]> wrote:
> We need to check for stack overflow only when the signal is on stack.
> So we can improve the patch "http://lkml.org/lkml/2007/11/27/101" as
> following.
thanks, applied.
Ingo
This change looks bogus to me. Before I get to the content, there is a nit
that annoys me. You changed the punctuation in my comment so that it no
longer means what it did, and now the comment is nonsensical. I don't
demand decent English from hackers of any linguistic bent, but please don't
louse up the coherent sentences I wrote when moving them down ten lines.
Please elaborate on the rationale that justifies this change.
I don't see it at all.
If you are already on the signal stack, it doesn't matter whether the
signal that just arrived has SA_ONSTACK set or not. If you are going to
overflow the stack with the new signal frame, we want to prevent that
clobberation regardless.
Thanks,
Roland
Ingo Molnar writes:
>
> * Shi Weihua <[email protected]> wrote:
>
> > We need to check for stack overflow only when the signal is on stack.
> > So we can improve the patch "http://lkml.org/lkml/2007/11/27/101" as
> > following.
>
> thanks, applied.
These patches change the behaviour of programs that longjmp out of a
signal handler on an alternate stack, don't they?
I'm interested to know what gave you confidence that changing that
behaviour won't break existing working programs.
Paul.
Paul Mackerras wrote:
> Ingo Molnar writes:
>> * Shi Weihua <[email protected]> wrote:
>>
>>> We need to check for stack overflow only when the signal is on stack.
>>> So we can improve the patch "http://lkml.org/lkml/2007/11/27/101" as
>>> following.
>> thanks, applied.
>
> These patches change the behaviour of programs that longjmp out of a
> signal handler on an alternate stack, don't they?
>
> I'm interested to know what gave you confidence that changing that
> behaviour won't break existing working programs.
>
Shouldn't such programs use sigsetjmp/siglongjmp, which should reset the
signal stack state?
-hpa
> Shouldn't such programs use sigsetjmp/siglongjmp, which should reset the
> signal stack state?
That is not really related. The distinction doesn't really exist for
programs using the normal API (setjmp is sigsetjmp(,1)). What siglongjmp
guarantees handled is signal mask changes, not sigaltstack.
Thanks,
Roland
> These patches change the behaviour of programs that longjmp out of a
> signal handler on an alternate stack, don't they?
No, they don't. Shi Weihua's original patch had that problem.
Thanks,
Roland
Roland McGrath wrote::
> This change looks bogus to me. Before I get to the content, there is a nit
> that annoys me. You changed the punctuation in my comment so that it no
> longer means what it did, and now the comment is nonsensical. I don't
> demand decent English from hackers of any linguistic bent, but please don't
> louse up the coherent sentences I wrote when moving them down ten lines.
>
> Please elaborate on the rationale that justifies this change.
> I don't see it at all.
I have corrected the comment in the latest patch which has been apllied by Ingo.
Please refer to http://lkml.org/lkml/2008/2/18/575 and http://lkml.org/lkml/2008/2/19/119 .
Thanks.
Shi Weihua
>
> If you are already on the signal stack, it doesn't matter whether the
> signal that just arrived has SA_ONSTACK set or not. If you are going to
> overflow the stack with the new signal frame, we want to prevent that
> clobberation regardless.
>
>
> Thanks,
> Roland
>
>
>
> > Please elaborate on the rationale that justifies this change.
> > I don't see it at all.
>
> I have corrected the comment in the latest patch which has been apllied by Ingo.
> Please refer to http://lkml.org/lkml/2008/2/18/575 and http://lkml.org/lkml/2008/2/19/119 .
You have yet to say anything to justify your change.
Thanks,
Roland
Roland McGrath wrote::
>>> Please elaborate on the rationale that justifies this change.
>>> I don't see it at all.
>> I have corrected the comment in the latest patch which has been apllied by Ingo.
>> Please refer to http://lkml.org/lkml/2008/2/18/575 and http://lkml.org/lkml/2008/2/19/119 .
>
> You have yet to say anything to justify your change.
You mean the comment?
I mean I made a mistake about the comment so I changed it, and then
Valdis.Kletnieks pointed it out for me. And late I sent a revised
patch with your original comment untouched.
Sorry for my poor English. :(
Thanks
Shi Weihua
> You mean the comment?
No, that is trivial and already corrected. I mean the substance of your
most recent patch. I described why I think it is wrong. You did not respond.
Roland McGrath wrote::
>> You mean the comment?
>
> No, that is trivial and already corrected. I mean the substance of your
> most recent patch. I described why I think it is wrong. You did not respond.
I spent some time read you mail carefully and dig into the code again.
And yes, you are right. It's possible that SA_ONSTACK has been cleared
before the second signal on the same stack comes.
So this patch is wrong :( . I will revise the other 4 patches.
Sorry for the noise.
> I spent some time read you mail carefully and dig into the code again.
>
> And yes, you are right. It's possible that SA_ONSTACK has been cleared
> before the second signal on the same stack comes.
It's not necessary for SA_ONSTACK to have "been cleared", by which I assume
you mean a sigaction call with SA_ONSTACK not set in sa_flags. That is
indeed possible, but it's not the only case your patch broke. It can just
be a different signal whose sigaction never had SA_ONSTACK, when you are
still on the signal stack from an earlier signal that did have SA_ONSTACK.
> So this patch is wrong :( . I will revise the other 4 patches.
For 2 and 3, I would rather just wait until we unify signal.c anyway.
Thanks,
Roland
Roland McGrath wrote::
>> I spent some time read you mail carefully and dig into the code again.
>>
>> And yes, you are right. It's possible that SA_ONSTACK has been cleared
>> before the second signal on the same stack comes.
>
> It's not necessary for SA_ONSTACK to have "been cleared", by which I assume
> you mean a sigaction call with SA_ONSTACK not set in sa_flags. That is
> indeed possible, but it's not the only case your patch broke. It can just
> be a different signal whose sigaction never had SA_ONSTACK, when you are
> still on the signal stack from an earlier signal that did have SA_ONSTACK.
Thanks for your explanation.
>
>> So this patch is wrong :( . I will revise the other 4 patches.
>
> For 2 and 3, I would rather just wait until we unify signal.c anyway.
Ok. I see.
On Tue, 2008-02-19 at 18:49 -0800, Roland McGrath wrote:
> > I spent some time read you mail carefully and dig into the code again.
> >
> > And yes, you are right. It's possible that SA_ONSTACK has been cleared
> > before the second signal on the same stack comes.
>
> It's not necessary for SA_ONSTACK to have "been cleared", by which I assume
> you mean a sigaction call with SA_ONSTACK not set in sa_flags. That is
> indeed possible, but it's not the only case your patch broke. It can just
> be a different signal whose sigaction never had SA_ONSTACK, when you are
> still on the signal stack from an earlier signal that did have SA_ONSTACK.
>
> > So this patch is wrong :( . I will revise the other 4 patches.
>
> For 2 and 3, I would rather just wait until we unify signal.c anyway.
>
I've been looking at that, at the same time a bunch of ia32/signal.c
looks like it can go away.
Harvey
> I've been looking at that, at the same time a bunch of ia32/signal.c
> looks like it can go away.
Yes, I meant the 3 into 1 unification.
Thanks,
Roland
* Roland McGrath <[email protected]> wrote:
> > I spent some time read you mail carefully and dig into the code again.
> >
> > And yes, you are right. It's possible that SA_ONSTACK has been cleared
> > before the second signal on the same stack comes.
>
> It's not necessary for SA_ONSTACK to have "been cleared", by which I
> assume you mean a sigaction call with SA_ONSTACK not set in sa_flags.
> That is indeed possible, but it's not the only case your patch broke.
> It can just be a different signal whose sigaction never had
> SA_ONSTACK, when you are still on the signal stack from an earlier
> signal that did have SA_ONSTACK.
>
> > So this patch is wrong :( . I will revise the other 4 patches.
>
> For 2 and 3, I would rather just wait until we unify signal.c anyway.
ok, i've removed these patches from x86.git#testing for now:
Subject: x86: improve the signal stack overflow logic, 32-bit
Subject: x86: add a signal stack overflow check, 64-bit
Subject: x86: add signal stack overflow check, 32-bit
and will wait for a resubmission and an Ack from Roland.
Ingo