Re: [tip:x86/signal] x86: signal: check signal stack overflow properly

[Oleg Nesterov <oleg@redhat.com>]

On 03/20, Hiroshi Shimamoto wrote:
>
> Commit-ID:  14fc9fbc700dc95b4f46ebd588169324fe6deff8
> Gitweb:     http://git.kernel.org/tip/14fc9fbc700dc95b4f46ebd588169324fe6deff8
> Author:     Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> AuthorDate: Thu, 19 Mar 2009 10:56:29 -0700
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Fri, 20 Mar 2009 19:01:31 +0100
>
> x86: signal: check signal stack overflow properly
>
> Impact: cleanup
>
> Check alternate signal stack overflow with proper stack pointer.
> The stack pointer of the next signal frame is different if that
> task has i387 state.

I think the patch is correct but I have a minor question,

> No need to check SA_ONSTACK if we're already using alternate signal stack.

Yes, but this also mean that we don't need sas_ss_flags() under
"if (!onsigstack)",

> @@ -211,31 +211,27 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size,
>  {
>  	/* Default to using normal stack */
>  	unsigned long sp = regs->sp;
> +	int onsigstack = on_sig_stack(sp);
>
>  #ifdef CONFIG_X86_64
>  	/* redzone */
>  	sp -= 128;
>  #endif /* CONFIG_X86_64 */
>
> -	/*
> -	 * 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)
> -			sp = current->sas_ss_sp + current->sas_ss_size;
> -	} else {
> +	if (!onsigstack) {
> +		/* This is the X/Open sanctioned signal stack switching.  */
> +		if (ka->sa.sa_flags & SA_ONSTACK) {
> +			if (sas_ss_flags(sp) == 0)
> +				sp = current->sas_ss_sp + current->sas_ss_size;

We can use "->sas_ss_size != 0" instead and avoid the unnecessary
sas_ss_flags()->on_sig_stack() check.

Please note that afaics sas_ss_flags()->on_sig_stack() is actually
wrong because we already adjusted "sp" above for redzone.

Suppose that on_sig_stack(regs->sp) = F, but "sp - 128" falls into
the altstack. In that case SA_ONSTACK won't switch the stack.

Of course, this is only theoretical, but still.

Oleg.