[PATCH 0/4] make sigaltstack() compatible with swapcontext()

[PATCH 0/4] make sigaltstack() compatible with swapcontext()

[Stas Sergeev]

The following patches add the self-test for
sigaltstack(SS_DISABLE) inside the signal handler,
and allow an app to temporarily disable and re-enable
the sigaltstack within a sighandler.
This is needed to make sigaltstack() compatible with
swapcontext(): before using swapcontext() inside
the sighandler, the sigaltstack should be disabled, or
the stack will be corrupted by the subsequent signals.



  arch/score/kernel/signal.c                   |    2
  arch/x86/kernel/signal.c                     |    2
  include/linux/sched.h                        |    8 +
  include/linux/signal.h                       |    2
  kernel/fork.c                                |    4
  kernel/signal.c                              |   54 +++++++----
  tools/testing/selftests/Makefile             |    1
  tools/testing/selftests/sigaltstack/Makefile |    8 +
  tools/testing/selftests/sigaltstack/sas.c    |  132 
+++++++++++++++++++++++++++
  9 files changed, 189 insertions(+), 24 deletions(-)

[PATCH 0/4] make sigaltstack() compatible with swapcontext()

[Stas Sergeev]

The following patches add the self-test for
sigaltstack(SS_DISABLE) inside the signal handler,
and allow an app to temporarily disable and re-enable
the sigaltstack within a sighandler.
This is needed to make sigaltstack() compatible with
swapcontext(): before using swapcontext() inside
the sighandler, the sigaltstack should be disabled, or
the stack will be corrupted by the subsequent signals.



  arch/score/kernel/signal.c                   |    2
  arch/x86/kernel/signal.c                     |    2
  include/linux/sched.h                        |    8 +
  include/linux/signal.h                       |    2
  kernel/fork.c                                |    4
  kernel/signal.c                              |   54 +++++++----
  tools/testing/selftests/Makefile             |    1
  tools/testing/selftests/sigaltstack/Makefile |    8 +
  tools/testing/selftests/sigaltstack/sas.c    |  132 
+++++++++++++++++++++++++++
  9 files changed, 189 insertions(+), 24 deletions(-)

[PATCH 2/4] score: signal: fix sigaltstack check

[Stas Sergeev]

Currently get_sigframe() checks only
(ka->sa.sa_flags & SA_ONSTACK) && (!on_sig_stack(sp))
to determine whether the switch to sigaltstack is needed.
It forgets to checks whether the sigaltstack was previously set.

This patch replaces the !on_sig_stack(sp) with the standard check
sas_ss_flags(sp) == 0
that takes into account both conditions: it succeeds only if the
sigaltstack is enabled but currently not active.

