[PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Shi Weihua]

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 <shiwh@cn.fujitsu.com> 

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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Ingo Molnar]

* Shi Weihua <shiwh@cn.fujitsu.com> 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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

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


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Shi Weihua]

Ingo Molnar wrote::
> * Shi Weihua <shiwh@cn.fujitsu.com> 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
> 
> 
> 

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Shi Weihua]

Valdis.Kletnieks@vt.edu 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.

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Shi Weihua]

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 <shiwh@cn.fujitsu.com> 

---

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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Ingo Molnar]

* Shi Weihua <shiwh@cn.fujitsu.com> 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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Roland McGrath]

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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Paul Mackerras]

Ingo Molnar writes:
> 
> * Shi Weihua <shiwh@cn.fujitsu.com> 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.

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[H. Peter Anvin]

Paul Mackerras wrote:
> Ingo Molnar writes:
>> * Shi Weihua <shiwh@cn.fujitsu.com> 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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Roland McGrath]

> 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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Roland McGrath]

> 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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Shi Weihua]

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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Roland McGrath]

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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Shi Weihua]

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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Roland McGrath]

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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Shi Weihua]

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.

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Roland McGrath]

> 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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Shi Weihua]

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.

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Harvey Harrison]

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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Roland McGrath]

> 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

Re: [PATCH 1/5] signal(x86_32): Improve the signal stack overflow check

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> 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