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

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:

quoted

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.

quoted

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

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