CC: Andy Lutomirski <luto@amacapital.net>
CC: linux-kernel@vger.kernel.org
CC: Chen Liqin <liqin.linux@gmail.com>
CC: Lennox Wu <lennox.wu@gmail.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Hogan <james.hogan@imgtec.com>

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  arch/score/kernel/signal.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/score/kernel/signal.c b/arch/score/kernel/signal.c
index e381c8c..bd1c7c8 100644
--- a/arch/score/kernel/signal.c
+++ b/arch/score/kernel/signal.c
@@ -127,7 +127,7 @@ static void __user *get_sigframe(struct k_sigaction *ka,
      sp -= 32;

      /* This is the X/Open sanctioned signal stack switching.  */
-    if ((ka->sa.sa_flags & SA_ONSTACK) && (!on_sig_stack(sp)))
+    if ((ka->sa.sa_flags & SA_ONSTACK) && (sas_ss_flags(sp) == 0))
          sp = current->sas_ss_sp + current->sas_ss_size;

      return (void __user*)((sp - frame_size) & ~7);
-- 
2.5.0

Re: [PATCH 2/4] score: signal: fix sigaltstack check

[Lennox Wu]

Hi Sergeev,
Your are right. I will merge it.
Thanks.

Acked-by: Lennox Wu <lennox.wu@gmail.com>

2016-02-01 0:21 GMT+08:00 Stas Sergeev <stsp@list.ru>:
>
> Currently get_sigframe() checks only
> (ka->sa.sa_flags & SA_ONSTACK) && (!on_sig_stack(sp))
> to determine whether the switch to sigaltstack is needed.
> It forgets to checks whether the sigaltstack was previously set.
>
> This patch replaces the !on_sig_stack(sp) with the standard check
> sas_ss_flags(sp) == 0
> that takes into account both conditions: it succeeds only if the
> sigaltstack is enabled but currently not active.
>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: linux-kernel@vger.kernel.org
> CC: Chen Liqin <liqin.linux@gmail.com>
> CC: Lennox Wu <lennox.wu@gmail.com>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: James Hogan <james.hogan@imgtec.com>
>
> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
> ---
>  arch/score/kernel/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/score/kernel/signal.c b/arch/score/kernel/signal.c
> index e381c8c..bd1c7c8 100644
> --- a/arch/score/kernel/signal.c
> +++ b/arch/score/kernel/signal.c
> @@ -127,7 +127,7 @@ static void __user *get_sigframe(struct k_sigaction *ka,
>      sp -= 32;
>
>      /* This is the X/Open sanctioned signal stack switching.  */
> -    if ((ka->sa.sa_flags & SA_ONSTACK) && (!on_sig_stack(sp)))
> +    if ((ka->sa.sa_flags & SA_ONSTACK) && (sas_ss_flags(sp) == 0))
>          sp = current->sas_ss_sp + current->sas_ss_size;
>
>      return (void __user*)((sp - frame_size) & ~7);
> --
> 2.5.0
>

[PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[Stas Sergeev]

Currently x86's get_sigframe() checks for "current->sas_ss_size"
to determine whether there is a need to switch to sigaltstack.
The common practice used by all other arches is to check for
sas_ss_flags(sp) == 0

This patch makes the code consistent with other arches and also
allows for the further sigaltstack improvements within this patch serie.

CC: Andy Lutomirski <luto@amacapital.net>
CC: linux-kernel@vger.kernel.org
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Borislav Petkov <bp@suse.de>
CC: Brian Gerst <brgerst@gmail.com>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Richard Weinberger <richard@nod.at>

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  arch/x86/kernel/signal.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index cb6282c..3955259 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -216,7 +216,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs 
*regs, size_t frame_size,
      if (!onsigstack) {
          /* This is the X/Open sanctioned signal stack switching. */
          if (ka->sa.sa_flags & SA_ONSTACK) {
-            if (current->sas_ss_size)
+            if (sas_ss_flags(sp) == 0)
                  sp = current->sas_ss_sp + current->sas_ss_size;
          } else if (config_enabled(CONFIG_X86_32) &&
                 (regs->ss & 0xffff) != __USER_DS &&
-- 
2.5.0

Re: [PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 8:24 AM, Stas Sergeev <stsp@list.ru> wrote:
>
> Currently x86's get_sigframe() checks for "current->sas_ss_size"
> to determine whether there is a need to switch to sigaltstack.
> The common practice used by all other arches is to check for
> sas_ss_flags(sp) == 0
>
> This patch makes the code consistent with other arches and also
> allows for the further sigaltstack improvements within this patch serie.

You've duplicate part of the check.  That whole block is already
checking !sigstack.  Can you remove that now?

--Andy

Re: [PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[Stas Sergeev]

31.01.2016 19:58, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 8:24 AM, Stas Sergeev <stsp@list.ru> wrote:
>> Currently x86's get_sigframe() checks for "current->sas_ss_size"
>> to determine whether there is a need to switch to sigaltstack.
>> The common practice used by all other arches is to check for
>> sas_ss_flags(sp) == 0
>>
>> This patch makes the code consistent with other arches and also
>> allows for the further sigaltstack improvements within this patch serie.
> You've duplicate part of the check.  That whole block is already
> checking !sigstack.  Can you remove that now?
OK.

[PATCH 1/4] selftests: Add test for sigaltstack(SS_DISABLE) inside sighandler

[Stas Sergeev]

sigaltstack needs to be disabled before the signal handler can
safely use swapcontext(). Unfortunately linux implementation of
sigaltstack() returns EPERM in that case.
Re-enabling is also needed and tested.

CC: Shuah Khan <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
CC: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>

Signed-off-by: Stas Sergeev <stsp-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
  tools/testing/selftests/Makefile             |   1 +
  tools/testing/selftests/sigaltstack/Makefile |   8 ++
  tools/testing/selftests/sigaltstack/sas.c    | 132 
+++++++++++++++++++++++++++
  3 files changed, 141 insertions(+)
  create mode 100644 tools/testing/selftests/sigaltstack/Makefile
  create mode 100644 tools/testing/selftests/sigaltstack/sas.c

diff --git a/tools/testing/selftests/Makefile 
b/tools/testing/selftests/Makefile
index c8edff6..d5b2005 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -17,6 +17,7 @@ TARGETS += powerpc
  TARGETS += pstore
  TARGETS += ptrace
  TARGETS += seccomp
+TARGETS += sigaltstack
  TARGETS += size
  TARGETS += static_keys
  TARGETS += sysctl
diff --git a/tools/testing/selftests/sigaltstack/Makefile 
b/tools/testing/selftests/sigaltstack/Makefile
new file mode 100644
index 0000000..56af56e
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/Makefile
@@ -0,0 +1,8 @@
+CFLAGS = -Wall
+BINARIES = sas
+all: $(BINARIES)
+
+include ../lib.mk
+
+clean:
+    rm -rf $(BINARIES)
diff --git a/tools/testing/selftests/sigaltstack/sas.c 
b/tools/testing/selftests/sigaltstack/sas.c
new file mode 100644
index 0000000..5d9aabd
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -0,0 +1,132 @@
+/*
+ * Stas Sergeev <stsp-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
+ *
+ * test sigcontext(SS_DISABLE) inside signal handler
+ * If that succeeds, then swapcontext() can be used safely.
+ *
+ */
+
+#define _GNU_SOURCE
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <ucontext.h>
+#include <alloca.h>
+#include <string.h>
+#include <assert.h>
+
+static void *sstack, *ustack;
+static ucontext_t uc, sc;
+static const char *msg = "[OK]\tStack preserved";
+static const char *msg2 = "[FAIL]\tStack corrupted";
+
+void my_usr1(int sig)
+{
+    char *aa, *p;
+    int *i, err;
+    stack_t stk = { };
+
+    aa = alloca(1024);
+    assert(aa);
+    p = aa + 512;
+    strcpy(p, msg);
+    i = (int *) (p + 128);
+    *i = 1;
+    printf("[RUN]\tsignal USR1\n");
+    stk.ss_flags = SS_DISABLE;
+    err = sigaltstack(&stk, &stk);
+    if (err) {
+        perror("[FAIL]\tsigaltstack(SS_DISABLE)");
+        /* don't exit to demonstrate the breakage */
+        /* exit(EXIT_FAILURE); */
+    }
+    swapcontext(&sc, &uc);
+    printf("%s\n", p);
+    if (!*i) {
+        printf("[RUN]\tAborting\n");
+        exit(EXIT_FAILURE);
+    }
+
+    if (stk.ss_flags != SS_ONSTACK) {
+        printf("[FAIL]\tsigaltstack() returned wrong ss_flags %i\n",
+                stk.ss_flags);
+        stk.ss_flags = SS_ONSTACK;
+    }
+    err = sigaltstack(&stk, NULL);
+    if (err)
+        printf("[OK]\tsigaltstack(SS_ONSTACK) failed for non_zero "
+                "size\n");
+        /* but don't fail otherwise, as this is unspecified */
+    stk.ss_size = 0;
+    err = sigaltstack(&stk, NULL);
+    if (err)
+        perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
+}
+
+void my_usr2(int sig)
+{
+    char *aa, *p;
+    int *i;
+
+    printf("[RUN]\tsignal USR2\n");
+    aa = alloca(1024);
+    /* dont run valgrind on this */
+    p = memmem(aa, 1024, msg, strlen(msg));
+    if (p) {
+        printf("[FAIL]\tsigaltstack re-used\n");
+        strcpy(p, msg2);
+        i = (int *) (p + 128);
+        *i = 0;
+    }
+}
+
+static void switch_fn(void)
+{
+    printf("[RUN]\tswitched to user ctx\n");
+    raise(SIGUSR2);
+    setcontext(&sc);
+}
+
+int main(void)
+{
+    struct sigaction act;
+    stack_t stk;
+    int err;
+
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = SA_ONSTACK;
+    act.sa_handler = my_usr1;
+    sigaction(SIGUSR1, &act, NULL);
+    act.sa_handler = my_usr2;
+    sigaction(SIGUSR2, &act, NULL);
+    sstack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+    if (sstack == MAP_FAILED) {
+        perror("mmap()");
+        return EXIT_FAILURE;
+    }
+    stk.ss_sp = sstack;
+    stk.ss_size = SIGSTKSZ;
+    stk.ss_flags = SS_ONSTACK;
+    err = sigaltstack(&stk, NULL);
+    if (err) {
+        perror("sigaltstack()");
+        return EXIT_FAILURE;
+    }
+
+    ustack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+    if (ustack == MAP_FAILED) {
+        perror("mmap()");
+        return EXIT_FAILURE;
+    }
+    getcontext(&uc);
+    uc.uc_link = NULL;
+    uc.uc_stack.ss_sp = ustack;
+    uc.uc_stack.ss_size = SIGSTKSZ;
+    makecontext(&uc, switch_fn, 0);
+    raise(SIGUSR1);
+    printf("[OK]\tTest passed\n");
+    return 0;
+}
-- 
2.5.0

[PATCH 1/4] selftests: Add test for sigaltstack(SS_DISABLE) inside sighandler

[Stas Sergeev]

sigaltstack needs to be disabled before the signal handler can
safely use swapcontext(). Unfortunately linux implementation of
sigaltstack() returns EPERM in that case.
Re-enabling is also needed and tested.

CC: Shuah Khan <shuahkh@osg.samsung.com>
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
CC: Andy Lutomirski <luto@amacapital.net>

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  tools/testing/selftests/Makefile             |   1 +
  tools/testing/selftests/sigaltstack/Makefile |   8 ++
  tools/testing/selftests/sigaltstack/sas.c    | 132 
+++++++++++++++++++++++++++
  3 files changed, 141 insertions(+)
  create mode 100644 tools/testing/selftests/sigaltstack/Makefile
  create mode 100644 tools/testing/selftests/sigaltstack/sas.c

diff --git a/tools/testing/selftests/Makefile 
b/tools/testing/selftests/Makefile
index c8edff6..d5b2005 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -17,6 +17,7 @@ TARGETS += powerpc
  TARGETS += pstore
  TARGETS += ptrace
  TARGETS += seccomp
+TARGETS += sigaltstack
  TARGETS += size
  TARGETS += static_keys
  TARGETS += sysctl
diff --git a/tools/testing/selftests/sigaltstack/Makefile 
b/tools/testing/selftests/sigaltstack/Makefile
new file mode 100644
index 0000000..56af56e
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/Makefile
@@ -0,0 +1,8 @@
+CFLAGS = -Wall
+BINARIES = sas
+all: $(BINARIES)
+
+include ../lib.mk
+
+clean:
+    rm -rf $(BINARIES)
diff --git a/tools/testing/selftests/sigaltstack/sas.c 
b/tools/testing/selftests/sigaltstack/sas.c
new file mode 100644
index 0000000..5d9aabd
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -0,0 +1,132 @@
+/*
+ * Stas Sergeev <stsp@users.sourceforge.net>
+ *
+ * test sigcontext(SS_DISABLE) inside signal handler
+ * If that succeeds, then swapcontext() can be used safely.
+ *
+ */
+
+#define _GNU_SOURCE
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <ucontext.h>
+#include <alloca.h>
+#include <string.h>
+#include <assert.h>
+
+static void *sstack, *ustack;
+static ucontext_t uc, sc;
+static const char *msg = "[OK]\tStack preserved";
+static const char *msg2 = "[FAIL]\tStack corrupted";
+
+void my_usr1(int sig)
+{
+    char *aa, *p;
+    int *i, err;
+    stack_t stk = { };
+
+    aa = alloca(1024);
+    assert(aa);
+    p = aa + 512;
+    strcpy(p, msg);
+    i = (int *) (p + 128);
+    *i = 1;
+    printf("[RUN]\tsignal USR1\n");
+    stk.ss_flags = SS_DISABLE;
+    err = sigaltstack(&stk, &stk);
+    if (err) {
+        perror("[FAIL]\tsigaltstack(SS_DISABLE)");
+        /* don't exit to demonstrate the breakage */
+        /* exit(EXIT_FAILURE); */
+    }
+    swapcontext(&sc, &uc);
+    printf("%s\n", p);
+    if (!*i) {
+        printf("[RUN]\tAborting\n");
+        exit(EXIT_FAILURE);
+    }
+
+    if (stk.ss_flags != SS_ONSTACK) {
+        printf("[FAIL]\tsigaltstack() returned wrong ss_flags %i\n",
+                stk.ss_flags);
+        stk.ss_flags = SS_ONSTACK;
+    }
+    err = sigaltstack(&stk, NULL);
+    if (err)
+        printf("[OK]\tsigaltstack(SS_ONSTACK) failed for non_zero "
+                "size\n");
+        /* but don't fail otherwise, as this is unspecified */
+    stk.ss_size = 0;
+    err = sigaltstack(&stk, NULL);
+    if (err)
+        perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
+}
+
+void my_usr2(int sig)
+{
+    char *aa, *p;
+    int *i;
+
+    printf("[RUN]\tsignal USR2\n");
+    aa = alloca(1024);
+    /* dont run valgrind on this */
+    p = memmem(aa, 1024, msg, strlen(msg));
+    if (p) {
+        printf("[FAIL]\tsigaltstack re-used\n");
+        strcpy(p, msg2);
+        i = (int *) (p + 128);
+        *i = 0;
+    }
+}
+
+static void switch_fn(void)
+{
+    printf("[RUN]\tswitched to user ctx\n");
+    raise(SIGUSR2);
+    setcontext(&sc);
+}
+
+int main(void)
+{
+    struct sigaction act;
+    stack_t stk;
+    int err;
+
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = SA_ONSTACK;
+    act.sa_handler = my_usr1;
+    sigaction(SIGUSR1, &act, NULL);
+    act.sa_handler = my_usr2;
+    sigaction(SIGUSR2, &act, NULL);
+    sstack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+    if (sstack == MAP_FAILED) {
+        perror("mmap()");
+        return EXIT_FAILURE;
+    }
+    stk.ss_sp = sstack;
+    stk.ss_size = SIGSTKSZ;
+    stk.ss_flags = SS_ONSTACK;
+    err = sigaltstack(&stk, NULL);
+    if (err) {
+        perror("sigaltstack()");
+        return EXIT_FAILURE;
+    }
+
+    ustack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+    if (ustack == MAP_FAILED) {
+        perror("mmap()");
+        return EXIT_FAILURE;
+    }
+    getcontext(&uc);
+    uc.uc_link = NULL;
+    uc.uc_stack.ss_sp = ustack;
+    uc.uc_stack.ss_size = SIGSTKSZ;
+    makecontext(&uc, switch_fn, 0);
+    raise(SIGUSR1);
+    printf("[OK]\tTest passed\n");
+    return 0;
+}
-- 
2.5.0

Re: [PATCH 1/4] selftests: Add test for sigaltstack(SS_DISABLE) inside sighandler

[Shuah Khan]

On 01/31/2016 09:18 AM, Stas Sergeev wrote:
> 
> sigaltstack needs to be disabled before the signal handler can
> safely use swapcontext(). Unfortunately linux implementation of
> sigaltstack() returns EPERM in that case.
> Re-enabling is also needed and tested.
> 
> CC: Shuah Khan <shuahkh@osg.samsung.com>
> CC: linux-kernel@vger.kernel.org
> CC: linux-api@vger.kernel.org
> CC: Andy Lutomirski <luto@amacapital.net>
> 
> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>

Hi Stas,

Is this patch v4 or Patch 1/4. Confirming to see if
I am missing 3 patches or this is supposed to be
version 4 of a single patch.

thanks,
-- Shuah
> ---
>  tools/testing/selftests/Makefile             |   1 +
>  tools/testing/selftests/sigaltstack/Makefile |   8 ++
>  tools/testing/selftests/sigaltstack/sas.c    | 132 +++++++++++++++++++++++++++
>  3 files changed, 141 insertions(+)
>  create mode 100644 tools/testing/selftests/sigaltstack/Makefile
>  create mode 100644 tools/testing/selftests/sigaltstack/sas.c
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index c8edff6..d5b2005 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -17,6 +17,7 @@ TARGETS += powerpc
>  TARGETS += pstore
>  TARGETS += ptrace
>  TARGETS += seccomp
> +TARGETS += sigaltstack
>  TARGETS += size
>  TARGETS += static_keys
>  TARGETS += sysctl
> diff --git a/tools/testing/selftests/sigaltstack/Makefile b/tools/testing/selftests/sigaltstack/Makefile
> new file mode 100644
> index 0000000..56af56e
> --- /dev/null
> +++ b/tools/testing/selftests/sigaltstack/Makefile
> @@ -0,0 +1,8 @@
> +CFLAGS = -Wall
> +BINARIES = sas
> +all: $(BINARIES)
> +
> +include ../lib.mk
> +
> +clean:
> +    rm -rf $(BINARIES)
> diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
> new file mode 100644
> index 0000000..5d9aabd
> --- /dev/null
> +++ b/tools/testing/selftests/sigaltstack/sas.c
> @@ -0,0 +1,132 @@
> +/*
> + * Stas Sergeev <stsp@users.sourceforge.net>
> + *
> + * test sigcontext(SS_DISABLE) inside signal handler
> + * If that succeeds, then swapcontext() can be used safely.
> + *
> + */
> +
> +#define _GNU_SOURCE
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/mman.h>
> +#include <ucontext.h>
> +#include <alloca.h>
> +#include <string.h>
> +#include <assert.h>
> +
> +static void *sstack, *ustack;
> +static ucontext_t uc, sc;
> +static const char *msg = "[OK]\tStack preserved";
> +static const char *msg2 = "[FAIL]\tStack corrupted";
> +
> +void my_usr1(int sig)
> +{
> +    char *aa, *p;
> +    int *i, err;
> +    stack_t stk = { };
> +
> +    aa = alloca(1024);
> +    assert(aa);
> +    p = aa + 512;
> +    strcpy(p, msg);
> +    i = (int *) (p + 128);
> +    *i = 1;
> +    printf("[RUN]\tsignal USR1\n");
> +    stk.ss_flags = SS_DISABLE;
> +    err = sigaltstack(&stk, &stk);
> +    if (err) {
> +        perror("[FAIL]\tsigaltstack(SS_DISABLE)");
> +        /* don't exit to demonstrate the breakage */
> +        /* exit(EXIT_FAILURE); */
> +    }
> +    swapcontext(&sc, &uc);
> +    printf("%s\n", p);
> +    if (!*i) {
> +        printf("[RUN]\tAborting\n");
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    if (stk.ss_flags != SS_ONSTACK) {
> +        printf("[FAIL]\tsigaltstack() returned wrong ss_flags %i\n",
> +                stk.ss_flags);
> +        stk.ss_flags = SS_ONSTACK;
> +    }
> +    err = sigaltstack(&stk, NULL);
> +    if (err)
> +        printf("[OK]\tsigaltstack(SS_ONSTACK) failed for non_zero "
> +                "size\n");
> +        /* but don't fail otherwise, as this is unspecified */
> +    stk.ss_size = 0;
> +    err = sigaltstack(&stk, NULL);
> +    if (err)
> +        perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
> +}
> +
> +void my_usr2(int sig)
> +{
> +    char *aa, *p;
> +    int *i;
> +
> +    printf("[RUN]\tsignal USR2\n");
> +    aa = alloca(1024);
> +    /* dont run valgrind on this */
> +    p = memmem(aa, 1024, msg, strlen(msg));
> +    if (p) {
> +        printf("[FAIL]\tsigaltstack re-used\n");
> +        strcpy(p, msg2);
> +        i = (int *) (p + 128);
> +        *i = 0;
> +    }
> +}
> +
> +static void switch_fn(void)
> +{
> +    printf("[RUN]\tswitched to user ctx\n");
> +    raise(SIGUSR2);
> +    setcontext(&sc);
> +}
> +
> +int main(void)
> +{
> +    struct sigaction act;
> +    stack_t stk;
> +    int err;
> +
> +    sigemptyset(&act.sa_mask);
> +    act.sa_flags = SA_ONSTACK;
> +    act.sa_handler = my_usr1;
> +    sigaction(SIGUSR1, &act, NULL);
> +    act.sa_handler = my_usr2;
> +    sigaction(SIGUSR2, &act, NULL);
> +    sstack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
> +              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
> +    if (sstack == MAP_FAILED) {
> +        perror("mmap()");
> +        return EXIT_FAILURE;
> +    }
> +    stk.ss_sp = sstack;
> +    stk.ss_size = SIGSTKSZ;
> +    stk.ss_flags = SS_ONSTACK;
> +    err = sigaltstack(&stk, NULL);
> +    if (err) {
> +        perror("sigaltstack()");
> +        return EXIT_FAILURE;
> +    }
> +
> +    ustack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
> +              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
> +    if (ustack == MAP_FAILED) {
> +        perror("mmap()");
> +        return EXIT_FAILURE;
> +    }
> +    getcontext(&uc);
> +    uc.uc_link = NULL;
> +    uc.uc_stack.ss_sp = ustack;
> +    uc.uc_stack.ss_size = SIGSTKSZ;
> +    makecontext(&uc, switch_fn, 0);
> +    raise(SIGUSR1);
> +    printf("[OK]\tTest passed\n");
> +    return 0;
> +}


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH 1/4] selftests: Add test for sigaltstack(SS_DISABLE) inside sighandler

[Stas Sergeev]

12.02.2016 19:12, Shuah Khan пишет:
> On 01/31/2016 09:18 AM, Stas Sergeev wrote:
>>
>> sigaltstack needs to be disabled before the signal handler can
>> safely use swapcontext(). Unfortunately linux implementation of
>> sigaltstack() returns EPERM in that case.
>> Re-enabling is also needed and tested.
>>
>> CC: Shuah Khan <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
>> CC: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> CC: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>>
>> Signed-off-by: Stas Sergeev <stsp-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> 
> Hi Stas,
> 
> Is this patch v4 or Patch 1/4. Confirming to see if
> I am missing 3 patches or this is supposed to be
> version 4 of a single patch.
Hi, I used get_maintainer.pl for the CC list, so you indeed
were CCed with just this patch, which is one of 4.
The series were however rejected, and I am planning to submit
another one later.

Re: [PATCH 1/4] selftests: Add test for sigaltstack(SS_DISABLE) inside sighandler

[Stas Sergeev]

12.02.2016 19:12, Shuah Khan пишет:
> On 01/31/2016 09:18 AM, Stas Sergeev wrote:
>>
>> sigaltstack needs to be disabled before the signal handler can
>> safely use swapcontext(). Unfortunately linux implementation of
>> sigaltstack() returns EPERM in that case.
>> Re-enabling is also needed and tested.
>>
>> CC: Shuah Khan <shuahkh@osg.samsung.com>
>> CC: linux-kernel@vger.kernel.org
>> CC: linux-api@vger.kernel.org
>> CC: Andy Lutomirski <luto@amacapital.net>
>>
>> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
> 
> Hi Stas,
> 
> Is this patch v4 or Patch 1/4. Confirming to see if
> I am missing 3 patches or this is supposed to be
> version 4 of a single patch.
Hi, I used get_maintainer.pl for the CC list, so you indeed
were CCed with just this patch, which is one of 4.
The series were however rejected, and I am planning to submit
another one later.

[PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

linux implements the sigaltstack() in a way that makes it impossible to
use with swapcontext(). Per the man page, sigaltstack is allowed to return
EPERM if the process is altering its sigaltstack while running on
sigaltstack.
This is likely needed to consistently return oss->ss_flags, that indicates
whether the process is being on sigaltstack or not.
Unfortunately, linux takes that permission to return EPERM too literally:
it returns EPERM even if you don't want to change to another sigaltstack,
but only want to temporarily disable sigaltstack with SS_DISABLE.
You can't use swapcontext() without disabling sigaltstack first, or the
stack will be re-used and overwritten by a subsequent signal.

With this patch, disabling sigaltstack inside a signal handler became
possible, and the swapcontext() can then be used safely. After switching
back to the sighandler, the app can re-enable the sigatlstack.
The oss->ss_flags will correctly indicate the current use of sigaltstack,
even if it is temporarily disabled. Any attempt to modify the sigaltstack
(rather than to disable or re-enable it) within the sighandler, will still
be punished with EPERM as suggested by POSIX.

CC: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
CC: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: "Amanieu d'Antras" <amanieu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
CC: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
CC: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: "Kirill A. Shutemov" <kirill.shutemov-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
CC: Jason Low <jason.low2-VXdhtT5mjnY@public.gmane.org>
CC: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
CC: Andrea Arcangeli <aarcange-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Konstantin Khlebnikov <khlebnikov-XoJtRXgx1JseBXzfvpsJ4g@public.gmane.org>
CC: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
CC: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
CC: Aleksa Sarai <cyphar-gVpy/LI/lHzQT0dZR+AlfA@public.gmane.org>
CC: Paul Moore <pmoore-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Palmer Dabbelt <palmer-96lFi9zoCfxBDgjK7y7TUQ@public.gmane.org>
CC: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
CC: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Signed-off-by: Stas Sergeev <stsp-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
  include/linux/sched.h  |  8 ++++++--
  include/linux/signal.h |  2 +-
  kernel/fork.c          |  4 +++-
  kernel/signal.c        | 54 
+++++++++++++++++++++++++++++++++-----------------
  4 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index edad7a4..ee8749e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1572,6 +1572,7 @@ struct task_struct {

      unsigned long sas_ss_sp;
      size_t sas_ss_size;
+    int sas_ss_flags;

      struct callback_head *task_works;

@@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long sp)
  {
      if (!current->sas_ss_size)
          return SS_DISABLE;
-
-    return on_sig_stack(sp) ? SS_ONSTACK : 0;
+    if (on_sig_stack(sp))
+        return SS_ONSTACK;
+    if (current->sas_ss_flags == SS_DISABLE)
+        return SS_DISABLE;
+    return 0;
  }

  static inline unsigned long sigsp(unsigned long sp, struct ksignal *ksig)
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 92557bb..844b113 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -432,7 +432,7 @@ int __save_altstack(stack_t __user *, unsigned long);
      stack_t __user *__uss = uss; \
      struct task_struct *t = current; \
      put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \
-    put_user_ex(sas_ss_flags(sp), &__uss->ss_flags); \
+    put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
      put_user_ex(t->sas_ss_size, &__uss->ss_size); \
  } while (0);

diff --git a/kernel/fork.c b/kernel/fork.c
index fce002e..e5edc5c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1483,8 +1483,10 @@ static struct task_struct *copy_process(unsigned 
long clone_flags,
      /*
       * sigaltstack should be cleared when sharing the same VM
       */
-    if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM)
+    if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM) {
          p->sas_ss_sp = p->sas_ss_size = 0;
+        p->sas_ss_flags = SS_DISABLE;
+    }

      /*
       * Syscall tracing and stepping should be turned off in the
diff --git a/kernel/signal.c b/kernel/signal.c
index f3f1f7a..d19180e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3101,6 +3101,7 @@ do_sigaltstack (const stack_t __user *uss, stack_t 
__user *uoss, unsigned long s
          void __user *ss_sp;
          size_t ss_size;
          int ss_flags;
+        int onsigstack;

          error = -EFAULT;
          if (!access_ok(VERIFY_READ, uss, sizeof(*uss)))
@@ -3111,32 +3112,49 @@ do_sigaltstack (const stack_t __user *uss, 
stack_t __user *uoss, unsigned long s
          if (error)
              goto out;

-        error = -EPERM;
-        if (on_sig_stack(sp))
-            goto out;
-
          error = -EINVAL;
-        /*
-         * Note - this code used to test ss_flags incorrectly:
-         *        old code may have been written using ss_flags==0
-         *      to mean ss_flags==SS_ONSTACK (as this was the only
-         *      way that worked) - this fix preserves that older
-         *      mechanism.
-         */
          if (ss_flags != SS_DISABLE && ss_flags != SS_ONSTACK && 
ss_flags != 0)
              goto out;

-        if (ss_flags == SS_DISABLE) {
-            ss_size = 0;
-            ss_sp = NULL;
-        } else {
+        onsigstack = on_sig_stack(sp);
+        if (ss_size == 0) {
+            switch (ss_flags) {
+            case 0:
+                error = -EPERM;
+                if (onsigstack)
+                    goto out;
+                current->sas_ss_sp = 0;
+                current->sas_ss_size = 0;
+                current->sas_ss_flags = SS_DISABLE;
+                break;
+            case SS_ONSTACK:
+                /* re-enable previously disabled sas */
+                error = -EINVAL;
+                if (current->sas_ss_size == 0)
+                    goto out;
+                break;
+            default:
+                break;
+            }
+        } else if (ss_flags != SS_DISABLE) {
+            error = -EPERM;
+            if (onsigstack)
+                goto out;
              error = -ENOMEM;
              if (ss_size < MINSIGSTKSZ)
                  goto out;
+            current->sas_ss_sp = (unsigned long) ss_sp;
+            current->sas_ss_size = ss_size;
+            /* unfortunately POSIX forces us to treat 0
+             * as SS_ONSTACK here, and some legacy apps
+             * perhaps used that...
+             */
+            if (ss_flags == 0)
+                ss_flags = SS_ONSTACK;
          }

-        current->sas_ss_sp = (unsigned long) ss_sp;
-        current->sas_ss_size = ss_size;
+        if (ss_flags != 0)
+            current->sas_ss_flags = ss_flags;
      }

      error = 0;
@@ -3168,7 +3186,7 @@ int __save_altstack(stack_t __user *uss, unsigned 
long sp)
  {
      struct task_struct *t = current;
      return  __put_user((void __user *)t->sas_ss_sp, &uss->ss_sp) |
-        __put_user(sas_ss_flags(sp), &uss->ss_flags) |
+        __put_user(t->sas_ss_flags, &uss->ss_flags) |
          __put_user(t->sas_ss_size, &uss->ss_size);
  }

-- 
2.5.0

[PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

linux implements the sigaltstack() in a way that makes it impossible to
use with swapcontext(). Per the man page, sigaltstack is allowed to return
EPERM if the process is altering its sigaltstack while running on
sigaltstack.
This is likely needed to consistently return oss->ss_flags, that indicates
whether the process is being on sigaltstack or not.
Unfortunately, linux takes that permission to return EPERM too literally:
it returns EPERM even if you don't want to change to another sigaltstack,
but only want to temporarily disable sigaltstack with SS_DISABLE.
You can't use swapcontext() without disabling sigaltstack first, or the
stack will be re-used and overwritten by a subsequent signal.

With this patch, disabling sigaltstack inside a signal handler became
possible, and the swapcontext() can then be used safely. After switching
back to the sighandler, the app can re-enable the sigatlstack.
The oss->ss_flags will correctly indicate the current use of sigaltstack,
even if it is temporarily disabled. Any attempt to modify the sigaltstack
(rather than to disable or re-enable it) within the sighandler, will still
be punished with EPERM as suggested by POSIX.

CC: Ingo Molnar <mingo@redhat.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: "Amanieu d'Antras" <amanieu@gmail.com>
CC: Richard Weinberger <richard@nod.at>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Tejun Heo <tj@kernel.org>
CC: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
CC: Jason Low <jason.low2@hp.com>
CC: Heinrich Schuchardt <xypron.glpk@gmx.de>
CC: Andrea Arcangeli <aarcange@redhat.com>
CC: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
CC: Josh Triplett <josh@joshtriplett.org>
CC: "Eric W. Biederman" <ebiederm@xmission.com>
CC: Aleksa Sarai <cyphar@cyphar.com>
CC: Paul Moore <pmoore@redhat.com>
CC: Palmer Dabbelt <palmer@dabbelt.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  include/linux/sched.h  |  8 ++++++--
  include/linux/signal.h |  2 +-
  kernel/fork.c          |  4 +++-
  kernel/signal.c        | 54 
+++++++++++++++++++++++++++++++++-----------------
  4 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index edad7a4..ee8749e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1572,6 +1572,7 @@ struct task_struct {

      unsigned long sas_ss_sp;
      size_t sas_ss_size;
+    int sas_ss_flags;

      struct callback_head *task_works;

@@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long sp)
  {
      if (!current->sas_ss_size)
          return SS_DISABLE;
-
-    return on_sig_stack(sp) ? SS_ONSTACK : 0;
+    if (on_sig_stack(sp))
+        return SS_ONSTACK;
+    if (current->sas_ss_flags == SS_DISABLE)
+        return SS_DISABLE;
+    return 0;
  }

  static inline unsigned long sigsp(unsigned long sp, struct ksignal *ksig)
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 92557bb..844b113 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -432,7 +432,7 @@ int __save_altstack(stack_t __user *, unsigned long);
      stack_t __user *__uss = uss; \
      struct task_struct *t = current; \
      put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \
-    put_user_ex(sas_ss_flags(sp), &__uss->ss_flags); \
+    put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
      put_user_ex(t->sas_ss_size, &__uss->ss_size); \
  } while (0);

diff --git a/kernel/fork.c b/kernel/fork.c
index fce002e..e5edc5c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1483,8 +1483,10 @@ static struct task_struct *copy_process(unsigned 
long clone_flags,
      /*
       * sigaltstack should be cleared when sharing the same VM
       */
-    if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM)
+    if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM) {
          p->sas_ss_sp = p->sas_ss_size = 0;
+        p->sas_ss_flags = SS_DISABLE;
+    }

      /*
       * Syscall tracing and stepping should be turned off in the
diff --git a/kernel/signal.c b/kernel/signal.c
index f3f1f7a..d19180e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3101,6 +3101,7 @@ do_sigaltstack (const stack_t __user *uss, stack_t 
__user *uoss, unsigned long s
          void __user *ss_sp;
          size_t ss_size;
          int ss_flags;
+        int onsigstack;

          error = -EFAULT;
          if (!access_ok(VERIFY_READ, uss, sizeof(*uss)))
@@ -3111,32 +3112,49 @@ do_sigaltstack (const stack_t __user *uss, 
stack_t __user *uoss, unsigned long s
          if (error)
              goto out;

-        error = -EPERM;
-        if (on_sig_stack(sp))
-            goto out;
-
          error = -EINVAL;
-        /*
-         * Note - this code used to test ss_flags incorrectly:
-         *        old code may have been written using ss_flags==0
-         *      to mean ss_flags==SS_ONSTACK (as this was the only
-         *      way that worked) - this fix preserves that older
-         *      mechanism.
-         */
          if (ss_flags != SS_DISABLE && ss_flags != SS_ONSTACK && 
ss_flags != 0)
              goto out;

-        if (ss_flags == SS_DISABLE) {
-            ss_size = 0;
-            ss_sp = NULL;
-        } else {
+        onsigstack = on_sig_stack(sp);
+        if (ss_size == 0) {
+            switch (ss_flags) {
+            case 0:
+                error = -EPERM;
+                if (onsigstack)
+                    goto out;
+                current->sas_ss_sp = 0;
+                current->sas_ss_size = 0;
+                current->sas_ss_flags = SS_DISABLE;
+                break;
+            case SS_ONSTACK:
+                /* re-enable previously disabled sas */
+                error = -EINVAL;
+                if (current->sas_ss_size == 0)
+                    goto out;
+                break;
+            default:
+                break;
+            }
+        } else if (ss_flags != SS_DISABLE) {
+            error = -EPERM;
+            if (onsigstack)
+                goto out;
              error = -ENOMEM;
              if (ss_size < MINSIGSTKSZ)
                  goto out;
+            current->sas_ss_sp = (unsigned long) ss_sp;
+            current->sas_ss_size = ss_size;
+            /* unfortunately POSIX forces us to treat 0
+             * as SS_ONSTACK here, and some legacy apps
+             * perhaps used that...
+             */
+            if (ss_flags == 0)
+                ss_flags = SS_ONSTACK;
          }

-        current->sas_ss_sp = (unsigned long) ss_sp;
-        current->sas_ss_size = ss_size;
+        if (ss_flags != 0)
+            current->sas_ss_flags = ss_flags;
      }

      error = 0;
@@ -3168,7 +3186,7 @@ int __save_altstack(stack_t __user *uss, unsigned 
long sp)
  {
      struct task_struct *t = current;
      return  __put_user((void __user *)t->sas_ss_sp, &uss->ss_sp) |
-        __put_user(sas_ss_flags(sp), &uss->ss_flags) |
+        __put_user(t->sas_ss_flags, &uss->ss_flags) |
          __put_user(t->sas_ss_size, &uss->ss_size);
  }

-- 
2.5.0

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 8:28 AM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>
> linux implements the sigaltstack() in a way that makes it impossible to
> use with swapcontext(). Per the man page, sigaltstack is allowed to return
> EPERM if the process is altering its sigaltstack while running on
> sigaltstack.
> This is likely needed to consistently return oss->ss_flags, that indicates
> whether the process is being on sigaltstack or not.
> Unfortunately, linux takes that permission to return EPERM too literally:
> it returns EPERM even if you don't want to change to another sigaltstack,
> but only want to temporarily disable sigaltstack with SS_DISABLE.
> You can't use swapcontext() without disabling sigaltstack first, or the
> stack will be re-used and overwritten by a subsequent signal.
>
> With this patch, disabling sigaltstack inside a signal handler became
> possible, and the swapcontext() can then be used safely. After switching
> back to the sighandler, the app can re-enable the sigatlstack.
> The oss->ss_flags will correctly indicate the current use of sigaltstack,
> even if it is temporarily disabled. Any attempt to modify the sigaltstack
> (rather than to disable or re-enable it) within the sighandler, will still
> be punished with EPERM as suggested by POSIX.

This seems considerably more complicated than my previous proposal to
add an SS_FORCE flag to say "I know what I'm doing.  Ignore POSIX and
let me change the sigaltstack configuration even if it's in use".
What's the advantage?

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 8:28 AM, Stas Sergeev <stsp@list.ru> wrote:
>
> linux implements the sigaltstack() in a way that makes it impossible to
> use with swapcontext(). Per the man page, sigaltstack is allowed to return
> EPERM if the process is altering its sigaltstack while running on
> sigaltstack.
> This is likely needed to consistently return oss->ss_flags, that indicates
> whether the process is being on sigaltstack or not.
> Unfortunately, linux takes that permission to return EPERM too literally:
> it returns EPERM even if you don't want to change to another sigaltstack,
> but only want to temporarily disable sigaltstack with SS_DISABLE.
> You can't use swapcontext() without disabling sigaltstack first, or the
> stack will be re-used and overwritten by a subsequent signal.
>
> With this patch, disabling sigaltstack inside a signal handler became
> possible, and the swapcontext() can then be used safely. After switching
> back to the sighandler, the app can re-enable the sigatlstack.
> The oss->ss_flags will correctly indicate the current use of sigaltstack,
> even if it is temporarily disabled. Any attempt to modify the sigaltstack
> (rather than to disable or re-enable it) within the sighandler, will still
> be punished with EPERM as suggested by POSIX.

This seems considerably more complicated than my previous proposal to
add an SS_FORCE flag to say "I know what I'm doing.  Ignore POSIX and
let me change the sigaltstack configuration even if it's in use".
What's the advantage?

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

31.01.2016 20:00, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 8:28 AM, Stas Sergeev <stsp@list.ru> wrote:
>> linux implements the sigaltstack() in a way that makes it impossible to
>> use with swapcontext(). Per the man page, sigaltstack is allowed to return
>> EPERM if the process is altering its sigaltstack while running on
>> sigaltstack.
>> This is likely needed to consistently return oss->ss_flags, that indicates
>> whether the process is being on sigaltstack or not.
>> Unfortunately, linux takes that permission to return EPERM too literally:
>> it returns EPERM even if you don't want to change to another sigaltstack,
>> but only want to temporarily disable sigaltstack with SS_DISABLE.
>> You can't use swapcontext() without disabling sigaltstack first, or the
>> stack will be re-used and overwritten by a subsequent signal.
>>
>> With this patch, disabling sigaltstack inside a signal handler became
>> possible, and the swapcontext() can then be used safely. After switching
>> back to the sighandler, the app can re-enable the sigatlstack.
>> The oss->ss_flags will correctly indicate the current use of sigaltstack,
>> even if it is temporarily disabled. Any attempt to modify the sigaltstack
>> (rather than to disable or re-enable it) within the sighandler, will still
>> be punished with EPERM as suggested by POSIX.
> This seems considerably more complicated than my previous proposal to
> add an SS_FORCE flag to say "I know what I'm doing.  Ignore POSIX and
> let me change the sigaltstack configuration even if it's in use".
> What's the advantage?
To me, the main advantage is to stay POSIX-compatible, rather
than adding the linux-specific flag. Please note that this flag does
not add any value other than to say "I want to ignore POSIX here"
in your interpretation of POSIX, and in my interpretation it doesn't
say even this, because in my interpretation the temporary disabling
is not prohibited. So if it doesn't even fit my interpretation, how would
I write a man description for it? I'll have to first clarify the vague
wording to clearly sound your way, and then add the flag to override
this. This whole procedure looks very illogical to me. So to find out
if it is just me, I'd like to hear from anyone else supporting the idea
of adding this flag. If people think its existence is justified, then fine.
But to me this flag is non-portable, while the both sigaltstack() and
swapcontext() are portable. So what will I gain with adding a
non-portable flag to my apps? A bunch of ifdefs?
IMHO as long as both swapcontext() and sigaltstack() are POSIX-compatible,
they should be compatible with each other in a POSIX-compatible
way. If POSIX needs the linux-specific flags to make them compatible
with each other, then POSIX is inconsistent. So lets just don't interpret
it the way that makes it so.

So in short:
Your concern is the patch complexity. Doing things your way will
however move the problem to the user: he will have to deal with the
linux-specific flags and add ifdefs for just a mere use of a 
posix-compatible
interfaces.

There can also be the subtle technical differences.
With your approach the nested signal can AFAIU overflow the
the disabled sigaltstack because you don't maintain the oss->ss_flags
in a consistent way. There is an overflow protection code:
---
/*
  * 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 (onsigstack && !likely(on_sig_stack(sp)))
          return (void __user *)-1L;
---
In your approach it will be bypassed.
And its not possible for an app to find out if it is running on a
sigaltstack now or not, after it is disabled.
Yes, maybe minor. But overall I see more disadvantages here
compared to the only disadvantage of my patch: its complexity.
But isn't the kernel developed to make user's life easier? :)
So to make it your way and start to add ifdef HAVE_SS_FORCE
to my apps, I'd like at least any other person to vote for your way.
Then fine.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 9:33 AM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
> 31.01.2016 20:00, Andy Lutomirski пишет:
>>
>> On Sun, Jan 31, 2016 at 8:28 AM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>>>
>>> linux implements the sigaltstack() in a way that makes it impossible to
>>> use with swapcontext(). Per the man page, sigaltstack is allowed to
>>> return
>>> EPERM if the process is altering its sigaltstack while running on
>>> sigaltstack.
>>> This is likely needed to consistently return oss->ss_flags, that
>>> indicates
>>> whether the process is being on sigaltstack or not.
>>> Unfortunately, linux takes that permission to return EPERM too literally:
>>> it returns EPERM even if you don't want to change to another sigaltstack,
>>> but only want to temporarily disable sigaltstack with SS_DISABLE.
>>> You can't use swapcontext() without disabling sigaltstack first, or the
>>> stack will be re-used and overwritten by a subsequent signal.
>>>
>>> With this patch, disabling sigaltstack inside a signal handler became
>>> possible, and the swapcontext() can then be used safely. After switching
>>> back to the sighandler, the app can re-enable the sigatlstack.
>>> The oss->ss_flags will correctly indicate the current use of sigaltstack,
>>> even if it is temporarily disabled. Any attempt to modify the sigaltstack
>>> (rather than to disable or re-enable it) within the sighandler, will
>>> still
>>> be punished with EPERM as suggested by POSIX.
>>
>> This seems considerably more complicated than my previous proposal to
>> add an SS_FORCE flag to say "I know what I'm doing.  Ignore POSIX and
>> let me change the sigaltstack configuration even if it's in use".
>> What's the advantage?
>
> To me, the main advantage is to stay POSIX-compatible, rather
> than adding the linux-specific flag. Please note that this flag does
> not add any value other than to say "I want to ignore POSIX here"
> in your interpretation of POSIX, and in my interpretation it doesn't
> say even this, because in my interpretation the temporary disabling
> is not prohibited.

POSIX says nothing about temporary anything.  It says:

       SS_ONSTACK  The process is currently executing on the alternate  signal
                   stack.  Attempts to modify the alternate signal stack while
                   the process is executing on it fail. This flag shall not be
                   modified by processes.

It's a bit ambiguous because "Attempts to modify the alternate signal
stack while the process is executing on it fail." is under SS_ONSTACK,
so it's not quite clear whether SS_DISABLE is *also* supposed to fail
if on the stack.


> So if it doesn't even fit my interpretation, how would
> I write a man description for it? I'll have to first clarify the vague
> wording to clearly sound your way, and then add the flag to override
> this. This whole procedure looks very illogical to me. So to find out
> if it is just me, I'd like to hear from anyone else supporting the idea
> of adding this flag. If people think its existence is justified, then fine.
> But to me this flag is non-portable, while the both sigaltstack() and
> swapcontext() are portable. So what will I gain with adding a
> non-portable flag to my apps? A bunch of ifdefs?
> IMHO as long as both swapcontext() and sigaltstack() are POSIX-compatible,
> they should be compatible with each other in a POSIX-compatible
> way. If POSIX needs the linux-specific flags to make them compatible
> with each other, then POSIX is inconsistent. So lets just don't interpret
> it the way that makes it so.

What do other operating systems do here?  You might be stuck with
Linux-specific code here no matter what.  If you're causing Linux to
match FreeBSD, that's a different store.

>
> So in short:
> Your concern is the patch complexity. Doing things your way will
> however move the problem to the user: he will have to deal with the
> linux-specific flags and add ifdefs for just a mere use of a
> posix-compatible
> interfaces.
>
> There can also be the subtle technical differences.
> With your approach the nested signal can AFAIU overflow the
> the disabled sigaltstack because you don't maintain the oss->ss_flags
> in a consistent way. There is an overflow protection code:
> ---
> /*
>  * 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 (onsigstack && !likely(on_sig_stack(sp)))
>          return (void __user *)-1L;
> ---
> In your approach it will be bypassed.
> And its not possible for an app to find out if it is running on a
> sigaltstack now or not, after it is disabled.

An app can figure out if it's on the altstack the same way the kernel
does.  In fact, the app needs to be quite careful with this temporary
disable thing.  If you temporarily disable sigaltstack, then
swapcontext, then you need to keep track of exactly when you're
supposed to re-enable it, which involves knowing what's going on with
the stacks.

Also, consider a use case like yours but with *two* contexts that use
their own altstack.  If you go to context A, enable sigaltstack, get a
signal, temporarily disable, then swapcontext to B, which tries to
re-enable its own sigaltstack, then everything gets confusing with
your patch, because, with your patch, the kernel is only tracking one
temporarily disabled sigaltstack.

So that's another argument in favor of my thought that there should
just be a way to override the permission checks to turn sigaltstack
all the way off or to reprogram it even if you're running on the
altstack.

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 9:33 AM, Stas Sergeev <stsp@list.ru> wrote:
> 31.01.2016 20:00, Andy Lutomirski пишет:
>>
>> On Sun, Jan 31, 2016 at 8:28 AM, Stas Sergeev <stsp@list.ru> wrote:
>>>
>>> linux implements the sigaltstack() in a way that makes it impossible to
>>> use with swapcontext(). Per the man page, sigaltstack is allowed to
>>> return
>>> EPERM if the process is altering its sigaltstack while running on
>>> sigaltstack.
>>> This is likely needed to consistently return oss->ss_flags, that
>>> indicates
>>> whether the process is being on sigaltstack or not.
>>> Unfortunately, linux takes that permission to return EPERM too literally:
>>> it returns EPERM even if you don't want to change to another sigaltstack,
>>> but only want to temporarily disable sigaltstack with SS_DISABLE.
>>> You can't use swapcontext() without disabling sigaltstack first, or the
>>> stack will be re-used and overwritten by a subsequent signal.
>>>
>>> With this patch, disabling sigaltstack inside a signal handler became
>>> possible, and the swapcontext() can then be used safely. After switching
>>> back to the sighandler, the app can re-enable the sigatlstack.
>>> The oss->ss_flags will correctly indicate the current use of sigaltstack,
>>> even if it is temporarily disabled. Any attempt to modify the sigaltstack
>>> (rather than to disable or re-enable it) within the sighandler, will
>>> still
>>> be punished with EPERM as suggested by POSIX.
>>
>> This seems considerably more complicated than my previous proposal to
>> add an SS_FORCE flag to say "I know what I'm doing.  Ignore POSIX and
>> let me change the sigaltstack configuration even if it's in use".
>> What's the advantage?
>
> To me, the main advantage is to stay POSIX-compatible, rather
> than adding the linux-specific flag. Please note that this flag does
> not add any value other than to say "I want to ignore POSIX here"
> in your interpretation of POSIX, and in my interpretation it doesn't
> say even this, because in my interpretation the temporary disabling
> is not prohibited.

POSIX says nothing about temporary anything.  It says:

       SS_ONSTACK  The process is currently executing on the alternate  signal
                   stack.  Attempts to modify the alternate signal stack while
                   the process is executing on it fail. This flag shall not be
                   modified by processes.

It's a bit ambiguous because "Attempts to modify the alternate signal
stack while the process is executing on it fail." is under SS_ONSTACK,
so it's not quite clear whether SS_DISABLE is *also* supposed to fail
if on the stack.


> So if it doesn't even fit my interpretation, how would
> I write a man description for it? I'll have to first clarify the vague
> wording to clearly sound your way, and then add the flag to override
> this. This whole procedure looks very illogical to me. So to find out
> if it is just me, I'd like to hear from anyone else supporting the idea
> of adding this flag. If people think its existence is justified, then fine.
> But to me this flag is non-portable, while the both sigaltstack() and
> swapcontext() are portable. So what will I gain with adding a
> non-portable flag to my apps? A bunch of ifdefs?
> IMHO as long as both swapcontext() and sigaltstack() are POSIX-compatible,
> they should be compatible with each other in a POSIX-compatible
> way. If POSIX needs the linux-specific flags to make them compatible
> with each other, then POSIX is inconsistent. So lets just don't interpret
> it the way that makes it so.

What do other operating systems do here?  You might be stuck with
Linux-specific code here no matter what.  If you're causing Linux to
match FreeBSD, that's a different store.

>
> So in short:
> Your concern is the patch complexity. Doing things your way will
> however move the problem to the user: he will have to deal with the
> linux-specific flags and add ifdefs for just a mere use of a
> posix-compatible
> interfaces.
>
> There can also be the subtle technical differences.
> With your approach the nested signal can AFAIU overflow the
> the disabled sigaltstack because you don't maintain the oss->ss_flags
> in a consistent way. There is an overflow protection code:
> ---
> /*
>  * 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 (onsigstack && !likely(on_sig_stack(sp)))
>          return (void __user *)-1L;
> ---
> In your approach it will be bypassed.
> And its not possible for an app to find out if it is running on a
> sigaltstack now or not, after it is disabled.

An app can figure out if it's on the altstack the same way the kernel
does.  In fact, the app needs to be quite careful with this temporary
disable thing.  If you temporarily disable sigaltstack, then
swapcontext, then you need to keep track of exactly when you're
supposed to re-enable it, which involves knowing what's going on with
the stacks.

Also, consider a use case like yours but with *two* contexts that use
their own altstack.  If you go to context A, enable sigaltstack, get a
signal, temporarily disable, then swapcontext to B, which tries to
re-enable its own sigaltstack, then everything gets confusing with
your patch, because, with your patch, the kernel is only tracking one
temporarily disabled sigaltstack.

So that's another argument in favor of my thought that there should
just be a way to override the permission checks to turn sigaltstack
all the way off or to reprogram it even if you're running on the
altstack.

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

31.01.2016 22:03, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 9:33 AM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>> 31.01.2016 20:00, Andy Lutomirski пишет:
>>> On Sun, Jan 31, 2016 at 8:28 AM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>>>> linux implements the sigaltstack() in a way that makes it impossible to
>>>> use with swapcontext(). Per the man page, sigaltstack is allowed to
>>>> return
>>>> EPERM if the process is altering its sigaltstack while running on
>>>> sigaltstack.
>>>> This is likely needed to consistently return oss->ss_flags, that
>>>> indicates
>>>> whether the process is being on sigaltstack or not.
>>>> Unfortunately, linux takes that permission to return EPERM too literally:
>>>> it returns EPERM even if you don't want to change to another sigaltstack,
>>>> but only want to temporarily disable sigaltstack with SS_DISABLE.
>>>> You can't use swapcontext() without disabling sigaltstack first, or the
>>>> stack will be re-used and overwritten by a subsequent signal.
>>>>
>>>> With this patch, disabling sigaltstack inside a signal handler became
>>>> possible, and the swapcontext() can then be used safely. After switching
>>>> back to the sighandler, the app can re-enable the sigatlstack.
>>>> The oss->ss_flags will correctly indicate the current use of sigaltstack,
>>>> even if it is temporarily disabled. Any attempt to modify the sigaltstack
>>>> (rather than to disable or re-enable it) within the sighandler, will
>>>> still
>>>> be punished with EPERM as suggested by POSIX.
>>> This seems considerably more complicated than my previous proposal to
>>> add an SS_FORCE flag to say "I know what I'm doing.  Ignore POSIX and
>>> let me change the sigaltstack configuration even if it's in use".
>>> What's the advantage?
>> To me, the main advantage is to stay POSIX-compatible, rather
>> than adding the linux-specific flag. Please note that this flag does
>> not add any value other than to say "I want to ignore POSIX here"
>> in your interpretation of POSIX, and in my interpretation it doesn't
>> say even this, because in my interpretation the temporary disabling
>> is not prohibited.
> POSIX says nothing about temporary anything.  It says:
>
>         SS_ONSTACK  The process is currently executing on the alternate  signal
>                     stack.  Attempts to modify the alternate signal stack while
>                     the process is executing on it fail. This flag shall not be
>                     modified by processes.
>
> It's a bit ambiguous because "Attempts to modify the alternate signal
> stack while the process is executing on it fail." is under SS_ONSTACK,
> so it's not quite clear whether SS_DISABLE is *also* supposed to fail
> if on the stack.
I think you are quoting the discription of the "oss" struct,
i.e. the one that is returned. It is correctly saying that when
SS_ONSTACK is _returned_, you can't modify the alternate
stack. So you likely confuse the above quote with the value
that is being set, not returned.
The ambiguity I am pointing to, is different. Above says you
can't _modify_ the sas. Modify likely means the modification
after which SS_ONSTACK will not be returned any more, i.e.
set to the different location. And I am not modifying it, and
the proof is that SS_ONSTACK is still returned as before.

>> So if it doesn't even fit my interpretation, how would
>> I write a man description for it? I'll have to first clarify the vague
>> wording to clearly sound your way, and then add the flag to override
>> this. This whole procedure looks very illogical to me. So to find out
>> if it is just me, I'd like to hear from anyone else supporting the idea
>> of adding this flag. If people think its existence is justified, then fine.
>> But to me this flag is non-portable, while the both sigaltstack() and
>> swapcontext() are portable. So what will I gain with adding a
>> non-portable flag to my apps? A bunch of ifdefs?
>> IMHO as long as both swapcontext() and sigaltstack() are POSIX-compatible,
>> they should be compatible with each other in a POSIX-compatible
>> way. If POSIX needs the linux-specific flags to make them compatible
>> with each other, then POSIX is inconsistent. So lets just don't interpret
>> it the way that makes it so.
> What do other operating systems do here?  You might be stuck with
> Linux-specific code here no matter what.  If you're causing Linux to
> match FreeBSD, that's a different store.
Likely not.
But the most intuitive way for the programmer is to just
use SS_DISABLE so I wonder if some OSes allow that.
FreeBSD seems not quite the case though:
http://www.hpdc.syr.edu/~chapin/cis657/FreeBSD_5.2.1_Doxygen/kern__sig_8c-source.html
So I don't know how they solve the swapcontext() problem.
Perhaps I should ask them.

>> So in short:
>> Your concern is the patch complexity. Doing things your way will
>> however move the problem to the user: he will have to deal with the
>> linux-specific flags and add ifdefs for just a mere use of a
>> posix-compatible
>> interfaces.
>>
>> There can also be the subtle technical differences.
>> With your approach the nested signal can AFAIU overflow the
>> the disabled sigaltstack because you don't maintain the oss->ss_flags
>> in a consistent way. There is an overflow protection code:
>> ---
>> /*
>>   * 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 (onsigstack && !likely(on_sig_stack(sp)))
>>           return (void __user *)-1L;
>> ---
>> In your approach it will be bypassed.
>> And its not possible for an app to find out if it is running on a
>> sigaltstack now or not, after it is disabled.
> An app can figure out if it's on the altstack the same way the kernel
> does.
With inline asm or register variable, not good for portability.
And only if it knows at the particular place to where was the
alt stack set before.
And if it to ask the kernel via sigaltstack(NULL, &oss), then with your
approach the kernel's reply will contradict with what an app have.

>    In fact, the app needs to be quite careful with this temporary
> disable thing.  If you temporarily disable sigaltstack, then
> swapcontext, then you need to keep track of exactly when you're
> supposed to re-enable it, which involves knowing what's going on with
> the stacks.
My test-case code simply does this in sighandler:

sigaltstack(SS_DISABLE);
swapcontext();
sigaltstack(SS_ENABLE);

which means that only the sighandler context should
manage the sigaltstack, then you are safe. Non-signal
context should never care.

> Also, consider a use case like yours but with *two* contexts that use
> their own altstack.  If you go to context A, enable sigaltstack, get a
> signal, temporarily disable, then swapcontext to B, which tries to
> re-enable its own sigaltstack, then everything gets confusing with
> your patch, because, with your patch, the kernel is only tracking one
> temporarily disabled sigaltstack.
Of course the good practice is to set the sigaltstack
before creating the contexts. Then the above scenario
should involve switching between 2 signal handlers to get
into troubles. I think the scenario with switching between
2 signal handlers is very-very unrealistic.

> So that's another argument in favor of my thought that there should
> just be a way to override the permission checks to turn sigaltstack
> all the way off or to reprogram it even if you're running on the
> altstack.
Ok so if you block this approach, I wonder how many people
will block the SS_FORCE approach, esp given that the approach
without any new flags was already released and seen.
Of course if no one else cares, I'll have to do that sooner or later.
Btw, you can do that too; you can even re-use my test case to
save time. :)

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

31.01.2016 22:03, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 9:33 AM, Stas Sergeev <stsp@list.ru> wrote:
>> 31.01.2016 20:00, Andy Lutomirski пишет:
>>> On Sun, Jan 31, 2016 at 8:28 AM, Stas Sergeev <stsp@list.ru> wrote:
>>>> linux implements the sigaltstack() in a way that makes it impossible to
>>>> use with swapcontext(). Per the man page, sigaltstack is allowed to
>>>> return
>>>> EPERM if the process is altering its sigaltstack while running on
>>>> sigaltstack.
>>>> This is likely needed to consistently return oss->ss_flags, that
>>>> indicates
>>>> whether the process is being on sigaltstack or not.
>>>> Unfortunately, linux takes that permission to return EPERM too literally:
>>>> it returns EPERM even if you don't want to change to another sigaltstack,
>>>> but only want to temporarily disable sigaltstack with SS_DISABLE.
>>>> You can't use swapcontext() without disabling sigaltstack first, or the
>>>> stack will be re-used and overwritten by a subsequent signal.
>>>>
>>>> With this patch, disabling sigaltstack inside a signal handler became
>>>> possible, and the swapcontext() can then be used safely. After switching
>>>> back to the sighandler, the app can re-enable the sigatlstack.
>>>> The oss->ss_flags will correctly indicate the current use of sigaltstack,
>>>> even if it is temporarily disabled. Any attempt to modify the sigaltstack
>>>> (rather than to disable or re-enable it) within the sighandler, will
>>>> still
>>>> be punished with EPERM as suggested by POSIX.
>>> This seems considerably more complicated than my previous proposal to
>>> add an SS_FORCE flag to say "I know what I'm doing.  Ignore POSIX and
>>> let me change the sigaltstack configuration even if it's in use".
>>> What's the advantage?
>> To me, the main advantage is to stay POSIX-compatible, rather
>> than adding the linux-specific flag. Please note that this flag does
>> not add any value other than to say "I want to ignore POSIX here"
>> in your interpretation of POSIX, and in my interpretation it doesn't
>> say even this, because in my interpretation the temporary disabling
>> is not prohibited.
> POSIX says nothing about temporary anything.  It says:
>
>         SS_ONSTACK  The process is currently executing on the alternate  signal
>                     stack.  Attempts to modify the alternate signal stack while
>                     the process is executing on it fail. This flag shall not be
>                     modified by processes.
>
> It's a bit ambiguous because "Attempts to modify the alternate signal
> stack while the process is executing on it fail." is under SS_ONSTACK,
> so it's not quite clear whether SS_DISABLE is *also* supposed to fail
> if on the stack.
I think you are quoting the discription of the "oss" struct,
i.e. the one that is returned. It is correctly saying that when
SS_ONSTACK is _returned_, you can't modify the alternate
stack. So you likely confuse the above quote with the value
that is being set, not returned.
The ambiguity I am pointing to, is different. Above says you
can't _modify_ the sas. Modify likely means the modification
after which SS_ONSTACK will not be returned any more, i.e.
set to the different location. And I am not modifying it, and
the proof is that SS_ONSTACK is still returned as before.

>> So if it doesn't even fit my interpretation, how would
>> I write a man description for it? I'll have to first clarify the vague
>> wording to clearly sound your way, and then add the flag to override
>> this. This whole procedure looks very illogical to me. So to find out
>> if it is just me, I'd like to hear from anyone else supporting the idea
>> of adding this flag. If people think its existence is justified, then fine.
>> But to me this flag is non-portable, while the both sigaltstack() and
>> swapcontext() are portable. So what will I gain with adding a
>> non-portable flag to my apps? A bunch of ifdefs?
>> IMHO as long as both swapcontext() and sigaltstack() are POSIX-compatible,
>> they should be compatible with each other in a POSIX-compatible
>> way. If POSIX needs the linux-specific flags to make them compatible
>> with each other, then POSIX is inconsistent. So lets just don't interpret
>> it the way that makes it so.
> What do other operating systems do here?  You might be stuck with
> Linux-specific code here no matter what.  If you're causing Linux to
> match FreeBSD, that's a different store.
Likely not.
But the most intuitive way for the programmer is to just
use SS_DISABLE so I wonder if some OSes allow that.
FreeBSD seems not quite the case though:
http://www.hpdc.syr.edu/~chapin/cis657/FreeBSD_5.2.1_Doxygen/kern__sig_8c-source.html
So I don't know how they solve the swapcontext() problem.
Perhaps I should ask them.

>> So in short:
>> Your concern is the patch complexity. Doing things your way will
>> however move the problem to the user: he will have to deal with the
>> linux-specific flags and add ifdefs for just a mere use of a
>> posix-compatible
>> interfaces.
>>
>> There can also be the subtle technical differences.
>> With your approach the nested signal can AFAIU overflow the
>> the disabled sigaltstack because you don't maintain the oss->ss_flags
>> in a consistent way. There is an overflow protection code:
>> ---
>> /*
>>   * 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 (onsigstack && !likely(on_sig_stack(sp)))
>>           return (void __user *)-1L;
>> ---
>> In your approach it will be bypassed.
>> And its not possible for an app to find out if it is running on a
>> sigaltstack now or not, after it is disabled.
> An app can figure out if it's on the altstack the same way the kernel
> does.
With inline asm or register variable, not good for portability.
And only if it knows at the particular place to where was the
alt stack set before.
And if it to ask the kernel via sigaltstack(NULL, &oss), then with your
approach the kernel's reply will contradict with what an app have.

>    In fact, the app needs to be quite careful with this temporary
> disable thing.  If you temporarily disable sigaltstack, then
> swapcontext, then you need to keep track of exactly when you're
> supposed to re-enable it, which involves knowing what's going on with
> the stacks.
My test-case code simply does this in sighandler:

sigaltstack(SS_DISABLE);
swapcontext();
sigaltstack(SS_ENABLE);

which means that only the sighandler context should
manage the sigaltstack, then you are safe. Non-signal
context should never care.

> Also, consider a use case like yours but with *two* contexts that use
> their own altstack.  If you go to context A, enable sigaltstack, get a
> signal, temporarily disable, then swapcontext to B, which tries to
> re-enable its own sigaltstack, then everything gets confusing with
> your patch, because, with your patch, the kernel is only tracking one
> temporarily disabled sigaltstack.
Of course the good practice is to set the sigaltstack
before creating the contexts. Then the above scenario
should involve switching between 2 signal handlers to get
into troubles. I think the scenario with switching between
2 signal handlers is very-very unrealistic.

> So that's another argument in favor of my thought that there should
> just be a way to override the permission checks to turn sigaltstack
> all the way off or to reprogram it even if you're running on the
> altstack.
Ok so if you block this approach, I wonder how many people
will block the SS_FORCE approach, esp given that the approach
without any new flags was already released and seen.
Of course if no one else cares, I'll have to do that sooner or later.
Btw, you can do that too; you can even re-use my test case to
save time. :)

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
> 31.01.2016 22:03, Andy Lutomirski пишет:
>> Also, consider a use case like yours but with *two* contexts that use
>> their own altstack.  If you go to context A, enable sigaltstack, get a
>> signal, temporarily disable, then swapcontext to B, which tries to
>> re-enable its own sigaltstack, then everything gets confusing with
>> your patch, because, with your patch, the kernel is only tracking one
>> temporarily disabled sigaltstack.
>
> Of course the good practice is to set the sigaltstack
> before creating the contexts. Then the above scenario
> should involve switching between 2 signal handlers to get
> into troubles. I think the scenario with switching between
> 2 signal handlers is very-very unrealistic.

Why is it so unrealistic?  You're already using swapcontext, which
means you're doing something like userspace threads (although I
imagine that one of your thread-like things is DOS, but still), and,
to me, that suggests that the kernel interface should be agnostic as
to how many thread-like thinks are alive.

With your patch, where the kernel remembers that you have a
temporarily disabled altstack, you can't swap out your context on one
kernel thread and swap it in on another, and you can't have two
different contexts that get used on the same thread.

ISTM it would be simpler if you did:

sigaltstack(disable, force)
swapcontext() to context using sigaltstack
sigaltstack(set new altstack)

and then later

sigaltstack(disable, force)  /* just in case.  save old state, too. */
swapcontext() to context not using sigaltstack
sigaltstack(set new altstack)

If it would be POSIX compliant to allow SS_DISABLE to work even if on
the altstack even without a new flag (which is what you're
suggesting), then getting rid of the temporary in-kernel state would
considerably simplify this patch series.  Just skip the -EPERM check
in the disable path.

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp@list.ru> wrote:
> 31.01.2016 22:03, Andy Lutomirski пишет:
>> Also, consider a use case like yours but with *two* contexts that use
>> their own altstack.  If you go to context A, enable sigaltstack, get a
>> signal, temporarily disable, then swapcontext to B, which tries to
>> re-enable its own sigaltstack, then everything gets confusing with
>> your patch, because, with your patch, the kernel is only tracking one
>> temporarily disabled sigaltstack.
>
> Of course the good practice is to set the sigaltstack
> before creating the contexts. Then the above scenario
> should involve switching between 2 signal handlers to get
> into troubles. I think the scenario with switching between
> 2 signal handlers is very-very unrealistic.

Why is it so unrealistic?  You're already using swapcontext, which
means you're doing something like userspace threads (although I
imagine that one of your thread-like things is DOS, but still), and,
to me, that suggests that the kernel interface should be agnostic as
to how many thread-like thinks are alive.

With your patch, where the kernel remembers that you have a
temporarily disabled altstack, you can't swap out your context on one
kernel thread and swap it in on another, and you can't have two
different contexts that get used on the same thread.

ISTM it would be simpler if you did:

sigaltstack(disable, force)
swapcontext() to context using sigaltstack
sigaltstack(set new altstack)

and then later

sigaltstack(disable, force)  /* just in case.  save old state, too. */
swapcontext() to context not using sigaltstack
sigaltstack(set new altstack)

If it would be POSIX compliant to allow SS_DISABLE to work even if on
the altstack even without a new flag (which is what you're
suggesting), then getting rid of the temporary in-kernel state would
considerably simplify this patch series.  Just skip the -EPERM check
in the disable path.

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

31.01.2016 23:11, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>> 31.01.2016 22:03, Andy Lutomirski пишет:
>>> Also, consider a use case like yours but with *two* contexts that use
>>> their own altstack.  If you go to context A, enable sigaltstack, get a
>>> signal, temporarily disable, then swapcontext to B, which tries to
>>> re-enable its own sigaltstack, then everything gets confusing with
>>> your patch, because, with your patch, the kernel is only tracking one
>>> temporarily disabled sigaltstack.
>> Of course the good practice is to set the sigaltstack
>> before creating the contexts. Then the above scenario
>> should involve switching between 2 signal handlers to get
>> into troubles. I think the scenario with switching between
>> 2 signal handlers is very-very unrealistic.
> Why is it so unrealistic?  You're already using swapcontext, which
> means you're doing something like userspace threads (although I
> imagine that one of your thread-like things is DOS, but still), and,
> to me, that suggests that the kernel interface should be agnostic as
> to how many thread-like thinks are alive.
But you only get into troubles when you switch between 2
_active signal handlers_, rather than between 2 normal contexts,
or between 2 normal context and 1 sighandler.
So I am probably misunderstanding the scenario you describe.
Without 2 sighandlers that are active at the same time and you
switch between them, how would you get into troubles?
You say "then swapcontext to B, which tries to re-enable its own 
sigaltstack"
but there can be only one sigaltstack per thread, so I am quite
sure by re-enabling "its own sigaltstack" it will still do the right
thing.

> With your patch, where the kernel remembers that you have a
> temporarily disabled altstack, you can't swap out your context on one
> kernel thread and swap it in on another,
I can if I always set up a new stack rather than re-enable, right?
But yes, this is a problem that counts.

>   and you can't have two
> different contexts that get used on the same thread.
I don't think this is the problem because only the signal handler
should re-enable the sigaltstack, and I don't think we really should
switch between 2 active signal handlers. And even if we did, there
can be only one sigaltstack per thread, so it will re-enable always
the right stack (there is only one).

> ISTM it would be simpler if you did:
>
> sigaltstack(disable, force)
> swapcontext() to context using sigaltstack
> sigaltstack(set new altstack)
>
> and then later
>
> sigaltstack(disable, force)  /* just in case.  save old state, too. */
> swapcontext() to context not using sigaltstack
> sigaltstack(set new altstack)
In the real world you don't even need sigaltstack(set new altstack)
because uc_stack does this for you on rt_sigreturn. It is only my
test-case that does so.

> If it would be POSIX compliant to allow SS_DISABLE to work even if on
> the altstack even without a new flag (which is what you're
> suggesting), then getting rid of the temporary in-kernel state would
> considerably simplify this patch series.  Just skip the -EPERM check
> in the disable path.
Yes, that's why I was suggesting to just remove the EPERM
check initially. We can still do exactly that. The only problem I
can see with removing EPERM is that it would be hard to emulate
the old behaviour if need be. For example if glibc want to return
EPERM behaviour, it will have problems doing so because oss->ss_flags
doesn't say if we are on a sigaltstack and there is no other way
to find out.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

31.01.2016 23:11, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp@list.ru> wrote:
>> 31.01.2016 22:03, Andy Lutomirski пишет:
>>> Also, consider a use case like yours but with *two* contexts that use
>>> their own altstack.  If you go to context A, enable sigaltstack, get a
>>> signal, temporarily disable, then swapcontext to B, which tries to
>>> re-enable its own sigaltstack, then everything gets confusing with
>>> your patch, because, with your patch, the kernel is only tracking one
>>> temporarily disabled sigaltstack.
>> Of course the good practice is to set the sigaltstack
>> before creating the contexts. Then the above scenario
>> should involve switching between 2 signal handlers to get
>> into troubles. I think the scenario with switching between
>> 2 signal handlers is very-very unrealistic.
> Why is it so unrealistic?  You're already using swapcontext, which
> means you're doing something like userspace threads (although I
> imagine that one of your thread-like things is DOS, but still), and,
> to me, that suggests that the kernel interface should be agnostic as
> to how many thread-like thinks are alive.
But you only get into troubles when you switch between 2
_active signal handlers_, rather than between 2 normal contexts,
or between 2 normal context and 1 sighandler.
So I am probably misunderstanding the scenario you describe.
Without 2 sighandlers that are active at the same time and you
switch between them, how would you get into troubles?
You say "then swapcontext to B, which tries to re-enable its own 
sigaltstack"
but there can be only one sigaltstack per thread, so I am quite
sure by re-enabling "its own sigaltstack" it will still do the right
thing.

> With your patch, where the kernel remembers that you have a
> temporarily disabled altstack, you can't swap out your context on one
> kernel thread and swap it in on another,
I can if I always set up a new stack rather than re-enable, right?
But yes, this is a problem that counts.

>   and you can't have two
> different contexts that get used on the same thread.
I don't think this is the problem because only the signal handler
should re-enable the sigaltstack, and I don't think we really should
switch between 2 active signal handlers. And even if we did, there
can be only one sigaltstack per thread, so it will re-enable always
the right stack (there is only one).

> ISTM it would be simpler if you did:
>
> sigaltstack(disable, force)
> swapcontext() to context using sigaltstack
> sigaltstack(set new altstack)
>
> and then later
>
> sigaltstack(disable, force)  /* just in case.  save old state, too. */
> swapcontext() to context not using sigaltstack
> sigaltstack(set new altstack)
In the real world you don't even need sigaltstack(set new altstack)
because uc_stack does this for you on rt_sigreturn. It is only my
test-case that does so.

> If it would be POSIX compliant to allow SS_DISABLE to work even if on
> the altstack even without a new flag (which is what you're
> suggesting), then getting rid of the temporary in-kernel state would
> considerably simplify this patch series.  Just skip the -EPERM check
> in the disable path.
Yes, that's why I was suggesting to just remove the EPERM
check initially. We can still do exactly that. The only problem I
can see with removing EPERM is that it would be hard to emulate
the old behaviour if need be. For example if glibc want to return
EPERM behaviour, it will have problems doing so because oss->ss_flags
doesn't say if we are on a sigaltstack and there is no other way
to find out.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 2:36 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
> 31.01.2016 23:11, Andy Lutomirski пишет:
>>
>> On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>>>
>>> 31.01.2016 22:03, Andy Lutomirski пишет:
>>>>
>>>> Also, consider a use case like yours but with *two* contexts that use
>>>> their own altstack.  If you go to context A, enable sigaltstack, get a
>>>> signal, temporarily disable, then swapcontext to B, which tries to
>>>> re-enable its own sigaltstack, then everything gets confusing with
>>>> your patch, because, with your patch, the kernel is only tracking one
>>>> temporarily disabled sigaltstack.
>>>
>>> Of course the good practice is to set the sigaltstack
>>> before creating the contexts. Then the above scenario
>>> should involve switching between 2 signal handlers to get
>>> into troubles. I think the scenario with switching between
>>> 2 signal handlers is very-very unrealistic.
>>
>> Why is it so unrealistic?  You're already using swapcontext, which
>> means you're doing something like userspace threads (although I
>> imagine that one of your thread-like things is DOS, but still), and,
>> to me, that suggests that the kernel interface should be agnostic as
>> to how many thread-like thinks are alive.
>
> But you only get into troubles when you switch between 2
> _active signal handlers_, rather than between 2 normal contexts,
> or between 2 normal context and 1 sighandler.
> So I am probably misunderstanding the scenario you describe.
> Without 2 sighandlers that are active at the same time and you
> switch between them, how would you get into troubles?
> You say "then swapcontext to B, which tries to re-enable its own
> sigaltstack"
> but there can be only one sigaltstack per thread, so I am quite
> sure by re-enabling "its own sigaltstack" it will still do the right
> thing.

As long as the kernel has a concept of a programmed but disabled
sigaltstack, something is confused when there is more than one
user-created inactive sigaltstack.  So I don't see why you want the
kernel to remember about disabled altstacks at all.

> I don't think this is the problem because only the signal handler
> should re-enable the sigaltstack, and I don't think we really should
> switch between 2 active signal handlers. And even if we did, there
> can be only one sigaltstack per thread, so it will re-enable always
> the right stack (there is only one).

Why would there only be one per thread?

>
>> ISTM it would be simpler if you did:
>>
>> sigaltstack(disable, force)
>> swapcontext() to context using sigaltstack
>> sigaltstack(set new altstack)
>>
>> and then later
>>
>> sigaltstack(disable, force)  /* just in case.  save old state, too. */
>> swapcontext() to context not using sigaltstack
>> sigaltstack(set new altstack)
>
> In the real world you don't even need sigaltstack(set new altstack)
> because uc_stack does this for you on rt_sigreturn. It is only my
> test-case that does so.

That's only the case if swapcontext is implemented using rt_sigreturn.  Is it?

>
>> If it would be POSIX compliant to allow SS_DISABLE to work even if on
>> the altstack even without a new flag (which is what you're
>> suggesting), then getting rid of the temporary in-kernel state would
>> considerably simplify this patch series.  Just skip the -EPERM check
>> in the disable path.
>
> Yes, that's why I was suggesting to just remove the EPERM
> check initially. We can still do exactly that. The only problem I
> can see with removing EPERM is that it would be hard to emulate
> the old behaviour if need be. For example if glibc want to return
> EPERM behaviour, it will have problems doing so because oss->ss_flags
> doesn't say if we are on a sigaltstack and there is no other way
> to find out.

...which is why I suggested SS_FORCE in the first place.

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 2:36 PM, Stas Sergeev <stsp@list.ru> wrote:
> 31.01.2016 23:11, Andy Lutomirski пишет:
>>
>> On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp@list.ru> wrote:
>>>
>>> 31.01.2016 22:03, Andy Lutomirski пишет:
>>>>
>>>> Also, consider a use case like yours but with *two* contexts that use
>>>> their own altstack.  If you go to context A, enable sigaltstack, get a
>>>> signal, temporarily disable, then swapcontext to B, which tries to
>>>> re-enable its own sigaltstack, then everything gets confusing with
>>>> your patch, because, with your patch, the kernel is only tracking one
>>>> temporarily disabled sigaltstack.
>>>
>>> Of course the good practice is to set the sigaltstack
>>> before creating the contexts. Then the above scenario
>>> should involve switching between 2 signal handlers to get
>>> into troubles. I think the scenario with switching between
>>> 2 signal handlers is very-very unrealistic.
>>
>> Why is it so unrealistic?  You're already using swapcontext, which
>> means you're doing something like userspace threads (although I
>> imagine that one of your thread-like things is DOS, but still), and,
>> to me, that suggests that the kernel interface should be agnostic as
>> to how many thread-like thinks are alive.
>
> But you only get into troubles when you switch between 2
> _active signal handlers_, rather than between 2 normal contexts,
> or between 2 normal context and 1 sighandler.
> So I am probably misunderstanding the scenario you describe.
> Without 2 sighandlers that are active at the same time and you
> switch between them, how would you get into troubles?
> You say "then swapcontext to B, which tries to re-enable its own
> sigaltstack"
> but there can be only one sigaltstack per thread, so I am quite
> sure by re-enabling "its own sigaltstack" it will still do the right
> thing.

