V7 [PATCH] sysconf: Add _SC_MINSIGSTKSZ/_SC_SIGSTKSZ [BZ #20305]

  On Tue, Oct 20, 2020 at 7:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Oct 20, 2020 at 2:19 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Mon, Oct 19, 2020 at 02:32:35PM -0700, H.J. Lu via Libc-alpha wrote:
> > > On Mon, Oct 19, 2020 at 8:08 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > > >
> > > > On Thu, Oct 15, 2020 at 04:57:28AM -0700, H.J. Lu via Libc-alpha wrote:
> >
> > [...]
> >
> > > > > > > diff --git a/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h b/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h
> >
> > [...]
> >
> > > > > > > +/* Emulate AT_MINSIGSTKSZ with XSAVE. */
> > > > > > > +
> > > > > > > +static inline void
> > > > > > > +dl_check_minsigstacksize (void)
> > > > > > > +{
> > > > > > > +  /* NB: Default to a constant MINSIGSTKSZ.  */
> > > > > > > +  _Static_assert (__builtin_constant_p (MINSIGSTKSZ),
> > > > > > > +           "MINSIGSTKSZ is constant");
> > > > > > > +  /* Return if AT_MINSIGSTKSZ is provide by kernel.  */
> > > > > > > +  if (GLRO(dl_minsigstacksize) != MINSIGSTKSZ)
> > > > > > > +    return;
> > > > > >
> > > > > > Couldn't the kernel actually yield MINSIGSTKSZ or a smaller value, say,
> > > > > > if running on hardware that doesn't have AVX-512?
> > > > > >
> > > > > It is OK for MINSIGSTKSZ > AT_MINSIGSTKSZ.  For _SC_SIGSTKSZ_SOURCE,
> > > > > dynamic MINSIGSTKSZ is defined as sysconf (_SC_SIGSTKSZ) which is
> > > > >
> > > > > MAX (SIGSTKSZ, MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4)
> > > > >
> > > > > and dynamic MINSIGSTKSZ is always > MINSIGSTKSZ.
> > > > >
> > > > > > We might want a separate flag to indicate whether we obtained a value
> > > > > > from the auxv, rather relying on MINSIGSTKSZ having this magic meaning.
> > > > >
> > > > > AT_MINSIGSTKSZ is the only way for GLRO(dl_minsigstacksize) != MINSIGSTKSZ.
> > > >
> > > > Yes, but reading AT_MINSIGSTKSZ doesn't guarantee that
> > > > GLRO(dl_minsigstkszsize) != MINSIGSTKSZ, no?
> > > >
> > > > What if the value reported for AT_MINSIGSTKSZ is actually the same as
> > > > MINSIGSTKSZ?  This could be the case on some arches in future even if
> > > > it's never true today.  But the code here assumes that AT_MINSIGSTKSZ
> > > > wasn't available in this case, and reverts to a fallback guess.
> > >
> > > Since the fallback tracks what the kernel does, if AT_MINSIGSTKSZ
> > > is 2KB, the fallback will be 2KB or slightly larger.
> >
> > Well, I guess that should be safe.  It still feels a bit like it works
> > by accident, but I may be being too paranoid.
>
> Let me work on that.
>

Here is the updated patch to initialize GLRO(dl_minsigstacksize)
to 0 on x86.
  

On Tue, Oct 20, 2020 at 11:19 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Oct 20, 2020 at 7:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Oct 20, 2020 at 2:19 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > >
> > > On Mon, Oct 19, 2020 at 02:32:35PM -0700, H.J. Lu via Libc-alpha wrote:
> > > > On Mon, Oct 19, 2020 at 8:08 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > > > >
> > > > > On Thu, Oct 15, 2020 at 04:57:28AM -0700, H.J. Lu via Libc-alpha wrote:
> > >
> > > [...]
> > >
> > > > > > > > diff --git a/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h b/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h
> > >
> > > [...]
> > >
> > > > > > > > +/* Emulate AT_MINSIGSTKSZ with XSAVE. */
> > > > > > > > +
> > > > > > > > +static inline void
> > > > > > > > +dl_check_minsigstacksize (void)
> > > > > > > > +{
> > > > > > > > +  /* NB: Default to a constant MINSIGSTKSZ.  */
> > > > > > > > +  _Static_assert (__builtin_constant_p (MINSIGSTKSZ),
> > > > > > > > +           "MINSIGSTKSZ is constant");
> > > > > > > > +  /* Return if AT_MINSIGSTKSZ is provide by kernel.  */
> > > > > > > > +  if (GLRO(dl_minsigstacksize) != MINSIGSTKSZ)
> > > > > > > > +    return;
> > > > > > >
> > > > > > > Couldn't the kernel actually yield MINSIGSTKSZ or a smaller value, say,
> > > > > > > if running on hardware that doesn't have AVX-512?
> > > > > > >
> > > > > > It is OK for MINSIGSTKSZ > AT_MINSIGSTKSZ.  For _SC_SIGSTKSZ_SOURCE,
> > > > > > dynamic MINSIGSTKSZ is defined as sysconf (_SC_SIGSTKSZ) which is
> > > > > >
> > > > > > MAX (SIGSTKSZ, MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4)
> > > > > >
> > > > > > and dynamic MINSIGSTKSZ is always > MINSIGSTKSZ.
> > > > > >
> > > > > > > We might want a separate flag to indicate whether we obtained a value
> > > > > > > from the auxv, rather relying on MINSIGSTKSZ having this magic meaning.
> > > > > >
> > > > > > AT_MINSIGSTKSZ is the only way for GLRO(dl_minsigstacksize) != MINSIGSTKSZ.
> > > > >
> > > > > Yes, but reading AT_MINSIGSTKSZ doesn't guarantee that
> > > > > GLRO(dl_minsigstkszsize) != MINSIGSTKSZ, no?
> > > > >
> > > > > What if the value reported for AT_MINSIGSTKSZ is actually the same as
> > > > > MINSIGSTKSZ?  This could be the case on some arches in future even if
> > > > > it's never true today.  But the code here assumes that AT_MINSIGSTKSZ
> > > > > wasn't available in this case, and reverts to a fallback guess.
> > > >
> > > > Since the fallback tracks what the kernel does, if AT_MINSIGSTKSZ
> > > > is 2KB, the fallback will be 2KB or slightly larger.
> > >
> > > Well, I guess that should be safe.  It still feels a bit like it works
> > > by accident, but I may be being too paranoid.
> >
> > Let me work on that.
> >
>
> Here is the updated patch to initialize GLRO(dl_minsigstacksize)
> to 0 on x86.
>

PING:

https://sourceware.org/pipermail/libc-alpha/2020-October/118843.html
  

On Mon, Nov 02, 2020 at 07:06:13PM -0800, H.J. Lu via Libc-alpha wrote:
> On Tue, Oct 20, 2020 at 11:19 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Tue, Oct 20, 2020 at 7:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 2:19 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > > >
> > > > On Mon, Oct 19, 2020 at 02:32:35PM -0700, H.J. Lu via Libc-alpha wrote:
> > > > > On Mon, Oct 19, 2020 at 8:08 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 15, 2020 at 04:57:28AM -0700, H.J. Lu via Libc-alpha wrote:
> > > >
> > > > [...]
> > > >
> > > > > > > > > diff --git a/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h b/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h
> > > >
> > > > [...]
> > > >
> > > > > > > > > +/* Emulate AT_MINSIGSTKSZ with XSAVE. */
> > > > > > > > > +
> > > > > > > > > +static inline void
> > > > > > > > > +dl_check_minsigstacksize (void)
> > > > > > > > > +{
> > > > > > > > > +  /* NB: Default to a constant MINSIGSTKSZ.  */
> > > > > > > > > +  _Static_assert (__builtin_constant_p (MINSIGSTKSZ),
> > > > > > > > > +           "MINSIGSTKSZ is constant");
> > > > > > > > > +  /* Return if AT_MINSIGSTKSZ is provide by kernel.  */
> > > > > > > > > +  if (GLRO(dl_minsigstacksize) != MINSIGSTKSZ)
> > > > > > > > > +    return;
> > > > > > > >
> > > > > > > > Couldn't the kernel actually yield MINSIGSTKSZ or a smaller value, say,
> > > > > > > > if running on hardware that doesn't have AVX-512?
> > > > > > > >
> > > > > > > It is OK for MINSIGSTKSZ > AT_MINSIGSTKSZ.  For _SC_SIGSTKSZ_SOURCE,
> > > > > > > dynamic MINSIGSTKSZ is defined as sysconf (_SC_SIGSTKSZ) which is
> > > > > > >
> > > > > > > MAX (SIGSTKSZ, MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4)
> > > > > > >
> > > > > > > and dynamic MINSIGSTKSZ is always > MINSIGSTKSZ.
> > > > > > >
> > > > > > > > We might want a separate flag to indicate whether we obtained a value
> > > > > > > > from the auxv, rather relying on MINSIGSTKSZ having this magic meaning.
> > > > > > >
> > > > > > > AT_MINSIGSTKSZ is the only way for GLRO(dl_minsigstacksize) != MINSIGSTKSZ.
> > > > > >
> > > > > > Yes, but reading AT_MINSIGSTKSZ doesn't guarantee that
> > > > > > GLRO(dl_minsigstkszsize) != MINSIGSTKSZ, no?
> > > > > >
> > > > > > What if the value reported for AT_MINSIGSTKSZ is actually the same as
> > > > > > MINSIGSTKSZ?  This could be the case on some arches in future even if
> > > > > > it's never true today.  But the code here assumes that AT_MINSIGSTKSZ
> > > > > > wasn't available in this case, and reverts to a fallback guess.
> > > > >
> > > > > Since the fallback tracks what the kernel does, if AT_MINSIGSTKSZ
> > > > > is 2KB, the fallback will be 2KB or slightly larger.
> > > >
> > > > Well, I guess that should be safe.  It still feels a bit like it works
> > > > by accident, but I may be being too paranoid.
> > >
> > > Let me work on that.
> > >
> >
> > Here is the updated patch to initialize GLRO(dl_minsigstacksize)
> > to 0 on x86.
> >
> 
> PING:
> 
> https://sourceware.org/pipermail/libc-alpha/2020-October/118843.html

Because the signal context doesn't actually fit in mcontext_t any more
(or, recursively, in ucontext_t), it would make sense to amend some of
the ucontext-related functions at the same time, and force people
towards this updated interface when building with _SC_SIGSTKSZ_SOURCE.

I'm aiming to get a draft proposal onto the list next week, if people
can wait for that before deciding whether the two sets of changes should
be coupled...

Cheers
---Dave
  

On Wed, Nov 4, 2020 at 8:50 AM Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Mon, Nov 02, 2020 at 07:06:13PM -0800, H.J. Lu via Libc-alpha wrote:
> > On Tue, Oct 20, 2020 at 11:19 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 7:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Tue, Oct 20, 2020 at 2:19 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > > > >
> > > > > On Mon, Oct 19, 2020 at 02:32:35PM -0700, H.J. Lu via Libc-alpha wrote:
> > > > > > On Mon, Oct 19, 2020 at 8:08 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 15, 2020 at 04:57:28AM -0700, H.J. Lu via Libc-alpha wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > > > diff --git a/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h b/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > > > +/* Emulate AT_MINSIGSTKSZ with XSAVE. */
> > > > > > > > > > +
> > > > > > > > > > +static inline void
> > > > > > > > > > +dl_check_minsigstacksize (void)
> > > > > > > > > > +{
> > > > > > > > > > +  /* NB: Default to a constant MINSIGSTKSZ.  */
> > > > > > > > > > +  _Static_assert (__builtin_constant_p (MINSIGSTKSZ),
> > > > > > > > > > +           "MINSIGSTKSZ is constant");
> > > > > > > > > > +  /* Return if AT_MINSIGSTKSZ is provide by kernel.  */
> > > > > > > > > > +  if (GLRO(dl_minsigstacksize) != MINSIGSTKSZ)
> > > > > > > > > > +    return;
> > > > > > > > >
> > > > > > > > > Couldn't the kernel actually yield MINSIGSTKSZ or a smaller value, say,
> > > > > > > > > if running on hardware that doesn't have AVX-512?
> > > > > > > > >
> > > > > > > > It is OK for MINSIGSTKSZ > AT_MINSIGSTKSZ.  For _SC_SIGSTKSZ_SOURCE,
> > > > > > > > dynamic MINSIGSTKSZ is defined as sysconf (_SC_SIGSTKSZ) which is
> > > > > > > >
> > > > > > > > MAX (SIGSTKSZ, MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4)
> > > > > > > >
> > > > > > > > and dynamic MINSIGSTKSZ is always > MINSIGSTKSZ.
> > > > > > > >
> > > > > > > > > We might want a separate flag to indicate whether we obtained a value
> > > > > > > > > from the auxv, rather relying on MINSIGSTKSZ having this magic meaning.
> > > > > > > >
> > > > > > > > AT_MINSIGSTKSZ is the only way for GLRO(dl_minsigstacksize) != MINSIGSTKSZ.
> > > > > > >
> > > > > > > Yes, but reading AT_MINSIGSTKSZ doesn't guarantee that
> > > > > > > GLRO(dl_minsigstkszsize) != MINSIGSTKSZ, no?
> > > > > > >
> > > > > > > What if the value reported for AT_MINSIGSTKSZ is actually the same as
> > > > > > > MINSIGSTKSZ?  This could be the case on some arches in future even if
> > > > > > > it's never true today.  But the code here assumes that AT_MINSIGSTKSZ
> > > > > > > wasn't available in this case, and reverts to a fallback guess.
> > > > > >
> > > > > > Since the fallback tracks what the kernel does, if AT_MINSIGSTKSZ
> > > > > > is 2KB, the fallback will be 2KB or slightly larger.
> > > > >
> > > > > Well, I guess that should be safe.  It still feels a bit like it works
> > > > > by accident, but I may be being too paranoid.
> > > >
> > > > Let me work on that.
> > > >
> > >
> > > Here is the updated patch to initialize GLRO(dl_minsigstacksize)
> > > to 0 on x86.
> > >
> >
> > PING:
> >
> > https://sourceware.org/pipermail/libc-alpha/2020-October/118843.html
>
> Because the signal context doesn't actually fit in mcontext_t any more
> (or, recursively, in ucontext_t), it would make sense to amend some of
> the ucontext-related functions at the same time, and force people
> towards this updated interface when building with _SC_SIGSTKSZ_SOURCE.

That may be useful.

> I'm aiming to get a draft proposal onto the list next week, if people
> can wait for that before deciding whether the two sets of changes should
> be coupled...
>

Thanks.
  

On Wed, Nov 4, 2020 at 9:48 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Nov 4, 2020 at 8:50 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Mon, Nov 02, 2020 at 07:06:13PM -0800, H.J. Lu via Libc-alpha wrote:
> > > On Tue, Oct 20, 2020 at 11:19 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > >
> > > > On Tue, Oct 20, 2020 at 7:59 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > >
> > > > > On Tue, Oct 20, 2020 at 2:19 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > > > > >
> > > > > > On Mon, Oct 19, 2020 at 02:32:35PM -0700, H.J. Lu via Libc-alpha wrote:
> > > > > > > On Mon, Oct 19, 2020 at 8:08 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Oct 15, 2020 at 04:57:28AM -0700, H.J. Lu via Libc-alpha wrote:
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > > > > diff --git a/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h b/sysdeps/unix/sysv/linux/x86/dl-minsigstacksize.h
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > > > > +/* Emulate AT_MINSIGSTKSZ with XSAVE. */
> > > > > > > > > > > +
> > > > > > > > > > > +static inline void
> > > > > > > > > > > +dl_check_minsigstacksize (void)
> > > > > > > > > > > +{
> > > > > > > > > > > +  /* NB: Default to a constant MINSIGSTKSZ.  */
> > > > > > > > > > > +  _Static_assert (__builtin_constant_p (MINSIGSTKSZ),
> > > > > > > > > > > +           "MINSIGSTKSZ is constant");
> > > > > > > > > > > +  /* Return if AT_MINSIGSTKSZ is provide by kernel.  */
> > > > > > > > > > > +  if (GLRO(dl_minsigstacksize) != MINSIGSTKSZ)
> > > > > > > > > > > +    return;
> > > > > > > > > >
> > > > > > > > > > Couldn't the kernel actually yield MINSIGSTKSZ or a smaller value, say,
> > > > > > > > > > if running on hardware that doesn't have AVX-512?
> > > > > > > > > >
> > > > > > > > > It is OK for MINSIGSTKSZ > AT_MINSIGSTKSZ.  For _SC_SIGSTKSZ_SOURCE,
> > > > > > > > > dynamic MINSIGSTKSZ is defined as sysconf (_SC_SIGSTKSZ) which is
> > > > > > > > >
> > > > > > > > > MAX (SIGSTKSZ, MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4)
> > > > > > > > >
> > > > > > > > > and dynamic MINSIGSTKSZ is always > MINSIGSTKSZ.
> > > > > > > > >
> > > > > > > > > > We might want a separate flag to indicate whether we obtained a value
> > > > > > > > > > from the auxv, rather relying on MINSIGSTKSZ having this magic meaning.
> > > > > > > > >
> > > > > > > > > AT_MINSIGSTKSZ is the only way for GLRO(dl_minsigstacksize) != MINSIGSTKSZ.
> > > > > > > >
> > > > > > > > Yes, but reading AT_MINSIGSTKSZ doesn't guarantee that
> > > > > > > > GLRO(dl_minsigstkszsize) != MINSIGSTKSZ, no?
> > > > > > > >
> > > > > > > > What if the value reported for AT_MINSIGSTKSZ is actually the same as
> > > > > > > > MINSIGSTKSZ?  This could be the case on some arches in future even if
> > > > > > > > it's never true today.  But the code here assumes that AT_MINSIGSTKSZ
> > > > > > > > wasn't available in this case, and reverts to a fallback guess.
> > > > > > >
> > > > > > > Since the fallback tracks what the kernel does, if AT_MINSIGSTKSZ
> > > > > > > is 2KB, the fallback will be 2KB or slightly larger.
> > > > > >
> > > > > > Well, I guess that should be safe.  It still feels a bit like it works
> > > > > > by accident, but I may be being too paranoid.
> > > > >
> > > > > Let me work on that.
> > > > >
> > > >
> > > > Here is the updated patch to initialize GLRO(dl_minsigstacksize)
> > > > to 0 on x86.
> > > >
> > >
> > > PING:
> > >
> > > https://sourceware.org/pipermail/libc-alpha/2020-October/118843.html
> >
> > Because the signal context doesn't actually fit in mcontext_t any more
> > (or, recursively, in ucontext_t), it would make sense to amend some of
> > the ucontext-related functions at the same time, and force people
> > towards this updated interface when building with _SC_SIGSTKSZ_SOURCE.
>
> That may be useful.
>
> > I'm aiming to get a draft proposal onto the list next week, if people
> > can wait for that before deciding whether the two sets of changes should
> > be coupled...
> >
>
> Thanks.

If there are no more comments, I will check in it together with

https://sourceware.org/pipermail/libc-alpha/2020-October/118707.html

on Friday, Nov 20.

Thanks.
  

On Wed, Nov 18, 2020 at 9:14 AM H.J. Lu via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> If there are no more comments, I will check in it together with
>
> https://sourceware.org/pipermail/libc-alpha/2020-October/118707.html
>
> on Friday, Nov 20.

I don't think you have consensus for this ABI-breaking change, and I
*certainly* don't think you should check it in without positive
agreement from at least one other person.

zw
  

On Wed, Nov 18, 2020 at 6:26 AM Zack Weinberg <zackw@panix.com> wrote:
>
> On Wed, Nov 18, 2020 at 9:14 AM H.J. Lu via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > If there are no more comments, I will check in it together with
> >
> > https://sourceware.org/pipermail/libc-alpha/2020-October/118707.html
> >
> > on Friday, Nov 20.
>
> I don't think you have consensus for this ABI-breaking change, and I
> *certainly* don't think you should check it in without positive
> agreement from at least one other person.
>

My patch adds _SC_SIGSTKSZ_SOURCE.   There are no changes
unless _SC_SIGSTKSZ_SOURCE is defined.

Dave, what do you think?
  

On Wed, Nov 18, 2020 at 9:40 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Nov 18, 2020 at 6:26 AM Zack Weinberg <zackw@panix.com> wrote:
> >
> > On Wed, Nov 18, 2020 at 9:14 AM H.J. Lu via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> > >
> > > If there are no more comments, I will check in it together with
> > >
> > > https://sourceware.org/pipermail/libc-alpha/2020-October/118707.html
> > >
> > > on Friday, Nov 20.
> >
> > I don't think you have consensus for this ABI-breaking change, and I
> > *certainly* don't think you should check it in without positive
> > agreement from at least one other person.
> >
>
> My patch adds _SC_SIGSTKSZ_SOURCE.   There are no changes
> unless _SC_SIGSTKSZ_SOURCE is defined.

OK, but do we have consensus on adding a new _*_SOURCE macro and a new
sysconf parameter ahead of their adoption by POSIX?  This is not a
rhetorical question, I have not been following that part of the
discussion closely enough to tell.

zw
  

On Wed, Nov 18, 2020 at 7:13 AM Zack Weinberg <zackw@panix.com> wrote:
>
> On Wed, Nov 18, 2020 at 9:40 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> > On Wed, Nov 18, 2020 at 6:26 AM Zack Weinberg <zackw@panix.com> wrote:
> > >
> > > On Wed, Nov 18, 2020 at 9:14 AM H.J. Lu via Libc-alpha
> > > <libc-alpha@sourceware.org> wrote:
> > > >
> > > > If there are no more comments, I will check in it together with
> > > >
> > > > https://sourceware.org/pipermail/libc-alpha/2020-October/118707.html
> > > >
> > > > on Friday, Nov 20.
> > >
> > > I don't think you have consensus for this ABI-breaking change, and I
> > > *certainly* don't think you should check it in without positive
> > > agreement from at least one other person.
> > >
> >
> > My patch adds _SC_SIGSTKSZ_SOURCE.   There are no changes
> > unless _SC_SIGSTKSZ_SOURCE is defined.
>
> OK, but do we have consensus on adding a new _*_SOURCE macro and a new
> sysconf parameter ahead of their adoption by POSIX?  This is not a

This is what Dave and I came up with.  This approach doesn't change ABI
unless it is requested by developers.

> rhetorical question, I have not been following that part of the
> discussion closely enough to tell.
>
> zw
  

* H. J. Lu via Libc-alpha:

> On Wed, Nov 18, 2020 at 7:13 AM Zack Weinberg <zackw@panix.com> wrote:
>>
>> On Wed, Nov 18, 2020 at 9:40 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>> > On Wed, Nov 18, 2020 at 6:26 AM Zack Weinberg <zackw@panix.com> wrote:
>> > >
>> > > On Wed, Nov 18, 2020 at 9:14 AM H.J. Lu via Libc-alpha
>> > > <libc-alpha@sourceware.org> wrote:
>> > > >
>> > > > If there are no more comments, I will check in it together with
>> > > >
>> > > > https://sourceware.org/pipermail/libc-alpha/2020-October/118707.html
>> > > >
>> > > > on Friday, Nov 20.
>> > >
>> > > I don't think you have consensus for this ABI-breaking change, and I
>> > > *certainly* don't think you should check it in without positive
>> > > agreement from at least one other person.
>> > >
>> >
>> > My patch adds _SC_SIGSTKSZ_SOURCE.   There are no changes
>> > unless _SC_SIGSTKSZ_SOURCE is defined.
>>
>> OK, but do we have consensus on adding a new _*_SOURCE macro and a new
>> sysconf parameter ahead of their adoption by POSIX?  This is not a
>
> This is what Dave and I came up with.  This approach doesn't change ABI
> unless it is requested by developers.

Did you mean ABI?

I think we should use _GNU_SOURCE. We can add another feature macro if
it's adopted by POSIX.
  

On Wed, Nov 18, 2020 at 04:20:44PM +0100, Florian Weimer wrote:
> * H. J. Lu via Libc-alpha:
> 
> > On Wed, Nov 18, 2020 at 7:13 AM Zack Weinberg <zackw@panix.com> wrote:
> >>
> >> On Wed, Nov 18, 2020 at 9:40 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >> > On Wed, Nov 18, 2020 at 6:26 AM Zack Weinberg <zackw@panix.com> wrote:
> >> > >
> >> > > On Wed, Nov 18, 2020 at 9:14 AM H.J. Lu via Libc-alpha
> >> > > <libc-alpha@sourceware.org> wrote:
> >> > > >
> >> > > > If there are no more comments, I will check in it together with
> >> > > >
> >> > > > https://sourceware.org/pipermail/libc-alpha/2020-October/118707.html
> >> > > >
> >> > > > on Friday, Nov 20.
> >> > >
> >> > > I don't think you have consensus for this ABI-breaking change, and I
> >> > > *certainly* don't think you should check it in without positive
> >> > > agreement from at least one other person.
> >> > >
> >> >
> >> > My patch adds _SC_SIGSTKSZ_SOURCE.   There are no changes
> >> > unless _SC_SIGSTKSZ_SOURCE is defined.
> >>
> >> OK, but do we have consensus on adding a new _*_SOURCE macro and a new
> >> sysconf parameter ahead of their adoption by POSIX?  This is not a
> >
> > This is what Dave and I came up with.  This approach doesn't change ABI
> > unless it is requested by developers.
> 
> Did you mean ABI?
> 
> I think we should use _GNU_SOURCE. We can add another feature macro if
> it's adopted by POSIX.

Having _GNU_SOURCE enable this feature will break existing source code.

It probably should go under _GNU_SOURCE in the long term, but is there
usually a transitional period when the change is opt-in via some feature
macro?

Otherwise, it feels like we would have a chicken-and-egg problem, with
nobody wanting to be the first adopter.


And also, what, if anything, should be done about the ucontext API?

The signal frame on a couple of arches now overruns the original
mcontext_t definition.

Ucontext seems to be in quite a broken state and is hard to fix because
of the lack of extensibility in the design.  Signals have also diverged
from setcontext() etc. -- some arches don't even encode the same data in
there, or put things in the same places between the two APIs.

I have some thoughts on what a better interface might look like --
basically separating the signal ucontext_t type from the setcontext()/
getcontext() etc. type, and providing accessors for the architectural
register state rather than just having a fixed struct definition for
mcontext_t.

But, there also may not be a lot of appetite for such a change, and
I can't see how it could be backwards compatible.

I can elaborate if people think it's worth discussing.

Cheers
---Dave
  

* Dave Martin:

> Having _GNU_SOURCE enable this feature will break existing source code.

Does this matter?

The code is already broken on a quickly increasing number of machines,
so it needs fixing anyway.  A compile-time error is probably
preferable to an obscure run-time failure.

> It probably should go under _GNU_SOURCE in the long term, but is there
> usually a transitional period when the change is opt-in via some feature
> macro?

Not always.  See the iszero and other macros in <math.h>, that also
broke existing sources (largely C++, so we worked around it by using
C++ features instead of macros).  There are also many older examples.

> I have some thoughts on what a better interface might look like --
> basically separating the signal ucontext_t type from the setcontext()/
> getcontext() etc. type, and providing accessors for the architectural
> register state rather than just having a fixed struct definition for
> mcontext_t.
>
> But, there also may not be a lot of appetite for such a change, and
> I can't see how it could be backwards compatible.
>
> I can elaborate if people think it's worth discussing.

I think Rich Felker wants to copy signal contexts around to implement
critical sections that can't be interrupted by a signal handler, I
think that would need this fully fixed.

But this is somewhat separate from fixing stack sizes to accomodate
kernel and hardware needs.

By the way, something that I tried to raise in the past, but wasn't
good at it: In the future, we may need a mechanism to reduce the
kernel stack size usage for legacy binaries, perhaps using
virtualization.  The last time I looked at this, the signal context
did not actually reduce when AVX-512 support was masked in a guest, I
think.  (Okay, this is largely for H.J.'s benefit.)  This also applies
to ld.so and its context save operation.  Fortunately, not many people
have run into this compatibility issue so far.  The case I remember
was an application that broke because it assumed zeros on the stack,
and the XSAVE trampoline wrote some non-zero bits there because it
reached much deeper into the stack.  But the user was happy when we
found the root cause and was able to add the missing initializer to
their code, so all was good.
  

On Wed, Nov 18, 2020 at 9:35 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Dave Martin:
>
> > Having _GNU_SOURCE enable this feature will break existing source code.
>
> Does this matter?
>
> The code is already broken on a quickly increasing number of machines,
> so it needs fixing anyway.  A compile-time error is probably
> preferable to an obscure run-time failure.

I can see points on both sides.  Either approach is better than the status quo.
Can we make a decision for glibc 2.33?

> > It probably should go under _GNU_SOURCE in the long term, but is there
> > usually a transitional period when the change is opt-in via some feature
> > macro?
>
> Not always.  See the iszero and other macros in <math.h>, that also
> broke existing sources (largely C++, so we worked around it by using
> C++ features instead of macros).  There are also many older examples.
>
> > I have some thoughts on what a better interface might look like --
> > basically separating the signal ucontext_t type from the setcontext()/
> > getcontext() etc. type, and providing accessors for the architectural
> > register state rather than just having a fixed struct definition for
> > mcontext_t.
> >
> > But, there also may not be a lot of appetite for such a change, and
> > I can't see how it could be backwards compatible.
> >
> > I can elaborate if people think it's worth discussing.
>
> I think Rich Felker wants to copy signal contexts around to implement
> critical sections that can't be interrupted by a signal handler, I
> think that would need this fully fixed.
>
> But this is somewhat separate from fixing stack sizes to accomodate
> kernel and hardware needs.
>
> By the way, something that I tried to raise in the past, but wasn't
> good at it: In the future, we may need a mechanism to reduce the
> kernel stack size usage for legacy binaries, perhaps using
> virtualization.  The last time I looked at this, the signal context
> did not actually reduce when AVX-512 support was masked in a guest, I
> think.  (Okay, this is largely for H.J.'s benefit.)  This also applies
> to ld.so and its context save operation.  Fortunately, not many people
> have run into this compatibility issue so far.  The case I remember
> was an application that broke because it assumed zeros on the stack,
> and the XSAVE trampoline wrote some non-zero bits there because it
> reached much deeper into the stack.  But the user was happy when we
> found the root cause and was able to add the missing initializer to
> their code, so all was good.

We, Intel, are investigating different approaches to improve signal handling
on machines with large XSAVE states.  One possibility is fast signal handler
where the programmer informs the kernel that only to save and restore a subset
of XSAVE states in the signal handler and update the compiler to enforce that.
  

On Wed, Nov 18, 2020 at 06:35:00PM +0100, Florian Weimer wrote:
> * Dave Martin:
> 
> > Having _GNU_SOURCE enable this feature will break existing source code.
> 
> Does this matter?
> 
> The code is already broken on a quickly increasing number of machines,
> so it needs fixing anyway.  A compile-time error is probably
> preferable to an obscure run-time failure.

Fair point, but I didn't want to be the one making it :)

If people are happy to go this way, I don't see a problem with it.
Distro maintainers may shout a bit while they deal with the fallout of
that glibc upgrade though.


> > It probably should go under _GNU_SOURCE in the long term, but is there
> > usually a transitional period when the change is opt-in via some feature
> > macro?
> 
> Not always.  See the iszero and other macros in <math.h>, that also
> broke existing sources (largely C++, so we worked around it by using
> C++ features instead of macros).  There are also many older examples.

I see.  Well, I guess that's not my call.


> > I have some thoughts on what a better interface might look like --
> > basically separating the signal ucontext_t type from the setcontext()/
> > getcontext() etc. type, and providing accessors for the architectural
> > register state rather than just having a fixed struct definition for
> > mcontext_t.
> >
> > But, there also may not be a lot of appetite for such a change, and
> > I can't see how it could be backwards compatible.
> >
> > I can elaborate if people think it's worth discussing.
> 
> I think Rich Felker wants to copy signal contexts around to implement
> critical sections that can't be interrupted by a signal handler, I
> think that would need this fully fixed.

Is rseq a more suitable way to do that sort of thing on new-ish Linux?
I guess a fallback may be needed for older / other kernels though.


I'm happy to follow up on here with my ucontext proposal, but it's quite
a big change.  I wasn't sure people would really be up for it...


> But this is somewhat separate from fixing stack sizes to accomodate
> kernel and hardware needs.

I was wondering whether we could feed in ucontext changes on the back of
the MINSIGSTKSZ stuff, so that if people want one they must adopt both.

Probably best to deal with it independently though -- as you say, the
ucontext breakage already exists.


> By the way, something that I tried to raise in the past, but wasn't
> good at it: In the future, we may need a mechanism to reduce the
> kernel stack size usage for legacy binaries, perhaps using
> virtualization.  The last time I looked at this, the signal context
> did not actually reduce when AVX-512 support was masked in a guest, I
> think.  (Okay, this is largely for H.J.'s benefit.)  This also applies
> to ld.so and its context save operation.  Fortunately, not many people
> have run into this compatibility issue so far.  The case I remember
> was an application that broke because it assumed zeros on the stack,
> and the XSAVE trampoline wrote some non-zero bits there because it
> reached much deeper into the stack.  But the user was happy when we
> found the root cause and was able to add the missing initializer to
> their code, so all was good.

For aarch64 an explicit prctl()/sysctl opt-in is needed to enable jumbo
vector registers before you see oversized signal frames, though I don't
think there is a similar control for AVX-512.

Even on aarch64, this interface is not very friendly though.  It might
be better to have some ELF attribute that ld.so or the libc startup can
arbitrate on and twiddle the appropriate switches.

Cheers
---Dave
  

The 11/18/2020 18:09, Dave Martin via Libc-alpha wrote:
> On Wed, Nov 18, 2020 at 06:35:00PM +0100, Florian Weimer wrote:
> > * Dave Martin:
> > > I have some thoughts on what a better interface might look like --
> > > basically separating the signal ucontext_t type from the setcontext()/
> > > getcontext() etc. type, and providing accessors for the architectural
> > > register state rather than just having a fixed struct definition for
> > > mcontext_t.
> > >
> > > But, there also may not be a lot of appetite for such a change, and
> > > I can't see how it could be backwards compatible.
> > >
> > > I can elaborate if people think it's worth discussing.
> > 
> > I think Rich Felker wants to copy signal contexts around to implement
> > critical sections that can't be interrupted by a signal handler, I
> > think that would need this fully fixed.
> 
> Is rseq a more suitable way to do that sort of thing on new-ish Linux?
> I guess a fallback may be needed for older / other kernels though.

rseq does not help with libc critical sections:

the point is not to restart the critical section
(which would require no side effect or mechanisms
to roll side effects back and that the section is
entirely written in asm between begin/end labels
so the kernel knows when the section is left),

but to let the critical section with all its side
effects complete and delay the signal handler
until then. (the slow and easy way to do this is
masking signals using syscalls around critical
sections, a fast solution needs signal wrapping
and saving the sigcontext.)

for example the entire malloc call can be a critical
section and an incomming signal delayed until malloc
completes. such solution allows hiding all libc
internal inconsistent state from user code so async
signal handlers can call all libc apis.

> For aarch64 an explicit prctl()/sysctl opt-in is needed to enable jumbo
> vector registers before you see oversized signal frames, though I don't
> think there is a similar control for AVX-512.
> 
> Even on aarch64, this interface is not very friendly though.  It might
> be better to have some ELF attribute that ld.so or the libc startup can
> arbitrate on and twiddle the appropriate switches.

we can probably mark binaries if they are large
vector length compatible. (but the incompatible
binaries have to be hunted down manually and all
we can do with them at dlopen time is to give a
nice dlerror in case the vlengh got increased)
  

On Thu, Nov 19, 2020 at 7:00 AM Szabolcs Nagy via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> The 11/18/2020 18:09, Dave Martin via Libc-alpha wrote:
> > On Wed, Nov 18, 2020 at 06:35:00PM +0100, Florian Weimer wrote:
> > > * Dave Martin:
> > > > I have some thoughts on what a better interface might look like --
> > > > basically separating the signal ucontext_t type from the setcontext()/
> > > > getcontext() etc. type, and providing accessors for the architectural
> > > > register state rather than just having a fixed struct definition for
> > > > mcontext_t.
> > > >
> > > > But, there also may not be a lot of appetite for such a change, and
> > > > I can't see how it could be backwards compatible.
> > > >
> > > > I can elaborate if people think it's worth discussing.
> > >
> > > I think Rich Felker wants to copy signal contexts around to implement
> > > critical sections that can't be interrupted by a signal handler, I
> > > think that would need this fully fixed.
> >
> > Is rseq a more suitable way to do that sort of thing on new-ish Linux?
> > I guess a fallback may be needed for older / other kernels though.
>
> rseq does not help with libc critical sections:
>
> the point is not to restart the critical section
> (which would require no side effect or mechanisms
> to roll side effects back and that the section is
> entirely written in asm between begin/end labels
> so the kernel knows when the section is left),
>
> but to let the critical section with all its side
> effects complete and delay the signal handler
> until then. (the slow and easy way to do this is
> masking signals using syscalls around critical
> sections, a fast solution needs signal wrapping
> and saving the sigcontext.)
>
> for example the entire malloc call can be a critical
> section and an incomming signal delayed until malloc
> completes. such solution allows hiding all libc
> internal inconsistent state from user code so async
> signal handlers can call all libc apis.
>
> > For aarch64 an explicit prctl()/sysctl opt-in is needed to enable jumbo
> > vector registers before you see oversized signal frames, though I don't
> > think there is a similar control for AVX-512.
> >
> > Even on aarch64, this interface is not very friendly though.  It might
> > be better to have some ELF attribute that ld.so or the libc startup can
> > arbitrate on and twiddle the appropriate switches.
>
> we can probably mark binaries if they are large
> vector length compatible. (but the incompatible
> binaries have to be hunted down manually and all
> we can do with them at dlopen time is to give a
> nice dlerror in case the vlengh got increased)

We can mark such binaries with GNU property.
  

On Thu, Nov 19, 2020 at 10:00 AM Szabolcs Nagy via Libc-alpha
<libc-alpha@sourceware.org> wrote:
...
> but to let the critical section with all its side
> effects complete and delay the signal handler
> until then. (the slow and easy way to do this is
> masking signals using syscalls around critical
> sections, a fast solution needs signal wrapping
> and saving the sigcontext.)
>
> for example the entire malloc call can be a critical
> section and an incomming signal delayed until malloc
> completes.

I read this and I'm wondering how impractical it would be to invent a
way to perform pthread_sigmask() operations without a system call.
Abstractly, the signal mask for each thread _could_ be placed in user
space at an ABI-specified location where the kernel can find it
(bottom of the thread stack, maybe, or an offset from the TLS base);
this doesn't make anything new possible (the kernel will just continue
to ignore the bits for SIGKILL, SIGSEGV, etc.) so it wouldn't be a
privilege violation.

Seems like that would be much easier than copying sigcontexts around,
and less likely to break code that thinks it knows stuff about signal
frames.

zw
  

* Zack Weinberg:

> I read this and I'm wondering how impractical it would be to invent a
> way to perform pthread_sigmask() operations without a system call.
> Abstractly, the signal mask for each thread _could_ be placed in user
> space at an ABI-specified location where the kernel can find it
> (bottom of the thread stack, maybe, or an offset from the TLS base);
> this doesn't make anything new possible (the kernel will just continue
> to ignore the bits for SIGKILL, SIGSEGV, etc.) so it wouldn't be a
> privilege violation.

Yes, this came up in the KTLS context.  The kernel also needs to set a
flag that indicates that unblocking requires a system call (so that a
pending signal can be delivered).  But it looks quite feasible to
implement.
  

On Thu, Nov 19, 2020 at 10:39:47AM -0500, Zack Weinberg wrote:
> On Thu, Nov 19, 2020 at 10:00 AM Szabolcs Nagy via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> ....
> > but to let the critical section with all its side
> > effects complete and delay the signal handler
> > until then. (the slow and easy way to do this is
> > masking signals using syscalls around critical
> > sections, a fast solution needs signal wrapping
> > and saving the sigcontext.)
> >
> > for example the entire malloc call can be a critical
> > section and an incomming signal delayed until malloc
> > completes.
> 
> I read this and I'm wondering how impractical it would be to invent a
> way to perform pthread_sigmask() operations without a system call.
> 
> Abstractly, the signal mask for each thread _could_ be placed in user
> space at an ABI-specified location where the kernel can find it

I've worked this out before, and it's mostly simple, but might have
some corner cases. Archs without an atomic (in single threaded sense)
64-bit (128-bit for MIPS) store operation can't update the mask
without tearing (a signal arriving between the partial writes could
see an inconsistent signal mask), so they couldn't use a naive form.
Having two slots you can alternate between (update the inactive one
then write the selector to switch which one is active) should work
though. There are probably lots of other minor issues like this but I
think they're all solveable.

> (bottom of the thread stack, maybe, or an offset from the TLS base);
> this doesn't make anything new possible (the kernel will just continue
> to ignore the bits for SIGKILL, SIGSEGV, etc.) so it wouldn't be a
> privilege violation.

It could either be managed by the vdso, or at an address registered by
a syscall (similar to robust_list) rather than some hard-coded
contract.

But in any case, even if this were added, it wouldn't meet the needs
for use case I proposed in musl. It doesn't solve the execve race with
abort (and can't, since it's just equivalent to optimizing signal
blocking we can already do, not introducing any new capability) and it
would not be available on past kernels, only future ones.

> Seems like that would be much easier than copying sigcontexts around,
> and less likely to break code that thinks it knows stuff about signal
> frames.

You don't need copy sigcontexts around. That's a misunderstanding of
how this works. It's just the siginfo that needs to be saved until the
deferred handler runs. The sigcontext passed to the handler is then
the synchronous context at the time it runs (which becomes the logical
time of signal delivery), not the earlier async context, and can
thereby be pretty much entirely uninitialized except for return
address and stack pointer. Everything else is a representation of
internal state from a libc function and can just be all zeros or
whatever.

Rich
  

On Thu, Nov 19, 2020 at 02:59:34PM +0000, Szabolcs Nagy via Libc-alpha wrote:
> The 11/18/2020 18:09, Dave Martin via Libc-alpha wrote:
> > On Wed, Nov 18, 2020 at 06:35:00PM +0100, Florian Weimer wrote:
> > > * Dave Martin:
> > > > I have some thoughts on what a better interface might look like --
> > > > basically separating the signal ucontext_t type from the setcontext()/
> > > > getcontext() etc. type, and providing accessors for the architectural
> > > > register state rather than just having a fixed struct definition for
> > > > mcontext_t.
> > > >
> > > > But, there also may not be a lot of appetite for such a change, and
> > > > I can't see how it could be backwards compatible.
> > > >
> > > > I can elaborate if people think it's worth discussing.
> > > 
> > > I think Rich Felker wants to copy signal contexts around to implement
> > > critical sections that can't be interrupted by a signal handler, I
> > > think that would need this fully fixed.
> > 
> > Is rseq a more suitable way to do that sort of thing on new-ish Linux?
> > I guess a fallback may be needed for older / other kernels though.
> 
> rseq does not help with libc critical sections:
> 
> the point is not to restart the critical section
> (which would require no side effect or mechanisms
> to roll side effects back and that the section is
> entirely written in asm between begin/end labels
> so the kernel knows when the section is left),
> 
> but to let the critical section with all its side
> effects complete and delay the signal handler
> until then. (the slow and easy way to do this is
> masking signals using syscalls around critical
> sections, a fast solution needs signal wrapping
> and saving the sigcontext.)
> 
> for example the entire malloc call can be a critical
> section and an incomming signal delayed until malloc
> completes. such solution allows hiding all libc
> internal inconsistent state from user code so async
> signal handlers can call all libc apis.

Isn't this a bit backwards.  This "makes" trivial signal handlers easier
to write, but this is a bit of a Trojan horse: precisely because signal
handlers can interrupt things, subtleties abound.  So, while there are
plenty of naive signal handlers out there, there are far fewer that are
genuinely trivial -- i.e., free from subtleties.

In any case, the problem of async signal safety remains: even if libc
uses internal locks to hide it, library functions in general may not.

A better approach would be to have function attributes that identify
code that may run in signal context and async-signal-safe functions, so
that the compiler can actually enforce that only reentrant functions are
called from signal context.

Finally, if a fault signal is delivered while blocked or ignored it
kills the process.  So handlers for fault signals raised by the kernel
still wouldn't be able to call random libc functions: to prevent sudden
death while in the middle of malloc etc., libc must not mask these
signals, and wouldn't be safely reentrant while handling then -- thus we
still have the problem we intended to solve.

This is ironic, since these signals are the _only_ signals that must be
handled using signal handlers.  Other signals can all be accepted by
other means, such as signalfd.

The other reason to use signal handlers is to minimise response latency
for asynchronous signals.  Masking signals in order to bulletproof
code that the signal handler probably isn't going to use anyway would
interfere with this goal.

> > For aarch64 an explicit prctl()/sysctl opt-in is needed to enable jumbo
> > vector registers before you see oversized signal frames, though I don't
> > think there is a similar control for AVX-512.
> > 
> > Even on aarch64, this interface is not very friendly though.  It might
> > be better to have some ELF attribute that ld.so or the libc startup can
> > arbitrate on and twiddle the appropriate switches.
> 
> we can probably mark binaries if they are large
> vector length compatible. (but the incompatible
> binaries have to be hunted down manually and all
> we can do with them at dlopen time is to give a
> nice dlerror in case the vlengh got increased)

Unless libc obeys the flag and actively prevents the setting of a larger
vector length.  It could do that, though I don't have a strong view on
whether it should...

dlopening a library with a smaller maximum vector length than the
established one would still be a problem though: we might have to fail
that.  But only in a process where some library uses SVE at all.  I
think there are ELF attributes for that too, no?

I think the incompatible binaries don't necessarily have to be hunted
down:  if a program was built with _GNU_SOURCE and a new enough C
library, it can't use the compile-time constant MINSIGSTKSZ etc., so we
can take the opportunity to mark the binary.  Other binaries can't be
assumed to be compatible with large vectors.

Cheers
---Dave
  

On Thu, Nov 19, 2020 at 11:16:57AM -0500, Rich Felker wrote:
> On Thu, Nov 19, 2020 at 10:39:47AM -0500, Zack Weinberg wrote:
> > On Thu, Nov 19, 2020 at 10:00 AM Szabolcs Nagy via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> > ....
> > > but to let the critical section with all its side
> > > effects complete and delay the signal handler
> > > until then. (the slow and easy way to do this is
> > > masking signals using syscalls around critical
> > > sections, a fast solution needs signal wrapping
> > > and saving the sigcontext.)
> > >
> > > for example the entire malloc call can be a critical
> > > section and an incomming signal delayed until malloc
> > > completes.
> > 
> > I read this and I'm wondering how impractical it would be to invent a
> > way to perform pthread_sigmask() operations without a system call.
> > 
> > Abstractly, the signal mask for each thread _could_ be placed in user
> > space at an ABI-specified location where the kernel can find it
> 
> I've worked this out before, and it's mostly simple, but might have
> some corner cases. Archs without an atomic (in single threaded sense)
> 64-bit (128-bit for MIPS) store operation can't update the mask
> without tearing (a signal arriving between the partial writes could
> see an inconsistent signal mask), so they couldn't use a naive form.
> Having two slots you can alternate between (update the inactive one
> then write the selector to switch which one is active) should work
> though. There are probably lots of other minor issues like this but I
> think they're all solveable.

I'd also thought about this, and I feel it does mostly work on systems
with atomic read-modify-writes of some sort, but there are a lot of
tricky details.

We might be able to move a lot of the "legacy" signal delivery logic to
userspace, eliminating all kernel entries except for asynchronous signal
delivery that comes out of the blue.

My instinct says that it would be better to have a more stripped down
kernel signal delivery mechanism that is actually designed as a
foundation for this.  Then all the "legacy" signal delivery logic could
be implemented in userspace.

POSIX signals are trying to meet a bunch of mutually defeating goals,
some of them historic and quite misguided -- so they're never going to
be all that satisfactory as a base to build on.

> 
> > (bottom of the thread stack, maybe, or an offset from the TLS base);
> > this doesn't make anything new possible (the kernel will just continue
> > to ignore the bits for SIGKILL, SIGSEGV, etc.) so it wouldn't be a
> > privilege violation.
> 
> It could either be managed by the vdso, or at an address registered by
> a syscall (similar to robust_list) rather than some hard-coded
> contract.
> 
> But in any case, even if this were added, it wouldn't meet the needs
> for use case I proposed in musl. It doesn't solve the execve race with
> abort (and can't, since it's just equivalent to optimizing signal
> blocking we can already do, not introducing any new capability) and it
> would not be available on past kernels, only future ones.
> 
> > Seems like that would be much easier than copying sigcontexts around,
> > and less likely to break code that thinks it knows stuff about signal
> > frames.
> 
> You don't need copy sigcontexts around. That's a misunderstanding of
> how this works. It's just the siginfo that needs to be saved until the
> deferred handler runs. The sigcontext passed to the handler is then
> the synchronous context at the time it runs (which becomes the logical
> time of signal delivery), not the earlier async context, and can
> thereby be pretty much entirely uninitialized except for return
> address and stack pointer. Everything else is a representation of
> internal state from a libc function and can just be all zeros or
> whatever.

I see.  I might still try to hack up a better ucontext interface if
(heh) I find myself with time on my hands, but I won't cite your needs
as a use case!

Cheers
  

On Thu, Nov 19, 2020 at 04:37:45PM +0000, Dave Martin wrote:
> On Thu, Nov 19, 2020 at 02:59:34PM +0000, Szabolcs Nagy via Libc-alpha wrote:
> > The 11/18/2020 18:09, Dave Martin via Libc-alpha wrote:
> > > On Wed, Nov 18, 2020 at 06:35:00PM +0100, Florian Weimer wrote:
> > > > * Dave Martin:
> > > > > I have some thoughts on what a better interface might look like --
> > > > > basically separating the signal ucontext_t type from the setcontext()/
> > > > > getcontext() etc. type, and providing accessors for the architectural
> > > > > register state rather than just having a fixed struct definition for
> > > > > mcontext_t.
> > > > >
> > > > > But, there also may not be a lot of appetite for such a change, and
> > > > > I can't see how it could be backwards compatible.
> > > > >
> > > > > I can elaborate if people think it's worth discussing.
> > > > 
> > > > I think Rich Felker wants to copy signal contexts around to implement
> > > > critical sections that can't be interrupted by a signal handler, I
> > > > think that would need this fully fixed.
> > > 
> > > Is rseq a more suitable way to do that sort of thing on new-ish Linux?
> > > I guess a fallback may be needed for older / other kernels though.
> > 
> > rseq does not help with libc critical sections:
> > 
> > the point is not to restart the critical section
> > (which would require no side effect or mechanisms
> > to roll side effects back and that the section is
> > entirely written in asm between begin/end labels
> > so the kernel knows when the section is left),
> > 
> > but to let the critical section with all its side
> > effects complete and delay the signal handler
> > until then. (the slow and easy way to do this is
> > masking signals using syscalls around critical
> > sections, a fast solution needs signal wrapping
> > and saving the sigcontext.)
> > 
> > for example the entire malloc call can be a critical
> > section and an incomming signal delayed until malloc
> > completes. such solution allows hiding all libc
> > internal inconsistent state from user code so async
> > signal handlers can call all libc apis.
> 
> Isn't this a bit backwards.  This "makes" trivial signal handlers easier
> to write, but this is a bit of a Trojan horse: precisely because signal
> handlers can interrupt things, subtleties abound.  So, while there are
> plenty of naive signal handlers out there, there are far fewer that are
> genuinely trivial -- i.e., free from subtleties.
> 
> In any case, the problem of async signal safety remains: even if libc
> uses internal locks to hide it, library functions in general may not.
> 
> A better approach..

You're missing a lot of the context of this to understand the
motivations. While the malloc example is interesting and making it
AS-safe as an extension is perhaps nice, the real motivation is places
where we're implementing an interface that is already required or at
least expected to be AS-safe. If such a function needs to access a
shared resource under lock, then presently that lock can never be
taken except with all application signals blocked. Otherwise, it's
possible that the AS-safe function needing the lock runs from a signal
handler that interrupted code that already held the lock. (Note:
recursive locks can't solve this, because a more complex version of
the same deadlock can be recreated with a pair of threads each
interrupted by signal handlers each needing the lock the other holds
in the interrupted code.)

One particularly nasty example is munmap (and mmap or mremap with
MAP_FIXED) which needs to synchronize with removal of robust mutexes
from the pending slot of the unlocking thread's robust_list. (Without
doing this, there are fundamental race conditions whereby async
termination of a process with robust mutexes can corrupt memory-mapped
files.) Presently, this makes it so our munmap, etc. are not AS-safe.
Note that they're not required to be AS-safe by POSIX, but most
programmers assume they are. In order to make them AS-safe, every
pthread_mutex_unlock on a robust mutex would need to mask and unmask
signals, so that the lock that inhibits changes to mappings couldn't
be held arbitrarily long and in deadlocking ways when interrupted by a
signal. But this masking/unmasking would introduce 2 syscalls to a
very hot path that's not supposed to have any.

However the original motivation for signal wrapping (which is what
enables deferral) is fixing an otherwise unfixable race between execve
and abort. In order to exit via SIGABRT when the signal starts out
with SIG_IGN disposition, abort has to change the disposition, and use
locks to prevent it from being changed back and prevent the changed
disposition from being observable. Morally, execve should take this
lock to prevent passing the changed disposition to a new process image
if it races with abort. However, the lock has to be an AS-safe one
(since abort has to be AS-safe), and execve can't mask signals or the
new mask would be passed to the new process image.

With the proposed wrapping/deferral, we can safely take the abort lock
without masking any signals. There's some additional machinery needed
here in the signal wrapper to make everything come out right, which
I'm glossing over because it's not the point here, but it all works
out and gives you an execve that can't leak internal state of abort.

> would be to have function attributes that identify
> code that may run in signal context and async-signal-safe functions, so
> that the compiler can actually enforce that only reentrant functions are
> called from signal context.
> 
> Finally, if a fault signal is delivered while blocked or ignored it
> kills the process.  So handlers for fault signals raised by the kernel
> still wouldn't be able to call random libc functions: to prevent sudden
> death while in the middle of malloc etc., libc must not mask these
> signals, and wouldn't be safely reentrant while handling then -- thus we
> still have the problem we intended to solve.

This has been raised plenty of times already. The desired outcome is
that the process terminate. This is a feature not a bug. If you really
don't want that, you *could* process synchronous signals (SIGSEGV
raised by invalid access) synchronously rather than deferring them,
possibly after releasing the lock for the critical section, but all
this does is de-harden the code. The *only* way a fault can happen in
this code is for the internal state to be corrupted in a manner in
which no further execution in the process context is safe.

> This is ironic, since these signals are the _only_ signals that must be
> handled using signal handlers.  Other signals can all be accepted by
> other means, such as signalfd.

I think you mean the only signals for which installing a handler is
meaningless because they're only delivered when the program already
has undefined behavior. (Technically you could use them in ways that
can have meaningfully defined model *from your own code* or even from
library code like memcpy or something if you've created mappings that
are intended to fault, but there is no defined or even reasonably
definable condition under which that access would end up taking place
in one of the critical sections under consideration here.)

> The other reason to use signal handlers is to minimise response latency
> for asynchronous signals.  Masking signals in order to bulletproof
> code that the signal handler probably isn't going to use anyway would
> interfere with this goal.

Yes, deferring signals is a matter of trading very small bounded
amounts of latency to get rid of unbounded latency and possible
deadlock from code that holds a lock being interrupted by a signal
handler. Note that the equivalent trade (with much much larger
latency) already happens anywhere you have to mask signals temporarily
via a sigprocmask.

Rich
  

The 11/19/2020 16:37, Dave Martin wrote:
> On Thu, Nov 19, 2020 at 02:59:34PM +0000, Szabolcs Nagy via Libc-alpha wrote:
> > the point is not to restart the critical section
> > (which would require no side effect or mechanisms
> > to roll side effects back and that the section is
> > entirely written in asm between begin/end labels
> > so the kernel knows when the section is left),
> > 
> > but to let the critical section with all its side
> > effects complete and delay the signal handler
> > until then. (the slow and easy way to do this is
> > masking signals using syscalls around critical
> > sections, a fast solution needs signal wrapping
> > and saving the sigcontext.)
> > 
> > for example the entire malloc call can be a critical
> > section and an incomming signal delayed until malloc
> > completes. such solution allows hiding all libc
> > internal inconsistent state from user code so async
> > signal handlers can call all libc apis.
> 
> Isn't this a bit backwards.  This "makes" trivial signal handlers easier
> to write, but this is a bit of a Trojan horse: precisely because signal
> handlers can interrupt things, subtleties abound.  So, while there are
> plenty of naive signal handlers out there, there are far fewer that are
> genuinely trivial -- i.e., free from subtleties.
> 
> In any case, the problem of async signal safety remains: even if libc
> uses internal locks to hide it, library functions in general may not.
> 
> A better approach would be to have function attributes that identify
> code that may run in signal context and async-signal-safe functions, so
> that the compiler can actually enforce that only reentrant functions are
> called from signal context.

this does not solve any of the real problems with
exposed libc internal state to signal handlers:

- linux has serious bugs that libc has to work around
(e.g. missing abort syscall: abort and execve must be
as-safe but abort must change SIGABRT handling that
must not be inherited by execve and execve cannot
block signals because that must not be inherited either.)

- signal handler can arbitrarily delay another thread
(because it may happen when libc internal lock is
held and user code in signal handlers may not return
immediately so a handler in one thread can break
real-time guarantees of another purely because of
libc internal details that must not be observable).

- there can be libc operations that must be as-safe
but internally needs to do non-as-safe operation
(e.g. tls access must be as-safe but in glibc it
may allocate or take internal locks.)

- compiler can generate non-as-safe libc calls into
signal handler code (e.g. memcpy is not required to
be as-safe but compiler can generate it, but a more
realistic example is the various sanitizer
instrumentations that break as-safety because print
diagnostic messages on failures).

- there are existing slow signal mask operations
around critical sections in libc which can be
improved by the proposed design.

> Finally, if a fault signal is delivered while blocked or ignored it
> kills the process.  So handlers for fault signals raised by the kernel
> still wouldn't be able to call random libc functions: to prevent sudden
> death while in the middle of malloc etc., libc must not mask these
> signals, and wouldn't be safely reentrant while handling then -- thus we
> still have the problem we intended to solve.

synchronous faults cannot happen in libc unless
the caller invoked ub so anything goes.

(i.e. not delaying those signals is fine)
  

On Thu, Nov 19, 2020 at 05:33:57PM +0000, Szabolcs Nagy via Libc-alpha wrote:
> The 11/19/2020 16:37, Dave Martin wrote:
> > On Thu, Nov 19, 2020 at 02:59:34PM +0000, Szabolcs Nagy via Libc-alpha wrote:
> > > the point is not to restart the critical section
> > > (which would require no side effect or mechanisms
> > > to roll side effects back and that the section is
> > > entirely written in asm between begin/end labels
> > > so the kernel knows when the section is left),
> > > 
> > > but to let the critical section with all its side
> > > effects complete and delay the signal handler
> > > until then. (the slow and easy way to do this is
> > > masking signals using syscalls around critical
> > > sections, a fast solution needs signal wrapping
> > > and saving the sigcontext.)
> > > 
> > > for example the entire malloc call can be a critical
> > > section and an incomming signal delayed until malloc
> > > completes. such solution allows hiding all libc
> > > internal inconsistent state from user code so async
> > > signal handlers can call all libc apis.
> > 
> > Isn't this a bit backwards.  This "makes" trivial signal handlers easier
> > to write, but this is a bit of a Trojan horse: precisely because signal
> > handlers can interrupt things, subtleties abound.  So, while there are
> > plenty of naive signal handlers out there, there are far fewer that are
> > genuinely trivial -- i.e., free from subtleties.
> > 
> > In any case, the problem of async signal safety remains: even if libc
> > uses internal locks to hide it, library functions in general may not.
> > 
> > A better approach would be to have function attributes that identify
> > code that may run in signal context and async-signal-safe functions, so
> > that the compiler can actually enforce that only reentrant functions are
> > called from signal context.
> 
> this does not solve any of the real problems with
> exposed libc internal state to signal handlers:
> 
> - linux has serious bugs that libc has to work around
> (e.g. missing abort syscall: abort and execve must be
> as-safe but abort must change SIGABRT handling that
> must not be inherited by execve and execve cannot
> block signals because that must not be inherited either.)

I can see the argument here, but this feels more like an implementation
detail of the execve() wrapper and libc's abort() implementation (?)

I guess this is analogous to spin_lock_irq() in the kernel: you take the
lock, but an irq that may be handled in the same (hardware) thread must
not attempt to take the same lock, since the spinlock code is not itself
reentrant for a given lock.  So, in the main thread you must also mask
IRQs before taking the lock.

> - signal handler can arbitrarily delay another thread
> (because it may happen when libc internal lock is
> held and user code in signal handlers may not return
> immediately so a handler in one thread can break
> real-time guarantees of another purely because of
> libc internal details that must not be observable).
> 
> - there can be libc operations that must be as-safe
> but internally needs to do non-as-safe operation
> (e.g. tls access must be as-safe but in glibc it
> may allocate or take internal locks.)

Seems reasonable.  I was concerned that these critical sections might be
long, potentially sleeping sequences of code.  It sounds like that's
definitley not the intention, so I guess it's workable.

The need to call sigprocmask() does suck here, and a way to get that
effect purely within userspace would be preferable.

> - compiler can generate non-as-safe libc calls into
> signal handler code (e.g. memcpy is not required to
> be as-safe but compiler can generate it, but a more

But presumably there is an understanding between the compiler and libcs
that it targets that the function called for out-of-line memcpy()s must
be as-safe.  By definition compiler output is not portable -- it assumes
a particular runtime environment.  So the lack of guranteed as-safety
for memcpy() in the standards is not necessarily an issue here.

(I wonder how may memcpy() implementations are really non-as-safe
though.  I guess that could happen if some kind of accelerator were used
for large copies.)

> realistic example is the various sanitizer
> instrumentations that break as-safety because print
> diagnostic messages on failures).

Well, you can do this in an as-safe way.  But it is unfortunate not to
be able to use the usual C functions to do the printing.

I generally assume that the sprintf() family of formats are at least
safe if you're not using locales or custom format specifiers.  I
probably shouldn't though, strictly speaking.


I suspect that unsafely printing diagnostics from handlers for fatal
signals is rather common in the wild, even if the standards say you
mustn't do it.

> - there are existing slow signal mask operations
> around critical sections in libc which can be
> improved by the proposed design.
> 
> > Finally, if a fault signal is delivered while blocked or ignored it
> > kills the process.  So handlers for fault signals raised by the kernel
> > still wouldn't be able to call random libc functions: to prevent sudden
> > death while in the middle of malloc etc., libc must not mask these
> > signals, and wouldn't be safely reentrant while handling then -- thus we
> > still have the problem we intended to solve.
> 
> synchronous faults cannot happen in libc unless
> the caller invoked ub so anything goes.
> 
> (i.e. not delaying those signals is fine)

Debatable.  The whole point of handling a fatal signal is to clean up
mess and/or capture diagnostic data.  By definition the process may be
in a partially unknown state but this point, but best efforts should
still be made to handle the signal -- otherwise handling the signal
doesn't make sense at all.  It doesn't seem right for this to fail
unpredictably depending on where the signal landed.

Cheers
---Dave
  

On Wed, Nov 18, 2020 at 10:09 AM Dave Martin via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Wed, Nov 18, 2020 at 06:35:00PM +0100, Florian Weimer wrote:
> > * Dave Martin:
> >
> > > Having _GNU_SOURCE enable this feature will break existing source code.
> >
> > Does this matter?
> >
> > The code is already broken on a quickly increasing number of machines,
> > so it needs fixing anyway.  A compile-time error is probably
> > preferable to an obscure run-time failure.
>
> Fair point, but I didn't want to be the one making it :)
>
> If people are happy to go this way, I don't see a problem with it.
> Distro maintainers may shout a bit while they deal with the fallout of
> that glibc upgrade though.
>
>
> > > It probably should go under _GNU_SOURCE in the long term, but is there
> > > usually a transitional period when the change is opt-in via some feature
> > > macro?
> >
> > Not always.  See the iszero and other macros in <math.h>, that also
> > broke existing sources (largely C++, so we worked around it by using
> > C++ features instead of macros).  There are also many older examples.
>
> I see.  Well, I guess that's not my call.
>

Can we reach a consensus for _GNU_SOURCE vs _SC_SIGSTKSZ_SOURCE?

How about we make _GNU_SOURCE to enable _SC_SIGSTKSZ_SOURCE and
check _SC_SIGSTKSZ_SOURCE != 0?  If _GNU_SOURCE triggers compilation
error and source codes can't be changed, we can add -D_SC_SIGSTKSZ_SOURCE=0
to disable _SC_SIGSTKSZ_SOURCE.


H.J.
  

* H. J. Lu:

> Can we reach a consensus for _GNU_SOURCE vs _SC_SIGSTKSZ_SOURCE?
>
> How about we make _GNU_SOURCE to enable _SC_SIGSTKSZ_SOURCE and
> check _SC_SIGSTKSZ_SOURCE != 0?  If _GNU_SOURCE triggers compilation
> error and source codes can't be changed, we can add
> -D_SC_SIGSTKSZ_SOURCE=0 to disable _SC_SIGSTKSZ_SOURCE.

I think the source code compatibility is much more obscure than the
other things we broke quite recently, even without deprecation.  Why
do we add the complexity in this case?  I feel it is disproportional.