lkml.org 
[lkml]   [2016]   [Jan]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] sigaltstack: remove EPERM check to make swapcontext() usable
From
Date
09.01.2016 05:03, Andy Lutomirski пишет:
> On Fri, Jan 8, 2016 at 5:18 PM, 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 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. The oss->ss_flags
>> will then return SS_DISABLE, which doesn't seem to contradict the
>> (very ambiguous) man page wording, namely:
>> SS_ONSTACK
>> The process is currently executing on the alternate signal
>> stack. (Note that it is not possible to change the alternate
>> signal stack if the process is currently executing on it.)
> You're definitely contradicting the "Note" part, though. POSIX is
> quite clear, too:
>
> "Attempts to modify the alternate signal stack while the process is
> executing on it fail."
"modify" may not include "disable".
You don't modify the stack's location or size, just temporary disable
its use.
So I believe SS_DISABLE should be fine.
Of course this is just one of the possible interpretations.
But it is the one that looks simple and satisfies everyone.

>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index f3f1f7a..0a6af54 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -3111,18 +3111,13 @@ 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.
>> + * Note - this code used to test on_sig_stack(sp) and
>> + * return -EPERM. But we need at least SS_DISABLE to
>> + * work while on sigaltstack, so the check was removed.
>
> That old comment was simply incorrect. POSIX says:
>
> The ss_flags member specifies the new stack state.
> If it is set to SS_DISABLE, the stack is disabled and ss_sp and ss_size
> are ignored. Otherwise, the stack shall be enabled, and the ss_sp and
> ss_size members specify the new address and size of the stack.
>
> Zero is perfectly valid. That being said, Linux has apparently
> rejected non-zero non-SA_ONSTACK values for a long time, so we should
> be fine.
>
> I think it would be safer and more posixly correct to change the
> behavior differently:
>
> ss_flags == 0 or SS_DISABLE or SS_ONSTACK: preserve old Linux behavior.
>
> ss_flags == SS_DISABLE | SS_FORCE: disable the altstack regardless of
> whether we're executing on it.
I really dislike the idea of adding SS_FORCE just for SS_DISABLE.
I think it is sane to use SS_DISABLE in a sighandler, and it doesn't
conflict with one of the possible interpretations of posix.
So I wonder what other people think (add Linus to CC).

> ss_flags == SS_ONSTACK | SS_FORCE: change the altstack regardless of
> whether we're executing on it.
This is something that no one will likely ever need.
For SS_DISABLE there is a clear use-case: swapcontext().
So we _need_ to address the SS_DISABLE case.
But why do we care about SA_ONSTACK in a sighandler's case?
Someone later, if need be, can add it together with the SA_FORCE.

> Users of SS_FORCE are required to be very careful with nested signals.
> In particular, changing the altstack using SS_ONSTACK | SS_FORCE is
> very dangerous:
So how about just not allowing such SS_ONSTACK until
someone really needs it... This is more difficult to implement,
but possible. Would you really add a new flag for the funtionality
that is both dangerous and useless?

\
 
 \ /
  Last update: 2016-01-09 04:21    [W:0.083 / U:0.736 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site