As long as the kernel has a concept of a programmed but disabled
sigaltstack, something is confused when there is more than one
user-created inactive sigaltstack.  So I don't see why you want the
kernel to remember about disabled altstacks at all.

> I don't think this is the problem because only the signal handler
> should re-enable the sigaltstack, and I don't think we really should
> switch between 2 active signal handlers. And even if we did, there
> can be only one sigaltstack per thread, so it will re-enable always
> the right stack (there is only one).

Why would there only be one per thread?

>
>> ISTM it would be simpler if you did:
>>
>> sigaltstack(disable, force)
>> swapcontext() to context using sigaltstack
>> sigaltstack(set new altstack)
>>
>> and then later
>>
>> sigaltstack(disable, force)  /* just in case.  save old state, too. */
>> swapcontext() to context not using sigaltstack
>> sigaltstack(set new altstack)
>
> In the real world you don't even need sigaltstack(set new altstack)
> because uc_stack does this for you on rt_sigreturn. It is only my
> test-case that does so.

That's only the case if swapcontext is implemented using rt_sigreturn.  Is it?

>
>> If it would be POSIX compliant to allow SS_DISABLE to work even if on
>> the altstack even without a new flag (which is what you're
>> suggesting), then getting rid of the temporary in-kernel state would
>> considerably simplify this patch series.  Just skip the -EPERM check
>> in the disable path.
>
> Yes, that's why I was suggesting to just remove the EPERM
> check initially. We can still do exactly that. The only problem I
> can see with removing EPERM is that it would be hard to emulate
> the old behaviour if need be. For example if glibc want to return
> EPERM behaviour, it will have problems doing so because oss->ss_flags
> doesn't say if we are on a sigaltstack and there is no other way
> to find out.

