lkml.org 
[lkml]   [2016]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler
From
Date
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?

\
 
 \ /
  Last update: 2016-02-01 18:41    [W:0.084 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site