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

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 2:36 PM, Stas Sergeev <stsp@xxxxxxx> wrote:
> 31.01.2016 23:11, Andy Lutomirski ÐÐÑÐÑ:
>>
>> On Sun, Jan 31, 2016 at 12:08 PM, Stas Sergeev <stsp@xxxxxxx> wrote:
>>>
>>> 31.01.2016 22:03, Andy Lutomirski ÐÐÑÐÑ:
>>>>
>>>> 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.
>>
>> 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.

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.

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

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

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

...which is why I suggested SS_FORCE in the first place.

--Andy