...which is why I suggested SS_FORCE in the first place.

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 01:44, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 2:36 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>> 31.01.2016 23:11, Andy Lutomirski пишет:
>>> On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>>>> 31.01.2016 22:03, Andy Lutomirski пишет:
>>>>> Also, consider a use case like yours but with *two* contexts that use
>>>>> their own altstack.  If you go to context A, enable sigaltstack, get a
>>>>> signal, temporarily disable, then swapcontext to B, which tries to
>>>>> re-enable its own sigaltstack, then everything gets confusing with
>>>>> your patch, because, with your patch, the kernel is only tracking one
>>>>> temporarily disabled sigaltstack.
>>>> Of course the good practice is to set the sigaltstack
>>>> before creating the contexts. Then the above scenario
>>>> should involve switching between 2 signal handlers to get
>>>> into troubles. I think the scenario with switching between
>>>> 2 signal handlers is very-very unrealistic.
>>> Why is it so unrealistic?  You're already using swapcontext, which
>>> means you're doing something like userspace threads (although I
>>> imagine that one of your thread-like things is DOS, but still), and,
>>> to me, that suggests that the kernel interface should be agnostic as
>>> to how many thread-like thinks are alive.
>> But you only get into troubles when you switch between 2
>> _active signal handlers_, rather than between 2 normal contexts,
>> or between 2 normal context and 1 sighandler.
>> So I am probably misunderstanding the scenario you describe.
>> Without 2 sighandlers that are active at the same time and you
>> switch between them, how would you get into troubles?
>> You say "then swapcontext to B, which tries to re-enable its own
>> sigaltstack"
>> but there can be only one sigaltstack per thread, so I am quite
>> sure by re-enabling "its own sigaltstack" it will still do the right
>> thing.
> As long as the kernel has a concept of a programmed but disabled
> sigaltstack, something is confused when there is more than one
> user-created inactive sigaltstack.
I simply don't understand how can we have more than one
sigaltstack per thread. Is this supported? If not then I don't
expect the different non-sighandler user contexts trying to
set up the different ones. So you are probably talking about
2 sighandlers switching between each other, right? And that
case is likely broken anyway.

