Messages in this thread Patch in this message | | | Subject | Re: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler | From | Stas Sergeev <> | Date | Mon, 1 Feb 2016 19:57:45 +0300 |
| |
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?
| |