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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Jan 16, 2019 at 9:06 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> 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?
>

The new tests failed on AVX512 machines:

Program received signal SIGUSR1, User defined signal 1.
__GI_raise (sig=sig@entry=10) at ../sysdeps/unix/sysv/linux/raise.c:50
50   return ret;
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
_dl_runtime_resolve_xsavec () at ../sysdeps/x86_64/dl-trampoline.h:93
93 movq %rax, REGISTER_SAVE_RAX(%rsp)
(gdb) bt
#0  _dl_runtime_resolve_xsavec () at ../sysdeps/x86_64/dl-trampoline.h:93
#1  0x0040248d in handler (unused=<optimized out>) at tst-minsigstksz-4.c:44
#2  <signal handler called>
#3  __GI_raise (sig=sig@entry=10) at ../sysdeps/unix/sysv/linux/raise.c:50
#4  0x004024da in do_test () at tst-minsigstksz-4.c:59
#5  0x00402cd6 in support_test_main (argc=1, argv=0xffffcef8,
    config=config@entry=0xffffcdf0) at support_test_main.c:350
#6  0x00402348 in main (argc=<optimized out>, argv=<optimized out>)
    at ../support/test-driver.c:168
(gdb)

AVX512 needs 2560 bytes to save processor state.

-- 
H.J.