>    So I don't see why you want the
> kernel to remember about disabled altstacks at all.
2 reasons:
- Language-lawyering around POSIX
- Consistently return oss->ss_flags when we are on a signal stack
Restoring the old sas is not among the goals, but allowing the
sighandler to freely install the new sas (as you suggest) is a clear
contradiction to POSIX. So that's why you propose SS_FORCE, yes,
but then the question: will _anyone_ use sigaltstack(SS_ONSTACK)
in a sighandler without SS_FORCE? And the answer is likely "no".
In which case your SS_FORCE erodes to "I run it from sighandler"
but this info the kernel already knows.

>> I don't think this is the problem because only the signal handler
>> should re-enable the sigaltstack, and I don't think we really should
>> switch between 2 active signal handlers. And even if we did, there
>> can be only one sigaltstack per thread, so it will re-enable always
>> the right stack (there is only one).
> Why would there only be one per thread?
If you mean every sighandler installs its own, then I think
switching between such sighandlers is broken anyway.
If you mean the non-sighandler contexts should install multiple
sigaltstacks, then I don't think this is supported or can work.

>>> ISTM it would be simpler if you did:
>>>
>>> sigaltstack(disable, force)
>>> swapcontext() to context using sigaltstack
>>> sigaltstack(set new altstack)
>>>
>>> and then later
>>>
>>> sigaltstack(disable, force)  /* just in case.  save old state, too. */
>>> swapcontext() to context not using sigaltstack
>>> sigaltstack(set new altstack)
>> In the real world you don't even need sigaltstack(set new altstack)
>> because uc_stack does this for you on rt_sigreturn. It is only my
>> test-case that does so.
> That's only the case if swapcontext is implemented using rt_sigreturn.  Is it?
No, its when you use SA_SIGINFO with sigaction().
Then the sigaltstack will magically restore itself once you
leave the sighandler. That's why I wouldn't suggest to ever
modify the sas inside the sighandler the way you propose:
it will simply not work, uc_stack will set it back. My re-enable
trick is at least in agreement with uc_stack.

>>> If it would be POSIX compliant to allow SS_DISABLE to work even if on
>>> the altstack even without a new flag (which is what you're
>>> suggesting), then getting rid of the temporary in-kernel state would
>>> considerably simplify this patch series.  Just skip the -EPERM check
>>> in the disable path.
>> Yes, that's why I was suggesting to just remove the EPERM
>> check initially. We can still do exactly that. The only problem I
>> can see with removing EPERM is that it would be hard to emulate
>> the old behaviour if need be. For example if glibc want to return
>> EPERM behaviour, it will have problems doing so because oss->ss_flags
>> doesn't say if we are on a sigaltstack and there is no other way
>> to find out.
> ...which is why I suggested SS_FORCE in the first place.
I understand. With SS_FORCE there is no temptation to emulate
the old behaviour, so there may be a fewer need to look into
oss->sa_flags even when you return it an inconsistent ways.

We probably should make a summary of our findings or they
will be forgotten.
So far I was pointing to a couple of minor problems with SS_FORCE:
- bypasses overflow protection
- prevents from asking the kernel if we are on sigaltstack or not
... and a few that I consider more important:
- does not bring any value other than to say "I am calling from sighandler"
- allows the programmer to freely modify sas while later it will
still be reset with uc_stack
(and I've likely forgot some already)

There are the upsides compared to just removing EPERM:
- fewer need to look into oss->sa_flags, so its inconsistency
became forgivable
(have I missed something else? please fill in)

There are the upsides compared to remembering the ss_flags:
- simpler code
- slightly better semantic wrt kernel threads (with my approach
restoring the context on a different kernel thread, will require
setting a separate sas there instead of restoring... but I am not
sure this is a considerably bad semantic because whoever messes
with kernel threads this way, should know how to propoerly
set sigaltstacks)

So from that summary I can agree that SS_FORCE may from
some POV be better than simply removing EPERM, as they both
share the downsides, with SS_FORCE providing minor advantages.
But I still don't see it beating my new approach.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 01:44, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 2:36 PM, Stas Sergeev <stsp@list.ru> wrote:
>> 31.01.2016 23:11, Andy Lutomirski пишет:
>>> On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp@list.ru> wrote:
>>>> 31.01.2016 22:03, Andy Lutomirski пишет:
>>>>> Also, consider a use case like yours but with *two* contexts that use
>>>>> their own altstack.  If you go to context A, enable sigaltstack, get a
>>>>> signal, temporarily disable, then swapcontext to B, which tries to
>>>>> re-enable its own sigaltstack, then everything gets confusing with
>>>>> your patch, because, with your patch, the kernel is only tracking one
>>>>> temporarily disabled sigaltstack.
>>>> Of course the good practice is to set the sigaltstack
>>>> before creating the contexts. Then the above scenario
>>>> should involve switching between 2 signal handlers to get
>>>> into troubles. I think the scenario with switching between
>>>> 2 signal handlers is very-very unrealistic.
>>> Why is it so unrealistic?  You're already using swapcontext, which
>>> means you're doing something like userspace threads (although I
>>> imagine that one of your thread-like things is DOS, but still), and,
>>> to me, that suggests that the kernel interface should be agnostic as
>>> to how many thread-like thinks are alive.
>> But you only get into troubles when you switch between 2
>> _active signal handlers_, rather than between 2 normal contexts,
>> or between 2 normal context and 1 sighandler.
>> So I am probably misunderstanding the scenario you describe.
>> Without 2 sighandlers that are active at the same time and you
>> switch between them, how would you get into troubles?
>> You say "then swapcontext to B, which tries to re-enable its own
>> sigaltstack"
>> but there can be only one sigaltstack per thread, so I am quite
>> sure by re-enabling "its own sigaltstack" it will still do the right
>> thing.
> As long as the kernel has a concept of a programmed but disabled
> sigaltstack, something is confused when there is more than one
> user-created inactive sigaltstack.
I simply don't understand how can we have more than one
sigaltstack per thread. Is this supported? If not then I don't
expect the different non-sighandler user contexts trying to
set up the different ones. So you are probably talking about
2 sighandlers switching between each other, right? And that
case is likely broken anyway.

>    So I don't see why you want the
> kernel to remember about disabled altstacks at all.
2 reasons:
- Language-lawyering around POSIX
- Consistently return oss->ss_flags when we are on a signal stack
Restoring the old sas is not among the goals, but allowing the
sighandler to freely install the new sas (as you suggest) is a clear
contradiction to POSIX. So that's why you propose SS_FORCE, yes,
but then the question: will _anyone_ use sigaltstack(SS_ONSTACK)
in a sighandler without SS_FORCE? And the answer is likely "no".
In which case your SS_FORCE erodes to "I run it from sighandler"
but this info the kernel already knows.

>> I don't think this is the problem because only the signal handler
>> should re-enable the sigaltstack, and I don't think we really should
>> switch between 2 active signal handlers. And even if we did, there
>> can be only one sigaltstack per thread, so it will re-enable always
>> the right stack (there is only one).
> Why would there only be one per thread?
If you mean every sighandler installs its own, then I think
switching between such sighandlers is broken anyway.
If you mean the non-sighandler contexts should install multiple
sigaltstacks, then I don't think this is supported or can work.

>>> ISTM it would be simpler if you did:
>>>
>>> sigaltstack(disable, force)
>>> swapcontext() to context using sigaltstack
>>> sigaltstack(set new altstack)
>>>
>>> and then later
>>>
>>> sigaltstack(disable, force)  /* just in case.  save old state, too. */
>>> swapcontext() to context not using sigaltstack
>>> sigaltstack(set new altstack)
>> In the real world you don't even need sigaltstack(set new altstack)
>> because uc_stack does this for you on rt_sigreturn. It is only my
>> test-case that does so.
> That's only the case if swapcontext is implemented using rt_sigreturn.  Is it?
No, its when you use SA_SIGINFO with sigaction().
Then the sigaltstack will magically restore itself once you
leave the sighandler. That's why I wouldn't suggest to ever
modify the sas inside the sighandler the way you propose:
it will simply not work, uc_stack will set it back. My re-enable
trick is at least in agreement with uc_stack.

