Re: [PATCH] Tests for minimal signal handler functionality in MINSIGSTKSZ space.

[Carlos O'Donell <carlos at redhat dot com>]

On 1/16/19 9:06 AM, Zack Weinberg wrote:
> The patch I just pushed is attached to this message.  Thanks for the
> quick reviews.  A couple of notes:
> 
>> I'm not sure if it's explicitly spelled out that the default disposition
>> for SIGABRT is to core
> 
> This is indeed specified in POSIX, see the table of default signal actions in
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html .
> 
>> +/* C2011 7.4.1.1p5 specifies that only the following operations are
>> +   guaranteed to be well-defined inside an asynchronous signal handler:
>> ...
>> +     * calling abort, _Exit, quick_exit, or signal
>> +       * signal may only be called with its first argument equal to the
>> +         number of the signal that caused the handler to be called
> 
> It occurred to me this morning that signal is implemented using
> sigaction, and a 'struct sigaction' is 152 bytes on x86-64 (because of
> the sigset_t).  Normally that's nothing to worry about, but when
> there's only 2048 bytes of stack in total and the kernel has filled
> more than half of that with ucontext data, it _could_ be a problem.
> I left the test as is for now, but I suggest that if it does crash on
> any architecture we should drop that part of tst-minsigstksz-1.c
> without hesitation.
> 
> It also occurs to me that on some architectures MINSIGSTKSZ is less
> than a page; on those architectures, the rounding done in xsigstack.c
> means we aren't _really_ testing this stuff in MINSIGSTKSZ space.
> Since overflow is much more of a concern than underflow, what do
> people think of adjusting the code in xsigstack.c so that the area
> actually passed to sigaltstack will not be rounded and will be right
> up against the guard in the direction of overflow?  This would mean
> xsigstack.c has to know which direction the stack grows, but I think
> we already have internal macros for that, so it's not a huge problem.
> I would do this as a follow-up patch if it's agreed to be a good idea.

I worried about this during the original review, but my conclusion was
that no sane user would do such an alignment to push their own stack
against the guard. At least I've never seen it done. This does mean that
the user has a variable amount of trailing stack space that depends on
the configured kernel page size, which is different from the userspace
configured static page size (but the same as the dynamically available
value from sysconf / AT_PAGESZ). If the kernel uses THP you also loose
because the page may be 2MiB and from userspace you don't know that.
So the test adjustment you propose is sensible, and I think would add
value to the test and catch out the currently silly values of 2048
(in most cases).

I expect to see some failures, so I think such an adjustment should
be made once 2.30 is open for development :-) Also keep in mind that
to be pedantic you need to know that the machine stack pointer
alignment requirement is not equal to the page alignment, maybe a note
there that MINSIGSTKSZ is such that it is a multiple of the stack
alignment?

-- 
Cheers,
Carlos.