by Dave Martin

[permalink] [raw]

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

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 <[email protected]> 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--

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.

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

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

[...]

Cheers
---Dave

2020-10-07 18:18:33

by H.J. Lu

[permalink] [raw]

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

On Wed, Oct 7, 2020 at 8:46 AM Dave Martin <[email protected]> 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 <[email protected]> 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.