>>> If it would be POSIX compliant to allow SS_DISABLE to work even if on
>>> the altstack even without a new flag (which is what you're
>>> suggesting), then getting rid of the temporary in-kernel state would
>>> considerably simplify this patch series.  Just skip the -EPERM check
>>> in the disable path.
>> Yes, that's why I was suggesting to just remove the EPERM
>> check initially. We can still do exactly that. The only problem I
>> can see with removing EPERM is that it would be hard to emulate
>> the old behaviour if need be. For example if glibc want to return
>> EPERM behaviour, it will have problems doing so because oss->ss_flags
>> doesn't say if we are on a sigaltstack and there is no other way
>> to find out.
> ...which is why I suggested SS_FORCE in the first place.
I understand. With SS_FORCE there is no temptation to emulate
the old behaviour, so there may be a fewer need to look into
oss->sa_flags even when you return it an inconsistent ways.

We probably should make a summary of our findings or they
will be forgotten.
So far I was pointing to a couple of minor problems with SS_FORCE:
- bypasses overflow protection
- prevents from asking the kernel if we are on sigaltstack or not
... and a few that I consider more important:
- does not bring any value other than to say "I am calling from sighandler"
- allows the programmer to freely modify sas while later it will
still be reset with uc_stack
(and I've likely forgot some already)

There are the upsides compared to just removing EPERM:
- fewer need to look into oss->sa_flags, so its inconsistency
became forgivable
(have I missed something else? please fill in)

There are the upsides compared to remembering the ss_flags:
- simpler code
- slightly better semantic wrt kernel threads (with my approach
restoring the context on a different kernel thread, will require
setting a separate sas there instead of restoring... but I am not
sure this is a considerably bad semantic because whoever messes
with kernel threads this way, should know how to propoerly
set sigaltstacks)

So from that summary I can agree that SS_FORCE may from
some POV be better than simply removing EPERM, as they both
share the downsides, with SS_FORCE providing minor advantages.
But I still don't see it beating my new approach.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

Honestly, I am not sure I understand what this patch does and why, and it is
white space damaged, please fix.

On 01/31, Stas Sergeev wrote:
>
> linux implements the sigaltstack() in a way that makes it impossible to
> use with swapcontext(). Per the man page, sigaltstack is allowed to return
> EPERM if the process is altering its sigaltstack while running on
> sigaltstack.
> This is likely needed to consistently return oss->ss_flags, that indicates
> whether the process is being on sigaltstack or not.
> Unfortunately, linux takes that permission to return EPERM too literally:
> it returns EPERM even if you don't want to change to another sigaltstack,
> but only want to temporarily disable sigaltstack with SS_DISABLE.
> You can't use swapcontext() without disabling sigaltstack first, or the
> stack will be re-used and overwritten by a subsequent signal.

So iiuc you want to switch the stack from the signal handler running on the
alt stack, and you need to ensure that another SA_ONSTACK signal won't corrupt
the alt stack in between, right?

Perhaps you can update the changelog to explain why do we want this change.


> @@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long sp)
>  {
>      if (!current->sas_ss_size)
>          return SS_DISABLE;
> -
> -    return on_sig_stack(sp) ? SS_ONSTACK : 0;
> +    if (on_sig_stack(sp))
> +        return SS_ONSTACK;
> +    if (current->sas_ss_flags == SS_DISABLE)
> +        return SS_DISABLE;
> +    return 0;

So this always return SS_ONSTACK if on_sig_stack(), see below.

> +        onsigstack = on_sig_stack(sp);
> +        if (ss_size == 0) {
> +            switch (ss_flags) {
> +            case 0:
> +                error = -EPERM;
> +                if (onsigstack)
> +                    goto out;
> +                current->sas_ss_sp = 0;
> +                current->sas_ss_size = 0;
> +                current->sas_ss_flags = SS_DISABLE;
> +                break;
> +            case SS_ONSTACK:
> +                /* re-enable previously disabled sas */
> +                error = -EINVAL;
> +                if (current->sas_ss_size == 0)
> +                    goto out;
> +                break;
> +            default:
> +                break;
> +            }

and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags
even if on_sig_stack().

So the sequence is

	// running on alt stack

	sigaltstack(SS_DISABLE);

	temporary_run_on_another_stack();

	sigaltstack(SS_ONSTACK);

and SS_DISABLE saves us from another SA_ONSTACK signal, right?

But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
your changes (afaics), it won't pick the alt stack after SS_DISABLE.

However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
uc_stack->ss_flags, and after return from signal handler restore_altstack() will
enable alt stack again?

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

Honestly, I am not sure I understand what this patch does and why, and it is
white space damaged, please fix.

On 01/31, Stas Sergeev wrote:
>
> linux implements the sigaltstack() in a way that makes it impossible to
> use with swapcontext(). Per the man page, sigaltstack is allowed to return
> EPERM if the process is altering its sigaltstack while running on
> sigaltstack.
> This is likely needed to consistently return oss->ss_flags, that indicates
> whether the process is being on sigaltstack or not.
> Unfortunately, linux takes that permission to return EPERM too literally:
> it returns EPERM even if you don't want to change to another sigaltstack,
> but only want to temporarily disable sigaltstack with SS_DISABLE.
> You can't use swapcontext() without disabling sigaltstack first, or the
> stack will be re-used and overwritten by a subsequent signal.

So iiuc you want to switch the stack from the signal handler running on the
alt stack, and you need to ensure that another SA_ONSTACK signal won't corrupt
the alt stack in between, right?

Perhaps you can update the changelog to explain why do we want this change.


> @@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long sp)
>  {
>      if (!current->sas_ss_size)
>          return SS_DISABLE;
> -
> -    return on_sig_stack(sp) ? SS_ONSTACK : 0;
> +    if (on_sig_stack(sp))
> +        return SS_ONSTACK;
> +    if (current->sas_ss_flags == SS_DISABLE)
> +        return SS_DISABLE;
> +    return 0;

So this always return SS_ONSTACK if on_sig_stack(), see below.

> +        onsigstack = on_sig_stack(sp);
> +        if (ss_size == 0) {
> +            switch (ss_flags) {
> +            case 0:
> +                error = -EPERM;
> +                if (onsigstack)
> +                    goto out;
> +                current->sas_ss_sp = 0;
> +                current->sas_ss_size = 0;
> +                current->sas_ss_flags = SS_DISABLE;
> +                break;
> +            case SS_ONSTACK:
> +                /* re-enable previously disabled sas */
> +                error = -EINVAL;
> +                if (current->sas_ss_size == 0)
> +                    goto out;
> +                break;
> +            default:
> +                break;
> +            }

and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags
even if on_sig_stack().

So the sequence is

	// running on alt stack

	sigaltstack(SS_DISABLE);

	temporary_run_on_another_stack();

	sigaltstack(SS_ONSTACK);

and SS_DISABLE saves us from another SA_ONSTACK signal, right?

But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
your changes (afaics), it won't pick the alt stack after SS_DISABLE.

However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
uc_stack->ss_flags, and after return from signal handler restore_altstack() will
enable alt stack again?

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 19:06, Oleg Nesterov пишет:
> Honestly, I am not sure I understand what this patch does and why, and it is
> white space damaged, please fix.
Arrr.

> On 01/31, Stas Sergeev wrote:
>> linux implements the sigaltstack() in a way that makes it impossible to
>> use with swapcontext(). Per the man page, sigaltstack is allowed to return
>> EPERM if the process is altering its sigaltstack while running on
>> sigaltstack.
>> This is likely needed to consistently return oss->ss_flags, that indicates
>> whether the process is being on sigaltstack or not.
>> Unfortunately, linux takes that permission to return EPERM too literally:
>> it returns EPERM even if you don't want to change to another sigaltstack,
>> but only want to temporarily disable sigaltstack with SS_DISABLE.
>> You can't use swapcontext() without disabling sigaltstack first, or the
>> stack will be re-used and overwritten by a subsequent signal.
> So iiuc you want to switch the stack from the signal handler running on the
> alt stack, and you need to ensure that another SA_ONSTACK signal won't corrupt
> the alt stack in between, right?
Yes.

> Perhaps you can update the changelog to explain why do we want this change.
Beyond the fact that swapcontext() is then usable for switching
in/out of sigaltstack? But this is already mentioned and I have no
other reason for getting this in.

>> @@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long sp)
>>   {
>>       if (!current->sas_ss_size)
>>           return SS_DISABLE;
>> -
>> -    return on_sig_stack(sp) ? SS_ONSTACK : 0;
>> +    if (on_sig_stack(sp))
>> +        return SS_ONSTACK;
>> +    if (current->sas_ss_flags == SS_DISABLE)
>> +        return SS_DISABLE;
>> +    return 0;
> So this always return SS_ONSTACK if on_sig_stack(), see below.
>
>> +        onsigstack = on_sig_stack(sp);
>> +        if (ss_size == 0) {
>> +            switch (ss_flags) {
>> +            case 0:
>> +                error = -EPERM;
>> +                if (onsigstack)
>> +                    goto out;
>> +                current->sas_ss_sp = 0;
>> +                current->sas_ss_size = 0;
>> +                current->sas_ss_flags = SS_DISABLE;
>> +                break;
>> +            case SS_ONSTACK:
>> +                /* re-enable previously disabled sas */
>> +                error = -EINVAL;
>> +                if (current->sas_ss_size == 0)
>> +                    goto out;
>> +                break;
>> +            default:
>> +                break;
>> +            }
> and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags
> even if on_sig_stack().
>
> So the sequence is
>
> 	// running on alt stack
>
> 	sigaltstack(SS_DISABLE);
>
> 	temporary_run_on_another_stack();
>
> 	sigaltstack(SS_ONSTACK);
>
> and SS_DISABLE saves us from another SA_ONSTACK signal, right?
Yes.
Note: there is a test-case in that patch serie from which
you can see or copy/paste the sample code.

> But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
> comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
> your changes (afaics), it won't pick the alt stack after SS_DISABLE.
>
> However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
> uc_stack->ss_flags, and after return from signal handler restore_altstack() will
> enable alt stack again?
I don't think so. Please see the following hunk:

diff --git a/include/linux/signal.h b/include/linux/signal.h
index 92557bb..844b113 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -432,7 +432,7 @@ int __save_altstack(stack_t __user *, unsigned long);
      stack_t __user *__uss = uss; \
      struct task_struct *t = current; \
      put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \
-    put_user_ex(sas_ss_flags(sp), &__uss->ss_flags); \
+    put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
      put_user_ex(t->sas_ss_size, &__uss->ss_size); \
  } while (0);

It pretends as if it changes __save_altstack(), but the reality
is that it actually changes save_altstack_ex(). This is some bug
in git perhaps (or it can't parse macros), I didn't apply any manual
editing to the patch.
The hunk that really modifies __save_altstack() also exists btw:

@@ -3168,7 +3186,7 @@ int __save_altstack(stack_t __user *uss, unsigned 
long sp)
  {
      struct task_struct *t = current;
      return  __put_user((void __user *)t->sas_ss_sp, &uss->ss_sp) |
-        __put_user(sas_ss_flags(sp), &uss->ss_flags) |
+        __put_user(t->sas_ss_flags, &uss->ss_flags) |
          __put_user(t->sas_ss_size, &uss->ss_size);
  }

So I understand this is very confusing, but I think the patch
is correct.

Do you think adding the SS_FORCE flag would be a better solution?

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 19:06, Oleg Nesterov пишет:
> Honestly, I am not sure I understand what this patch does and why, and it is
> white space damaged, please fix.
Arrr.

> On 01/31, Stas Sergeev wrote:
>> linux implements the sigaltstack() in a way that makes it impossible to
>> use with swapcontext(). Per the man page, sigaltstack is allowed to return
>> EPERM if the process is altering its sigaltstack while running on
>> sigaltstack.
>> This is likely needed to consistently return oss->ss_flags, that indicates
>> whether the process is being on sigaltstack or not.
>> Unfortunately, linux takes that permission to return EPERM too literally:
>> it returns EPERM even if you don't want to change to another sigaltstack,
>> but only want to temporarily disable sigaltstack with SS_DISABLE.
>> You can't use swapcontext() without disabling sigaltstack first, or the
>> stack will be re-used and overwritten by a subsequent signal.
> So iiuc you want to switch the stack from the signal handler running on the
> alt stack, and you need to ensure that another SA_ONSTACK signal won't corrupt
> the alt stack in between, right?
Yes.

> Perhaps you can update the changelog to explain why do we want this change.
Beyond the fact that swapcontext() is then usable for switching
in/out of sigaltstack? But this is already mentioned and I have no
other reason for getting this in.

>> @@ -2550,8 +2551,11 @@ static inline int sas_ss_flags(unsigned long sp)
>>   {
>>       if (!current->sas_ss_size)
>>           return SS_DISABLE;
>> -
>> -    return on_sig_stack(sp) ? SS_ONSTACK : 0;
>> +    if (on_sig_stack(sp))
>> +        return SS_ONSTACK;
>> +    if (current->sas_ss_flags == SS_DISABLE)
>> +        return SS_DISABLE;
>> +    return 0;
> So this always return SS_ONSTACK if on_sig_stack(), see below.
>
>> +        onsigstack = on_sig_stack(sp);
>> +        if (ss_size == 0) {
>> +            switch (ss_flags) {
>> +            case 0:
>> +                error = -EPERM;
>> +                if (onsigstack)
>> +                    goto out;
>> +                current->sas_ss_sp = 0;
>> +                current->sas_ss_size = 0;
>> +                current->sas_ss_flags = SS_DISABLE;
>> +                break;
>> +            case SS_ONSTACK:
>> +                /* re-enable previously disabled sas */
>> +                error = -EINVAL;
>> +                if (current->sas_ss_size == 0)
>> +                    goto out;
>> +                break;
>> +            default:
>> +                break;
>> +            }
> and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags
> even if on_sig_stack().
>
> So the sequence is
>
> 	// running on alt stack
>
> 	sigaltstack(SS_DISABLE);
>
> 	temporary_run_on_another_stack();
>
> 	sigaltstack(SS_ONSTACK);
>
> and SS_DISABLE saves us from another SA_ONSTACK signal, right?
Yes.
Note: there is a test-case in that patch serie from which
you can see or copy/paste the sample code.

> But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
> comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
> your changes (afaics), it won't pick the alt stack after SS_DISABLE.
>
> However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
> uc_stack->ss_flags, and after return from signal handler restore_altstack() will
> enable alt stack again?
I don't think so. Please see the following hunk:

diff --git a/include/linux/signal.h b/include/linux/signal.h
index 92557bb..844b113 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -432,7 +432,7 @@ int __save_altstack(stack_t __user *, unsigned long);
      stack_t __user *__uss = uss; \
      struct task_struct *t = current; \
      put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \
-    put_user_ex(sas_ss_flags(sp), &__uss->ss_flags); \
+    put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
      put_user_ex(t->sas_ss_size, &__uss->ss_size); \
  } while (0);

It pretends as if it changes __save_altstack(), but the reality
is that it actually changes save_altstack_ex(). This is some bug
in git perhaps (or it can't parse macros), I didn't apply any manual
editing to the patch.
The hunk that really modifies __save_altstack() also exists btw:

@@ -3168,7 +3186,7 @@ int __save_altstack(stack_t __user *uss, unsigned 
long sp)
  {
      struct task_struct *t = current;
      return  __put_user((void __user *)t->sas_ss_sp, &uss->ss_sp) |
-        __put_user(sas_ss_flags(sp), &uss->ss_flags) |
+        __put_user(t->sas_ss_flags, &uss->ss_flags) |
          __put_user(t->sas_ss_size, &uss->ss_size);
  }

So I understand this is very confusing, but I think the patch
is correct.

Do you think adding the SS_FORCE flag would be a better solution?

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

On 02/01, Stas Sergeev wrote:
>
> >So the sequence is
> >
> >	// running on alt stack
> >
> >	sigaltstack(SS_DISABLE);
> >
> >	temporary_run_on_another_stack();
> >
> >	sigaltstack(SS_ONSTACK);
> >
> >and SS_DISABLE saves us from another SA_ONSTACK signal, right?
> Yes.
> Note: there is a test-case in that patch serie from which
> you can see or copy/paste the sample code.

OK, I wasn't cc'ed

> >But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
> >comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
> >your changes (afaics), it won't pick the alt stack after SS_DISABLE.
> >
> >However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
> >uc_stack->ss_flags, and after return from signal handler restore_altstack() will
> >enable alt stack again?
> I don't think so. Please see the following hunk:

Yes, see another email, I already noticed this change.

> So I understand this is very confusing, but I think the patch
> is correct.

Not sure, but I can hardly read this patch and I can't apply it.

> Do you think adding the SS_FORCE flag would be a better solution?

Yes, certainly. I see no point to remember that a thread actually has the alt stack
but it was disabled.

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

On 02/01, Stas Sergeev wrote:
>
> >So the sequence is
> >
> >	// running on alt stack
> >
> >	sigaltstack(SS_DISABLE);
> >
> >	temporary_run_on_another_stack();
> >
> >	sigaltstack(SS_ONSTACK);
> >
> >and SS_DISABLE saves us from another SA_ONSTACK signal, right?
> Yes.
> Note: there is a test-case in that patch serie from which
> you can see or copy/paste the sample code.

OK, I wasn't cc'ed

> >But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
> >comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
> >your changes (afaics), it won't pick the alt stack after SS_DISABLE.
> >
> >However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
> >uc_stack->ss_flags, and after return from signal handler restore_altstack() will
> >enable alt stack again?
> I don't think so. Please see the following hunk:

Yes, see another email, I already noticed this change.

> So I understand this is very confusing, but I think the patch
> is correct.

Not sure, but I can hardly read this patch and I can't apply it.

> Do you think adding the SS_FORCE flag would be a better solution?

Yes, certainly. I see no point to remember that a thread actually has the alt stack
but it was disabled.

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

On 02/01, Oleg Nesterov wrote:
>
> > +        onsigstack = on_sig_stack(sp);
> > +        if (ss_size == 0) {
> > +            switch (ss_flags) {
> > +            case 0:
> > +                error = -EPERM;
> > +                if (onsigstack)
> > +                    goto out;
> > +                current->sas_ss_sp = 0;
> > +                current->sas_ss_size = 0;
> > +                current->sas_ss_flags = SS_DISABLE;
> > +                break;
> > +            case SS_ONSTACK:
> > +                /* re-enable previously disabled sas */
> > +                error = -EINVAL;
> > +                if (current->sas_ss_size == 0)
> > +                    goto out;
> > +                break;
> > +            default:
> > +                break;
> > +            }
>
> and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags
> even if on_sig_stack().
>
> So the sequence is
>
> 	// running on alt stack
>
> 	sigaltstack(SS_DISABLE);
>
> 	temporary_run_on_another_stack();
>
> 	sigaltstack(SS_ONSTACK);
>
> and SS_DISABLE saves us from another SA_ONSTACK signal, right?
>
> But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
> comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
> your changes (afaics), it won't pick the alt stack after SS_DISABLE.
>
> However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
> uc_stack->ss_flags, and after return from signal handler restore_altstack() will
> enable alt stack again?

OK, I didn't notice you modified save_altstack_ex() to use ->sas_ss_flags instead
of sas_ss_flags()... still doesn't look right, in this case restore_altstack() will
not restore sas_ss_size/sas_ss_sp and they can be changed by signal handler.

Anyway, whatever I missed I agree with Andy, SS_FORCE looks simpler and better to me.

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

On 02/01, Oleg Nesterov wrote:
>
> > +        onsigstack = on_sig_stack(sp);
> > +        if (ss_size == 0) {
> > +            switch (ss_flags) {
> > +            case 0:
> > +                error = -EPERM;
> > +                if (onsigstack)
> > +                    goto out;
> > +                current->sas_ss_sp = 0;
> > +                current->sas_ss_size = 0;
> > +                current->sas_ss_flags = SS_DISABLE;
> > +                break;
> > +            case SS_ONSTACK:
> > +                /* re-enable previously disabled sas */
> > +                error = -EINVAL;
> > +                if (current->sas_ss_size == 0)
> > +                    goto out;
> > +                break;
> > +            default:
> > +                break;
> > +            }
>
> and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags
> even if on_sig_stack().
>
> So the sequence is
>
> 	// running on alt stack
>
> 	sigaltstack(SS_DISABLE);
>
> 	temporary_run_on_another_stack();
>
> 	sigaltstack(SS_ONSTACK);
>
> and SS_DISABLE saves us from another SA_ONSTACK signal, right?
>
> But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
> comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
> your changes (afaics), it won't pick the alt stack after SS_DISABLE.
>
> However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
> uc_stack->ss_flags, and after return from signal handler restore_altstack() will
> enable alt stack again?

OK, I didn't notice you modified save_altstack_ex() to use ->sas_ss_flags instead
of sas_ss_flags()... still doesn't look right, in this case restore_altstack() will
not restore sas_ss_size/sas_ss_sp and they can be changed by signal handler.

Anyway, whatever I missed I agree with Andy, SS_FORCE looks simpler and better to me.

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 20:09, Oleg Nesterov пишет:
> On 02/01, Oleg Nesterov wrote:
>>> +        onsigstack = on_sig_stack(sp);
>>> +        if (ss_size == 0) {
>>> +            switch (ss_flags) {
>>> +            case 0:
>>> +                error = -EPERM;
>>> +                if (onsigstack)
>>> +                    goto out;
>>> +                current->sas_ss_sp = 0;
>>> +                current->sas_ss_size = 0;
>>> +                current->sas_ss_flags = SS_DISABLE;
>>> +                break;
>>> +            case SS_ONSTACK:
>>> +                /* re-enable previously disabled sas */
>>> +                error = -EINVAL;
>>> +                if (current->sas_ss_size == 0)
>>> +                    goto out;
>>> +                break;
>>> +            default:
>>> +                break;
>>> +            }
>> and iiuc the "default" case allows you to write SS_DISABLE into ->sas_ss_flags
>> even if on_sig_stack().
>>
>> So the sequence is
>>
>> 	// running on alt stack
>>
>> 	sigaltstack(SS_DISABLE);
>>
>> 	temporary_run_on_another_stack();
>>
>> 	sigaltstack(SS_ONSTACK);
>>
>> and SS_DISABLE saves us from another SA_ONSTACK signal, right?
>>
>> But afaics it can only help after we change the stack. Suppose that SA_ONSTACK signal
>> comess before temporary_run_on_another_stack(). get_sigframe() should be fine after
>> your changes (afaics), it won't pick the alt stack after SS_DISABLE.
>>
>> However, unless I missed something save_altstack_ex() will record SS_ONSTACK in
>> uc_stack->ss_flags, and after return from signal handler restore_altstack() will
>> enable alt stack again?
> OK, I didn't notice you modified save_altstack_ex() to use ->sas_ss_flags instead
> of sas_ss_flags()... still doesn't look right, in this case restore_altstack() will
> not restore sas_ss_size/sas_ss_sp and they can be changed by signal handler.
How?
Trying to change them in a sighandler with sigaltstack()
will get EPERM. And if you change them in uc_stack without
setting ss_flags back to SS_ONSTACK, they should better be ignored.

> Anyway, whatever I missed I agree with Andy, SS_FORCE looks simpler and better to me.
But perhaps you missed the most important thing, that
it is not possible to change the altstack in sighandler - you'll
get EPERM, even with my patch. But with SS_FORCE this is
exactly not the case. So I'd like you to confirm your opinion
after all the implementation details are understood.
Also it would be interesting to know what do you think about
just removing the EPERM check instead of this all. There are
3 possibilities to choose from, not 2. Removing EPERM looks
simplest.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

On 02/01, Stas Sergeev wrote:
>
> 01.02.2016 20:09, Oleg Nesterov пишет:
> >OK, I didn't notice you modified save_altstack_ex() to use ->sas_ss_flags instead
> >of sas_ss_flags()... still doesn't look right, in this case restore_altstack() will
> >not restore sas_ss_size/sas_ss_sp and they can be changed by signal handler.
> How?
> Trying to change them in a sighandler with sigaltstack()
> will get EPERM.

Only if on_sig_stack() and this is not true if we change the stack.

> >Anyway, whatever I missed I agree with Andy, SS_FORCE looks simpler and better to me.
>
> But perhaps you missed the most important thing, that
> it is not possible to change the altstack in sighandler - you'll
> get EPERM, even with my patch.

See above.

> But with SS_FORCE this is
> exactly not the case.

Yes, and SS_FORCE means "I know what I do", looks very simple.

> Also it would be interesting to know what do you think about
> just removing the EPERM check instead of this all.

I won't argue, but to me it would be better to keep this EPERM if !force.
Just because we should avoid the incompatible changes if possible.

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 21:04, Oleg Nesterov пишет:
 > Yes, and SS_FORCE means "I know what I do", looks very simple.
But to me its not because I don't know what to do with
uc_stack after SS_FORCE is applied.

> I won't argue, but to me it would be better to keep this EPERM if !force.
> Just because we should avoid the incompatible changes if possible.
Ok then. Lets implement SS_FORCE.
What semantic should it have wrt uc_stack?

sigaltstack(SS_DISABLE | SS_FORCE);
swapcontext();
sigaltstack(set up new_sas);
rt_sigreturn();

What's at the end? Do we want a surprise for the user
that he's new_sas got ignored?

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 21:04, Oleg Nesterov пишет:
 > Yes, and SS_FORCE means "I know what I do", looks very simple.
But to me its not because I don't know what to do with
uc_stack after SS_FORCE is applied.

> I won't argue, but to me it would be better to keep this EPERM if !force.
> Just because we should avoid the incompatible changes if possible.
Ok then. Lets implement SS_FORCE.
What semantic should it have wrt uc_stack?

sigaltstack(SS_DISABLE | SS_FORCE);
swapcontext();
sigaltstack(set up new_sas);
rt_sigreturn();

What's at the end? Do we want a surprise for the user
that he's new_sas got ignored?

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Mon, Feb 1, 2016 at 10:16 AM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
> 01.02.2016 21:04, Oleg Nesterov пишет:
>> Yes, and SS_FORCE means "I know what I do", looks very simple.
> But to me its not because I don't know what to do with
> uc_stack after SS_FORCE is applied.
>
>> I won't argue, but to me it would be better to keep this EPERM if !force.
>> Just because we should avoid the incompatible changes if possible.
>
> Ok then. Lets implement SS_FORCE.
> What semantic should it have wrt uc_stack?
>
> sigaltstack(SS_DISABLE | SS_FORCE);
> swapcontext();
> sigaltstack(set up new_sas);
> rt_sigreturn();
>
> What's at the end? Do we want a surprise for the user
> that he's new_sas got ignored?

More detail please.  What context are you returning to with
rt_sigreturn?  What's in uc_stack?

Presumably we should continue to honor uc_stack in rt_sigreturn.  I'm
less clear on whether we should have an implicit SS_FORCE when
restoring uc_stack.  I'm also not clear on why uc_stack exists in the
first place.

If I were designing this from scratch, I'd have signal delivery for an
SA_ONSTACK signal save away the altstack information and clear it so
that nested signals work without checking sp during signal delivery.
But I'm not designing it from scratch, and I haven't spotted uc_stack
or similar mentioned in POSIX or the man page, so I'm not really clear
on what it's for.

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Andy Lutomirski]

On Mon, Feb 1, 2016 at 10:16 AM, Stas Sergeev <stsp@list.ru> wrote:
> 01.02.2016 21:04, Oleg Nesterov пишет:
>> Yes, and SS_FORCE means "I know what I do", looks very simple.
> But to me its not because I don't know what to do with
> uc_stack after SS_FORCE is applied.
>
>> I won't argue, but to me it would be better to keep this EPERM if !force.
>> Just because we should avoid the incompatible changes if possible.
>
> Ok then. Lets implement SS_FORCE.
> What semantic should it have wrt uc_stack?
>
> sigaltstack(SS_DISABLE | SS_FORCE);
> swapcontext();
> sigaltstack(set up new_sas);
> rt_sigreturn();
>
> What's at the end? Do we want a surprise for the user
> that he's new_sas got ignored?

More detail please.  What context are you returning to with
rt_sigreturn?  What's in uc_stack?

Presumably we should continue to honor uc_stack in rt_sigreturn.  I'm
less clear on whether we should have an implicit SS_FORCE when
restoring uc_stack.  I'm also not clear on why uc_stack exists in the
first place.

If I were designing this from scratch, I'd have signal delivery for an
SA_ONSTACK signal save away the altstack information and clear it so
that nested signals work without checking sp during signal delivery.
But I'm not designing it from scratch, and I haven't spotted uc_stack
or similar mentioned in POSIX or the man page, so I'm not really clear
on what it's for.

--Andy

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 21:28, Andy Lutomirski пишет:
> On Mon, Feb 1, 2016 at 10:16 AM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>> 01.02.2016 21:04, Oleg Nesterov пишет:
>>> Yes, and SS_FORCE means "I know what I do", looks very simple.
>> But to me its not because I don't know what to do with
>> uc_stack after SS_FORCE is applied.
>>
>>> I won't argue, but to me it would be better to keep this EPERM if !force.
>>> Just because we should avoid the incompatible changes if possible.
>> Ok then. Lets implement SS_FORCE.
>> What semantic should it have wrt uc_stack?
>>
>> sigaltstack(SS_DISABLE | SS_FORCE);
>> swapcontext();
>> sigaltstack(set up new_sas);
>> rt_sigreturn();
>>
>> What's at the end? Do we want a surprise for the user
>> that he's new_sas got ignored?
> More detail please.  What context are you returning to with
> rt_sigreturn?  What's in uc_stack?
Whatever was saved there by save_altstack_ex() I guess.
Which is the sas params on signal entry.
And I am returning to the interrupted user context.
I am using SA_SIGINFO with sigaction().
This is actually what I was asking you already yeaterday.
I don't think SS_FORCE can play nicely with uc_stack and
you haven't clarified that part, so lets try again.

> Presumably we should continue to honor uc_stack in rt_sigreturn.
In this case, the above sigaltstack(set up new_sas) just
gets ignored. Are we allright with that? If so, I can code
up the patch. Whatever. :)

>    I'm
> less clear on whether we should have an implicit SS_FORCE when
> restoring uc_stack.
This is obscure and is likely outside of the scope of the
problem at hands.

>    I'm also not clear on why uc_stack exists in the
> first place.
>
> If I were designing this from scratch, I'd have signal delivery for an
> SA_ONSTACK signal save away the altstack information and clear it so
> that nested signals work without checking sp during signal delivery.
How would you then evaluate oss->ss_flags?

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 21:28, Andy Lutomirski пишет:
> On Mon, Feb 1, 2016 at 10:16 AM, Stas Sergeev <stsp@list.ru> wrote:
>> 01.02.2016 21:04, Oleg Nesterov пишет:
>>> Yes, and SS_FORCE means "I know what I do", looks very simple.
>> But to me its not because I don't know what to do with
>> uc_stack after SS_FORCE is applied.
>>
>>> I won't argue, but to me it would be better to keep this EPERM if !force.
>>> Just because we should avoid the incompatible changes if possible.
>> Ok then. Lets implement SS_FORCE.
>> What semantic should it have wrt uc_stack?
>>
>> sigaltstack(SS_DISABLE | SS_FORCE);
>> swapcontext();
>> sigaltstack(set up new_sas);
>> rt_sigreturn();
>>
>> What's at the end? Do we want a surprise for the user
>> that he's new_sas got ignored?
> More detail please.  What context are you returning to with
> rt_sigreturn?  What's in uc_stack?
Whatever was saved there by save_altstack_ex() I guess.
Which is the sas params on signal entry.
And I am returning to the interrupted user context.
I am using SA_SIGINFO with sigaction().
This is actually what I was asking you already yeaterday.
I don't think SS_FORCE can play nicely with uc_stack and
you haven't clarified that part, so lets try again.

> Presumably we should continue to honor uc_stack in rt_sigreturn.
In this case, the above sigaltstack(set up new_sas) just
gets ignored. Are we allright with that? If so, I can code
up the patch. Whatever. :)

>    I'm
> less clear on whether we should have an implicit SS_FORCE when
> restoring uc_stack.
This is obscure and is likely outside of the scope of the
problem at hands.

>    I'm also not clear on why uc_stack exists in the
> first place.
>
> If I were designing this from scratch, I'd have signal delivery for an
> SA_ONSTACK signal save away the altstack information and clear it so
> that nested signals work without checking sp during signal delivery.
How would you then evaluate oss->ss_flags?

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

Stas, I probably missed something, but I don't understand your concerns,

On 02/01, Stas Sergeev wrote:
>
> 01.02.2016 21:04, Oleg Nesterov пишет:
> > Yes, and SS_FORCE means "I know what I do", looks very simple.
> But to me its not because I don't know what to do with
> uc_stack after SS_FORCE is applied.

Nothing? restore_sigaltstack() should work as expected?

> >I won't argue, but to me it would be better to keep this EPERM if !force.
> >Just because we should avoid the incompatible changes if possible.
> Ok then. Lets implement SS_FORCE.
> What semantic should it have wrt uc_stack?
>
> sigaltstack(SS_DISABLE | SS_FORCE);
> swapcontext();
> sigaltstack(set up new_sas);
> rt_sigreturn();

Yes, or

	sigaltstack({ DISABLE | FORCE}, &old_ss);
	swapcontext();
	sigaltstack(&old_ss, NULL);
	rt_sigreturn();

and if you are going to return from sighandler you do not even need the 2nd
sigaltstack(), you can rely on sigreturn.

> What's at the end? Do we want a surprise for the user
> that he's new_sas got ignored?

Can't understand.... do you mean "set up new_sas" will be ignored because
rt_sigreturn() does restore_sigaltstack() ? I see no problem here...

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

Stas, I probably missed something, but I don't understand your concerns,

On 02/01, Stas Sergeev wrote:
>
> 01.02.2016 21:04, Oleg Nesterov пишет:
> > Yes, and SS_FORCE means "I know what I do", looks very simple.
> But to me its not because I don't know what to do with
> uc_stack after SS_FORCE is applied.

Nothing? restore_sigaltstack() should work as expected?

> >I won't argue, but to me it would be better to keep this EPERM if !force.
> >Just because we should avoid the incompatible changes if possible.
> Ok then. Lets implement SS_FORCE.
> What semantic should it have wrt uc_stack?
>
> sigaltstack(SS_DISABLE | SS_FORCE);
> swapcontext();
> sigaltstack(set up new_sas);
> rt_sigreturn();

Yes, or

	sigaltstack({ DISABLE | FORCE}, &old_ss);
	swapcontext();
	sigaltstack(&old_ss, NULL);
	rt_sigreturn();

and if you are going to return from sighandler you do not even need the 2nd
sigaltstack(), you can rely on sigreturn.

> What's at the end? Do we want a surprise for the user
> that he's new_sas got ignored?

Can't understand.... do you mean "set up new_sas" will be ignored because
rt_sigreturn() does restore_sigaltstack() ? I see no problem here...

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 21:52, Oleg Nesterov пишет:
> Stas, I probably missed something, but I don't understand your concerns,
>
> On 02/01, Stas Sergeev wrote:
>> 01.02.2016 21:04, Oleg Nesterov пишет:
>>> Yes, and SS_FORCE means "I know what I do", looks very simple.
>> But to me its not because I don't know what to do with
>> uc_stack after SS_FORCE is applied.
> Nothing? restore_sigaltstack() should work as expected?
That's likely the reason for EPERM: restore_sigaltstack()
does the job, so manual modifications are disallowed.
Allowing them will bring in the surprises where the changes
done by the user are ignored.

>>> I won't argue, but to me it would be better to keep this EPERM if !force.
>>> Just because we should avoid the incompatible changes if possible.
>> Ok then. Lets implement SS_FORCE.
>> What semantic should it have wrt uc_stack?
>>
>> sigaltstack(SS_DISABLE | SS_FORCE);
>> swapcontext();
>> sigaltstack(set up new_sas);
>> rt_sigreturn();
> Yes, or
>
> 	sigaltstack({ DISABLE | FORCE}, &old_ss);
> 	swapcontext();
> 	sigaltstack(&old_ss, NULL);
> 	rt_sigreturn();
>
> and if you are going to return from sighandler you do not even need the 2nd
> sigaltstack(), you can rely on sigreturn.
Yes, that's what I do in my app already.
But its only there when SA_SIGINFO is used.

>> What's at the end? Do we want a surprise for the user
>> that he's new_sas got ignored?
> Can't understand.... do you mean "set up new_sas" will be ignored because
> rt_sigreturn() does restore_sigaltstack() ? I see no problem here...
Allowing the modifications that were previously EPERMed
but will now be silently ignored, may be seen as a problem.
But if it isn't - fine, lets code that.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 21:52, Oleg Nesterov пишет:
> Stas, I probably missed something, but I don't understand your concerns,
>
> On 02/01, Stas Sergeev wrote:
>> 01.02.2016 21:04, Oleg Nesterov пишет:
>>> Yes, and SS_FORCE means "I know what I do", looks very simple.
>> But to me its not because I don't know what to do with
>> uc_stack after SS_FORCE is applied.
> Nothing? restore_sigaltstack() should work as expected?
That's likely the reason for EPERM: restore_sigaltstack()
does the job, so manual modifications are disallowed.
Allowing them will bring in the surprises where the changes
done by the user are ignored.

>>> I won't argue, but to me it would be better to keep this EPERM if !force.
>>> Just because we should avoid the incompatible changes if possible.
>> Ok then. Lets implement SS_FORCE.
>> What semantic should it have wrt uc_stack?
>>
>> sigaltstack(SS_DISABLE | SS_FORCE);
>> swapcontext();
>> sigaltstack(set up new_sas);
>> rt_sigreturn();
> Yes, or
>
> 	sigaltstack({ DISABLE | FORCE}, &old_ss);
> 	swapcontext();
> 	sigaltstack(&old_ss, NULL);
> 	rt_sigreturn();
>
> and if you are going to return from sighandler you do not even need the 2nd
> sigaltstack(), you can rely on sigreturn.
Yes, that's what I do in my app already.
But its only there when SA_SIGINFO is used.

>> What's at the end? Do we want a surprise for the user
>> that he's new_sas got ignored?
> Can't understand.... do you mean "set up new_sas" will be ignored because
> rt_sigreturn() does restore_sigaltstack() ? I see no problem here...
Allowing the modifications that were previously EPERMed
but will now be silently ignored, may be seen as a problem.
But if it isn't - fine, lets code that.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

On 02/01, Stas Sergeev wrote:
>
> 01.02.2016 21:52, Oleg Nesterov пишет:
> >Stas, I probably missed something, but I don't understand your concerns,
> >
> >On 02/01, Stas Sergeev wrote:
> >>01.02.2016 21:04, Oleg Nesterov пишет:
> >>>Yes, and SS_FORCE means "I know what I do", looks very simple.
> >>But to me its not because I don't know what to do with
> >>uc_stack after SS_FORCE is applied.
> >Nothing? restore_sigaltstack() should work as expected?
> That's likely the reason for EPERM: restore_sigaltstack()
> does the job, so manual modifications are disallowed.
> Allowing them will bring in the surprises where the changes
> done by the user are ignored.

Unlikely. Suppose you do sigalstack() and then a non SA_ONSTACK signal handler
runs and calls sigaltstack() again. This won't fail, but restore_sigaltstack()
will restore the old alt stack after return.

I too do not know why uc_stack exists, in fact I do not know about it until
today when I read your patch ;) But it is here, and I do not think SS_FORCE
can add more confusion than we already have.

> >Yes, or
> >
> >	sigaltstack({ DISABLE | FORCE}, &old_ss);
> >	swapcontext();
> >	sigaltstack(&old_ss, NULL);
> >	rt_sigreturn();
> >
> >and if you are going to return from sighandler you do not even need the 2nd
> >sigaltstack(), you can rely on sigreturn.
> Yes, that's what I do in my app already.
> But its only there when SA_SIGINFO is used.

Hmm. how this connects to SA_SIGINFO ?

> >>What's at the end? Do we want a surprise for the user
> >>that he's new_sas got ignored?
> >Can't understand.... do you mean "set up new_sas" will be ignored because
> >rt_sigreturn() does restore_sigaltstack() ? I see no problem here...
> Allowing the modifications that were previously EPERMed
> but will now be silently ignored, may be seen as a problem.
> But if it isn't - fine, lets code that.

Still can't understand. The 2nd sigaltstack() is no longer EPERMed because
application used SS_FORCED before that and disabled altstack.

And it is not ignored, it actually changes alt stack. Until we return from
handler.

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 22:29, Oleg Nesterov пишет:
> On 02/01, Stas Sergeev wrote:
>> 01.02.2016 21:52, Oleg Nesterov пишет:
>>> Stas, I probably missed something, but I don't understand your concerns,
>>>
>>> On 02/01, Stas Sergeev wrote:
>>>> 01.02.2016 21:04, Oleg Nesterov пишет:
>>>>> Yes, and SS_FORCE means "I know what I do", looks very simple.
>>>> But to me its not because I don't know what to do with
>>>> uc_stack after SS_FORCE is applied.
>>> Nothing? restore_sigaltstack() should work as expected?
>> That's likely the reason for EPERM: restore_sigaltstack()
>> does the job, so manual modifications are disallowed.
>> Allowing them will bring in the surprises where the changes
>> done by the user are ignored.
> Unlikely. Suppose you do sigalstack() and then a non SA_ONSTACK signal handler
> runs and calls sigaltstack() again. This won't fail, but restore_sigaltstack()
> will restore the old alt stack after return.
OK.

> I too do not know why uc_stack exists, in fact I do not know about it until
> today when I read your patch ;) But it is here, and I do not think SS_FORCE
> can add more confusion than we already have.
OK.

>>> Yes, or
>>>
>>> 	sigaltstack({ DISABLE | FORCE}, &old_ss);
>>> 	swapcontext();
>>> 	sigaltstack(&old_ss, NULL);
>>> 	rt_sigreturn();
>>>
>>> and if you are going to return from sighandler you do not even need the 2nd
>>> sigaltstack(), you can rely on sigreturn.
>> Yes, that's what I do in my app already.
>> But its only there when SA_SIGINFO is used.
> Hmm. how this connects to SA_SIGINFO ?
AFAIK without SA_SIGINFO you get sigreturn instead of
rt_sigreturn, which doesn't seem to do restore_altstack().
Or am I wrong?

Hmm:

         /* Set up the stack frame */
         if (is_ia32_frame()) {
                 if (ksig->ka.sa.sa_flags & SA_SIGINFO)
                         return ia32_setup_rt_frame(usig, ksig, cset, regs);
                 else
                         return ia32_setup_frame(usig, ksig, cset, regs);
         } else if (is_x32_frame()) {
                 return x32_setup_rt_frame(ksig, cset, regs);
         } else {
                 return __setup_rt_frame(ksig->sig, ksig, set, regs);
         }



>>>> What's at the end? Do we want a surprise for the user
>>>> that he's new_sas got ignored?
>>> Can't understand.... do you mean "set up new_sas" will be ignored because
>>> rt_sigreturn() does restore_sigaltstack() ? I see no problem here...
>> Allowing the modifications that were previously EPERMed
>> but will now be silently ignored, may be seen as a problem.
>> But if it isn't - fine, lets code that.
> Still can't understand. The 2nd sigaltstack() is no longer EPERMed because
> application used SS_FORCED before that and disabled altstack.
>
> And it is not ignored, it actually changes alt stack. Until we return from
> handler.
Before we return, the signals are usually blocked.
So whatever is after return is most important.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 22:29, Oleg Nesterov пишет:
> On 02/01, Stas Sergeev wrote:
>> 01.02.2016 21:52, Oleg Nesterov пишет:
>>> Stas, I probably missed something, but I don't understand your concerns,
>>>
>>> On 02/01, Stas Sergeev wrote:
>>>> 01.02.2016 21:04, Oleg Nesterov пишет:
>>>>> Yes, and SS_FORCE means "I know what I do", looks very simple.
>>>> But to me its not because I don't know what to do with
>>>> uc_stack after SS_FORCE is applied.
>>> Nothing? restore_sigaltstack() should work as expected?
>> That's likely the reason for EPERM: restore_sigaltstack()
>> does the job, so manual modifications are disallowed.
>> Allowing them will bring in the surprises where the changes
>> done by the user are ignored.
> Unlikely. Suppose you do sigalstack() and then a non SA_ONSTACK signal handler
> runs and calls sigaltstack() again. This won't fail, but restore_sigaltstack()
> will restore the old alt stack after return.
OK.

> I too do not know why uc_stack exists, in fact I do not know about it until
> today when I read your patch ;) But it is here, and I do not think SS_FORCE
> can add more confusion than we already have.
OK.

>>> Yes, or
>>>
>>> 	sigaltstack({ DISABLE | FORCE}, &old_ss);
>>> 	swapcontext();
>>> 	sigaltstack(&old_ss, NULL);
>>> 	rt_sigreturn();
>>>
>>> and if you are going to return from sighandler you do not even need the 2nd
>>> sigaltstack(), you can rely on sigreturn.
>> Yes, that's what I do in my app already.
>> But its only there when SA_SIGINFO is used.
> Hmm. how this connects to SA_SIGINFO ?
AFAIK without SA_SIGINFO you get sigreturn instead of
rt_sigreturn, which doesn't seem to do restore_altstack().
Or am I wrong?

Hmm:

         /* Set up the stack frame */
         if (is_ia32_frame()) {
                 if (ksig->ka.sa.sa_flags & SA_SIGINFO)
                         return ia32_setup_rt_frame(usig, ksig, cset, regs);
                 else
                         return ia32_setup_frame(usig, ksig, cset, regs);
         } else if (is_x32_frame()) {
                 return x32_setup_rt_frame(ksig, cset, regs);
         } else {
                 return __setup_rt_frame(ksig->sig, ksig, set, regs);
         }



>>>> What's at the end? Do we want a surprise for the user
>>>> that he's new_sas got ignored?
>>> Can't understand.... do you mean "set up new_sas" will be ignored because
>>> rt_sigreturn() does restore_sigaltstack() ? I see no problem here...
>> Allowing the modifications that were previously EPERMed
>> but will now be silently ignored, may be seen as a problem.
>> But if it isn't - fine, lets code that.
> Still can't understand. The 2nd sigaltstack() is no longer EPERMed because
> application used SS_FORCED before that and disabled altstack.
>
> And it is not ignored, it actually changes alt stack. Until we return from
> handler.
Before we return, the signals are usually blocked.
So whatever is after return is most important.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Oleg Nesterov]

On 02/01, Stas Sergeev wrote:
>
> 01.02.2016 22:29, Oleg Nesterov пишет:
> >>>
> >>>	sigaltstack({ DISABLE | FORCE}, &old_ss);
> >>>	swapcontext();
> >>>	sigaltstack(&old_ss, NULL);
> >>>	rt_sigreturn();
> >>>
> >>>and if you are going to return from sighandler you do not even need the 2nd
> >>>sigaltstack(), you can rely on sigreturn.
> >>Yes, that's what I do in my app already.
> >>But its only there when SA_SIGINFO is used.
> >Hmm. how this connects to SA_SIGINFO ?
> AFAIK without SA_SIGINFO you get sigreturn instead of
> rt_sigreturn, which doesn't seem to do restore_altstack().
> Or am I wrong?
>
> Hmm:
>
>         /* Set up the stack frame */
>         if (is_ia32_frame()) {
>                 if (ksig->ka.sa.sa_flags & SA_SIGINFO)
>                         return ia32_setup_rt_frame(usig, ksig, cset, regs);
>                 else
>                         return ia32_setup_frame(usig, ksig, cset, regs);

Ah, ia32... So this is even more confusing.

> >>>>What's at the end? Do we want a surprise for the user
> >>>>that he's new_sas got ignored?
> >>>Can't understand.... do you mean "set up new_sas" will be ignored because
> >>>rt_sigreturn() does restore_sigaltstack() ? I see no problem here...
> >>Allowing the modifications that were previously EPERMed
> >>but will now be silently ignored, may be seen as a problem.
> >>But if it isn't - fine, lets code that.
> >Still can't understand. The 2nd sigaltstack() is no longer EPERMed because
> >application used SS_FORCED before that and disabled altstack.
> >
> >And it is not ignored, it actually changes alt stack. Until we return from
> >handler.
> Before we return, the signals are usually blocked.
> So whatever is after return is most important.

Yes, but I still can't understand your "silently ignored". At least how does
this differ from the case when a non-SA_ONSTACK signal handler does
sigaltstack() and then rt_sigreturn() restores the old stack.

Oleg.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 23:41, Oleg Nesterov пишет:
> On 02/01, Stas Sergeev wrote:
>> 01.02.2016 22:29, Oleg Nesterov пишет:
>>>>> 	sigaltstack({ DISABLE | FORCE}, &old_ss);
>>>>> 	swapcontext();
>>>>> 	sigaltstack(&old_ss, NULL);
>>>>> 	rt_sigreturn();
>>>>>
>>>>> and if you are going to return from sighandler you do not even need the 2nd
>>>>> sigaltstack(), you can rely on sigreturn.
>>>> Yes, that's what I do in my app already.
>>>> But its only there when SA_SIGINFO is used.
>>> Hmm. how this connects to SA_SIGINFO ?
>> AFAIK without SA_SIGINFO you get sigreturn instead of
>> rt_sigreturn, which doesn't seem to do restore_altstack().
>> Or am I wrong?
>>
>> Hmm:
>>
>>          /* Set up the stack frame */
>>          if (is_ia32_frame()) {
>>                  if (ksig->ka.sa.sa_flags & SA_SIGINFO)
>>                          return ia32_setup_rt_frame(usig, ksig, cset, regs);
>>                  else
>>                          return ia32_setup_frame(usig, ksig, cset, regs);
> Ah, ia32... So this is even more confusing.
>
>>>>>> What's at the end? Do we want a surprise for the user
>>>>>> that he's new_sas got ignored?
>>>>> Can't understand.... do you mean "set up new_sas" will be ignored because
>>>>> rt_sigreturn() does restore_sigaltstack() ? I see no problem here...
>>>> Allowing the modifications that were previously EPERMed
>>>> but will now be silently ignored, may be seen as a problem.
>>>> But if it isn't - fine, lets code that.
>>> Still can't understand. The 2nd sigaltstack() is no longer EPERMed because
>>> application used SS_FORCED before that and disabled altstack.
>>>
>>> And it is not ignored, it actually changes alt stack. Until we return from
>>> handler.
>> Before we return, the signals are usually blocked.
>> So whatever is after return is most important.
> Yes, but I still can't understand your "silently ignored". At least how does
> this differ from the case when a non-SA_ONSTACK signal handler does
> sigaltstack() and then rt_sigreturn() restores the old stack.
There is quite a difference.
It is very-very unlikely that non-SA_ONSTACK signal handler does 
sigaltstack().
I think only the test-case could exhibit this.
But with SS_FORCE - most of every SS_FORCE user will be
trapped, because, as you mentioned, not many know about
uc_stack. My patch was allowing them to do only what is safe:
just as it was without a patch.
But anyway, I'll be implementing SS_FORCE because 2 people
have voted.

Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

01.02.2016 23:41, Oleg Nesterov пишет:
> On 02/01, Stas Sergeev wrote:
>> 01.02.2016 22:29, Oleg Nesterov пишет:
>>>>> 	sigaltstack({ DISABLE | FORCE}, &old_ss);
>>>>> 	swapcontext();
>>>>> 	sigaltstack(&old_ss, NULL);
>>>>> 	rt_sigreturn();
>>>>>
>>>>> and if you are going to return from sighandler you do not even need the 2nd
>>>>> sigaltstack(), you can rely on sigreturn.
>>>> Yes, that's what I do in my app already.
>>>> But its only there when SA_SIGINFO is used.
>>> Hmm. how this connects to SA_SIGINFO ?
>> AFAIK without SA_SIGINFO you get sigreturn instead of
>> rt_sigreturn, which doesn't seem to do restore_altstack().
>> Or am I wrong?
>>
>> Hmm:
>>
>>          /* Set up the stack frame */
>>          if (is_ia32_frame()) {
>>                  if (ksig->ka.sa.sa_flags & SA_SIGINFO)
>>                          return ia32_setup_rt_frame(usig, ksig, cset, regs);
>>                  else
>>                          return ia32_setup_frame(usig, ksig, cset, regs);
> Ah, ia32... So this is even more confusing.
>
>>>>>> What's at the end? Do we want a surprise for the user
>>>>>> that he's new_sas got ignored?
>>>>> Can't understand.... do you mean "set up new_sas" will be ignored because
>>>>> rt_sigreturn() does restore_sigaltstack() ? I see no problem here...
>>>> Allowing the modifications that were previously EPERMed
>>>> but will now be silently ignored, may be seen as a problem.
>>>> But if it isn't - fine, lets code that.
>>> Still can't understand. The 2nd sigaltstack() is no longer EPERMed because
>>> application used SS_FORCED before that and disabled altstack.
>>>
>>> And it is not ignored, it actually changes alt stack. Until we return from
>>> handler.
>> Before we return, the signals are usually blocked.
>> So whatever is after return is most important.
> Yes, but I still can't understand your "silently ignored". At least how does
> this differ from the case when a non-SA_ONSTACK signal handler does
> sigaltstack() and then rt_sigreturn() restores the old stack.
There is quite a difference.
It is very-very unlikely that non-SA_ONSTACK signal handler does 
sigaltstack().
I think only the test-case could exhibit this.
But with SS_FORCE - most of every SS_FORCE user will be
trapped, because, as you mentioned, not many know about
uc_stack. My patch was allowing them to do only what is safe:
just as it was without a patch.
But anyway, I'll be implementing SS_FORCE because 2 people
have voted.

[PATCH v2 0/4] make sigaltstack() compatible with swapcontext()

[Stas Sergeev]

The following patches add the self-test for
sigaltstack(SS_DISABLE) inside the signal handler,
and allow an app to temporarily disable and re-enable
the sigaltstack within a sighandler.
This is needed to make sigaltstack() compatible with
swapcontext(): before using swapcontext() inside
the sighandler, the sigaltstack should be disabled, or
the stack will be corrupted by the subsequent signals.


v2: avoid duplicating the "!sigstack" check in get_sigframe()
as requested by Andy Lutomirski.



  arch/score/kernel/signal.c                   |    2
  arch/x86/kernel/signal.c                     |    2
  include/linux/sched.h                        |    8 +
  include/linux/signal.h                       |    2
  kernel/fork.c                                |    4
  kernel/signal.c                              |   54 +++++++----
  tools/testing/selftests/Makefile             |    1
  tools/testing/selftests/sigaltstack/Makefile |    8 +
  tools/testing/selftests/sigaltstack/sas.c    |  132 
+++++++++++++++++++++++++++
  9 files changed, 189 insertions(+), 24 deletions(-)

[PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[Stas Sergeev]

Currently x86's get_sigframe() checks for "current->sas_ss_size"
to determine whether there is a need to switch to sigaltstack.
The common practice used by all other arches is to check for
sas_ss_flags(sp) == 0

This patch makes the code consistent with other arches and also
allows for the further sigaltstack improvements within this patch serie.
The slight complexity of the patch is added by the optimization on
!sigstack check that was requested by Andy Lutomirski: sas_ss_flags(sp)==0
already implies that we are not on a sigstack, so the code is shuffled
to avoid the duplicate checking.

CC: Andy Lutomirski <luto@amacapital.net>
CC: linux-kernel@vger.kernel.org
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Borislav Petkov <bp@suse.de>
CC: Brian Gerst <brgerst@gmail.com>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Richard Weinberger <richard@nod.at>

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  arch/x86/kernel/signal.c | 23 +++++++++++------------
  1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index cb6282c..285183b 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -213,18 +213,17 @@ get_sigframe(struct k_sigaction *ka, struct 
pt_regs *regs, size_t frame_size,
      if (config_enabled(CONFIG_X86_64))
          sp -= 128;

-    if (!onsigstack) {
-        /* This is the X/Open sanctioned signal stack switching. */
-        if (ka->sa.sa_flags & SA_ONSTACK) {
-            if (current->sas_ss_size)
-                sp = current->sas_ss_sp + current->sas_ss_size;
-        } else if (config_enabled(CONFIG_X86_32) &&
-               (regs->ss & 0xffff) != __USER_DS &&
-               !(ka->sa.sa_flags & SA_RESTORER) &&
-               ka->sa.sa_restorer) {
-                /* This is the legacy signal stack switching. */
-                sp = (unsigned long) ka->sa.sa_restorer;
-        }
+    /* 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 (config_enabled(CONFIG_X86_32) &&
+           !onsigstack &&
+           (regs->ss & 0xffff) != __USER_DS &&
+           !(ka->sa.sa_flags & SA_RESTORER) &&
+           ka->sa.sa_restorer) {
+        /* This is the legacy signal stack switching. */
+        sp = (unsigned long) ka->sa.sa_restorer;
      }

      if (fpu->fpstate_active) {
-- 
2.5.0