make sigaltstack() compatible with swapcontext()

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 <[email protected]>

CC [email protected]

CC [email protected]

CC Andy Lutomirski <[email protected]>

Signed-off-by Stas Sergeev <[email protected]>

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

tools/testing/selftests/Makefile +1 -0

index c8edff6..d5b2005 100644
@@ -17,6 +17,7 @@ TARGETS += powerpc
 TARGETS += pstore
 TARGETS += ptrace
 TARGETS += seccomp
+TARGETS += sigaltstack
 TARGETS += size
 TARGETS += static_keys
 TARGETS += sysctl

tools/testing/selftests/sigaltstack/Makefile new file +8 -0

new file mode 100644
index 0000000..56af56e
@@ -0,0 +1,8 @@
+CFLAGS = -Wall
+BINARIES = sas
+all: $(BINARIES)
+
+include ../lib.mk
+
+clean:
+    rm -rf $(BINARIES)

tools/testing/selftests/sigaltstack/sas.c new file +132 -0

new file mode 100644
index 0000000..5d9aabd
@@ -0,0 +1,132 @@
+/*
+ * Stas Sergeev <[email protected]>
+ *
+ * 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;
+}

Quoted 11 lines

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 <[email protected]>

CC [email protected]

CC [email protected]

CC Andy Lutomirski <[email protected]>

Quoted 1 line

Signed-off-by Stas Sergeev <[email protected]>

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.

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 <[email protected]>

CC [email protected]

CC Chen Liqin <[email protected]>

CC Lennox Wu <[email protected]>

CC Michael Ellerman <[email protected]>

CC Andrew Morton <[email protected]>

CC James Hogan <[email protected]>

Signed-off-by Stas Sergeev <[email protected]>

---
 arch/score/kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/score/kernel/signal.c +1 -2

index e381c8c..bd1c7c8 100644
@@ -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

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.

Quoted 7 lines

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?

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.

Quoted 12 lines

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.

Quoted 7 lines

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?

31.01.2016 20:00, Andy Lutomirski пишет:

Quoted 26 lines · Stas Sergeev

Quoted 15 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 28 lines

31.01.2016 20:00, Andy Lutomirski пишет:

Quoted 23 lines · Stas Sergeev

Quoted 14 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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?

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 17 lines

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

Quoted 5 lines

          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.

31.01.2016 22:03, Andy Lutomirski пишет:

Quoted 6 lines

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.

Quoted 7 lines

31.01.2016 22:03, Andy Lutomirski пишет:

Quoted 6 lines

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.

Quoted 5 lines

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.

31.01.2016 23:11, Andy Lutomirski пишет:

Quoted 18 lines · Stas Sergeev

Quoted 10 lines

31.01.2016 22:03, Andy Lutomirski пишет:

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

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

Quoted 9 lines

ISTM it would be simpler if you did:

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

and then later

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 19 lines

31.01.2016 23:11, Andy Lutomirski пишет:

Quoted 13 lines · Stas Sergeev

Quoted 7 lines

31.01.2016 22:03, Andy Lutomirski пишет:

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 11 lines

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.

Quoted 5 lines

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?

Quoted 8 lines

Quoted 8 lines

ISTM it would be simpler if you did:

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

and then later

Quoted 6 lines

Quoted 3 lines

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?

Quoted 5 lines

Quoted 5 lines

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.

Quoted 7 lines

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.

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.

(quoted diff) +10 -0

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

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

Quoted 11 lines

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.

Review static inline int sas_ss_flags(unsigned long sp) · line 2558

Quoted 11 lines

(quoted diff) +5 -2

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

Quoted 10 lines

(quoted diff) +10 -0

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

Review static inline int sas_ss_flags(unsigned long sp)

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 7 lines

So the sequence is

	// running on alt stack

	sigaltstack(SS_DISABLE);

Quoted 5 lines

	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.

Quoted 7 lines

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:

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?

Quoted 11 lines

(quoted diff) +10 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 39 lines

Quoted 10 lines

(quoted diff) +10 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 3 lines

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.

01.02.2016 20:09, Oleg Nesterov пишет:

Quoted 3 lines

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.

Quoted 1 line

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.

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

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

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.

01.02.2016 21:04, Oleg Nesterov пишет:

Quoted 1 line

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.

Quoted 2 lines

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?

Quoted 15 lines

01.02.2016 21:04, Oleg Nesterov пишет:

Quoted 1 line

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.

Quoted 2 lines

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();

Quoted 2 lines

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.

01.02.2016 21:04, Oleg Nesterov пишет:

Quoted 1 line

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.

Quoted 2 lines

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?

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

Quoted 4 lines · Stas Sergeev

01.02.2016 21:04, Oleg Nesterov пишет:

Quoted 1 line

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?

Quoted 9 lines

Quoted 2 lines

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.

Quoted 2 lines

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

01.02.2016 21:52, Oleg Nesterov пишет:

Quoted 4 lines

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

Quoted 1 line · Stas Sergeev

01.02.2016 21:04, Oleg Nesterov пишет:

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

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.

Quoted 9 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 8 lines

01.02.2016 21:52, Oleg Nesterov пишет:

Quoted 7 lines

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

Quoted 4 lines · Stas Sergeev

01.02.2016 21:04, Oleg Nesterov пишет:

Quoted 1 line

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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 7 lines

Yes, or

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

Quoted 4 lines

Quoted 2 lines

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 ?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 3 lines

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.

01.02.2016 22:29, Oleg Nesterov пишет:

Quoted 11 lines

Quoted 10 lines

Quoted 8 lines

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

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 15 lines

01.02.2016 22:29, Oleg Nesterov пишет:

Quoted 10 lines

Quoted 9 lines

Quoted 7 lines

	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?

Quoted 8 lines

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.

Quoted 10 lines

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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.