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

[Stas Sergeev]

01.02.2016 01:44, 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.
I simply don't understand how can we have more than one
sigaltstack per thread. Is this supported? If not then I don't
expect the different non-sighandler user contexts trying to
set up the different ones. So you are probably talking about
2 sighandlers switching between each other, right? And that
case is likely broken anyway.

>    So I don't see why you want the
> kernel to remember about disabled altstacks at all.
2 reasons:
- Language-lawyering around POSIX
- Consistently return oss->ss_flags when we are on a signal stack
Restoring the old sas is not among the goals, but allowing the
sighandler to freely install the new sas (as you suggest) is a clear
contradiction to POSIX. So that's why you propose SS_FORCE, yes,
but then the question: will _anyone_ use sigaltstack(SS_ONSTACK)
in a sighandler without SS_FORCE? And the answer is likely "no".
In which case your SS_FORCE erodes to "I run it from sighandler"
but this info the kernel already knows.

> > 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?
If you mean every sighandler installs its own, then I think
switching between such sighandlers is broken anyway.
If you mean the non-sighandler contexts should install multiple
sigaltstacks, then I don't think this is supported or can work.

> > > 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?
No, its when you use SA_SIGINFO with sigaction().
Then the sigaltstack will magically restore itself once you
leave the sighandler. That's why I wouldn't suggest to ever
modify the sas inside the sighandler the way you propose:
it will simply not work, uc_stack will set it back. My re-enable
trick is at least in agreement with uc_stack.

> > > 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.
I understand. With SS_FORCE there is no temptation to emulate
the old behaviour, so there may be a fewer need to look into
oss->sa_flags even when you return it an inconsistent ways.

We probably should make a summary of our findings or they
will be forgotten.
So far I was pointing to a couple of minor problems with SS_FORCE:
- bypasses overflow protection
- prevents from asking the kernel if we are on sigaltstack or not
... and a few that I consider more important:
- does not bring any value other than to say "I am calling from sighandler"
- allows the programmer to freely modify sas while later it will
still be reset with uc_stack
(and I've likely forgot some already)

There are the upsides compared to just removing EPERM:
- fewer need to look into oss->sa_flags, so its inconsistency
became forgivable
(have I missed something else? please fill in)

There are the upsides compared to remembering the ss_flags:
- simpler code
- slightly better semantic wrt kernel threads (with my approach
restoring the context on a different kernel thread, will require
setting a separate sas there instead of restoring... but I am not
sure this is a considerably bad semantic because whoever messes
with kernel threads this way, should know how to propoerly
set sigaltstacks)

So from that summary I can agree that SS_FORCE may from
some POV be better than simply removing EPERM, as they both
share the downsides, with SS_FORCE providing minor advantages.
But I still don't see it beating my new approach.