Re: [RFC PATCH 0/4] x86: Improve Minimum Alternate Stack Size

[H.J. Lu]

On Wed, Oct 7, 2020 at 8:46 AM Dave Martin <Dave.Martin@xxxxxxx> wrote:
>
> On Wed, Oct 07, 2020 at 06:30:03AM -0700, H.J. Lu wrote:
> > On Wed, Oct 7, 2020 at 3:47 AM Dave Martin <Dave.Martin@xxxxxxx> wrote:
> > >
> > > On Tue, Oct 06, 2020 at 10:44:14AM -0700, H.J. Lu wrote:
>
> [...]
>
> > > > I updated my glibc patch to add both _SC_MINSIGSTKSZ and _SC_SIGSTKSZ.
> > > > _SC_MINSIGSTKSZ is  the minimum signal stack size from AT_MINSIGSTKSZ,
> > > > which is the signal frame size used by kernel, and _SC_SIGSTKSZ is the value
> > > > of sysconf (_SC_MINSIGSTKSZ) + 6KB for user application.
> > >
> > > Can I suggest sysconf (_SC_MINSIGSTKSZ) * 4 instead?
> >
> > Done.
>
> OK.  I was prepared to have to argue my case a bit more, but if you
> think this is OK then I will stop arguing ;)
>
>
> > > If the arch has more or bigger registers to save in the signal frame,
> > > the chances are that they're going to get saved in some userspace stack
> > > frames too.  So I suspect that the user signal handler stack usage may
> > > scale up to some extent rather than being a constant.
> > >
> > >
> > > To help people migrate without unpleasant surprises, I also figured it
> > > would be a good idea to make sure that sysconf (_SC_MINSIGSTKSZ) >=
> > > legacy MINSIGSTKSZ, and sysconf (_SC_SIGSTKSZ) >= legacy SIGSTKSZ.
> > > This should makes it safer to use sysconf (_SC_MINSIGSTKSZ) as a
> > > drop-in replacement for MINSIGSTKSZ, etc.
> > >
> > > (To explain: AT_MINSIGSTKSZ may actually be < MINSIGSTKSZ on AArch64.
> > > My idea was that sysconf () should hide this surprise, but people who
> > > really want to know the kernel value would tolerate some
> > > nonportability and read AT_MINSIGSTKSZ directly.)
> > >
> > >
> > > So then:
> > >
> > >         kernel_minsigstksz = getauxval(AT_MINSIGSTKSZ);
> > >         minsigstksz = LEGACY_MINSIGSTKSZ;
> > >         if (kernel_minsigstksz > minsigstksz)
> > >                 minsistksz = kernel_minsigstksz;
> > >
> > >         sigstksz = LEGACY_SIGSTKSZ;
> > >         if (minsigstksz * 4 > sigstksz)
> > >                 sigstksz = minsigstksz * 4;
> >
> > I updated users/hjl/AT_MINSIGSTKSZ branch with
> >
> > +@item _SC_MINSIGSTKSZ
> > +@standards{GNU, unistd.h}
>
> Can we specify these more agressively now?
>
> > +Inquire about the signal stack size used by the kernel.
>
> I think we've already concluded that this should included all mandatory
> overheads, including those imposed by the compiler and glibc?
>
> e.g.:
>
> --8<--
>
> The returned value is the minimum number of bytes of free stack space
> required in order to gurantee successful, non-nested handling of a
> single signal whose handler is an empty function.
>
> -->8--
>
> > +
> > +@item _SC_SIGSTKSZ
> > +@standards{GNU, unistd.h}
> > +Inquire about the default signal stack size for a signal handler.
>
> Similarly:
>
> --8<--
>
> The returned value is the suggested minimum number of bytes of stack
> space required for a signal stack.
>
> This is not guaranteed to be enough for any specific purpose other than
> the invocation of a single, non-nested, empty handler, but nonetheless
> should be enough for basic scenarios involving simple signal handlers
> and very low levels of signal nesting (say, 2 or 3 levels at the very
> most).
>
> This value is provided for developer convenience and to ease migration
> from the legacy SIGSTKSZ constant.  Programs requiring stronger
> guarantees should avoid using it if at all possible.
>
> -->8--

Done.

>
> If these descriptions are too wordy, we might want to move some of it
> out to signal.texi, though.
>
> >
> >     case _SC_MINSIGSTKSZ:
> >       assert (GLRO(dl_minsigstacksize) != 0);
> >       return GLRO(dl_minsigstacksize);
> >
> >     case _SC_SIGSTKSZ:
> >       {
> >         /* Return MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4.  */
> >         long int minsigstacksize = GLRO(dl_minsigstacksize);
> >         _Static_assert (__builtin_constant_p (MINSIGSTKSZ),
> >                         "MINSIGSTKSZ is constant");
> >         if (minsigstacksize < MINSIGSTKSZ)
> >           minsigstacksize = MINSIGSTKSZ;
> >         return minsigstacksize * 4;
> >       }
> >
> > >
> > > (Or something like that, unless the architecture provides its own
> > > definitions.  ia64's MINSIGSTKSZ is enormous, so it probably doesn't
> > > want this.)
> > >
> > >
> > > Also: should all these values be rounded up to a multiple of the
> > > architecture's preferred stack alignment?
> >
> > Kernel should provide a properly aligned AT_MINSIGSTKSZ.
>
> OK.  Can you comment on Chang S. Bae's series?  I wasn't sure that the
> proposal produces an aligned value for AT_MINSIGSTKSZ on x86, but maybe
> I just worked it out wrong.

In Linux kernel, the signal frame data is composed of the following areas
and laid out as:
     ------------------------------
     | alignment padding          |
     ------------------------------
     | xsave buffer               |  <<<<<<< This must be 64 byte aligned
     ------------------------------
     | fsave header (32-bit only) |
     ------------------------------
     | siginfo + ucontext         |
     ------------------------------

In order to get the proper alignment for xsave buffer, the signal stake
size should be rounded up to 64 byte aligned.  In glibc, I have

 /* Size of xsave buffer.  */
  unsigned int sigframe_size = ebx;
if (64 bit)
  /* NB: sizeof(struct rt_sigframe) in Linux kernel.  */
  sigframe_size += 440;
else
  /* NB: sizeof(struct sigframe_ia32) + sizeof(struct fregs_state)) in
     Linux kernel.  */
  sigframe_size += 736 + 112;
endif

  /* Round up to 64-byte aligned.  */
  sigframe_size = ALIGN_UP (sigframe_size, 64);

  GLRO(dl_minsigstacksize) = sigframe_size;

Kernel should do something similar.

>
> > > Should the preferred stack alignment also be exposed through sysconf?
> > > Portable code otherwise has no way to know this, though if the
> > > preferred alignment is <= the minimum malloc()/alloca() alignment then
> > > this is generally not an issue.)
> >
> > Good question.  But it is orthogonal to the signal stack size issue.
>
> Ack.
>
> There are various other brokennesses around this area, such as the fact
> that the register data may now run off the end of the mcontext_t object
> that is supposed to contain it.  Ideally we should fork ucontext_t or
> mcontext_t into two types, one for the ucontext API and one for the
> signal API (which are anyway highly incompatible with each other).
>
> If this stuff is addressed, I guess we could bundle it under a more
> general feature test macro.  But it's probably best to nail down this
> series in something like its current form first.

Agreed.

>
> I'll follow up on libc-alpha with a wishlist, but that will be a
> separate conversation...
>
> [...]
>
> Cheers
> ---Dave



-- 
H.J.