Patchwork nptl: Start new threads with all signals blocked [BZ #25098]

login
register
mail settings
Submitter Florian Weimer
Date Oct. 14, 2019, 12:33 p.m.
Message ID <87k197ik0o.fsf@oldenburg2.str.redhat.com>
Download mbox | patch
Permalink /patch/34936/
State New
Headers show

Comments

Florian Weimer - Oct. 14, 2019, 12:33 p.m.
New threads inherit the signal mask from the current thread.  This
means that signal handlers can run on the newly created thread
immediately after the kernel has created the userspace thread, even
before glibc has initialized the TCB.  Consequently, new threads can
observe uninitialized ctype data, among other things.

To address this, block all signals before starting the thread, and
pass the original signal mask to the start routine wrapper.  On the
new thread, first perform all thread initialization, and then unblock
signals.

The cost of doing this is two rt_sigprocmask system calls on the old
thread, and one rt_sigprocmask system call on the new thread.  (If
there was a way to clone a new thread with a signals disabled, this
could be brought down to one system call each.)  The thread descriptor
increases in size, too, and sigset_t is fairly large.  This increase
could be brought down by reusing space the in the descriptor which is
not needed before running user code, or by switching to an internal
sigset_t definition which only covers the signals supported by the
kernel definition.  (Part of the thread descriptor size increase is
already offset by reduced stack usage in the thread start wrapper
routine after this commit.)

Tested on aarch64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu,
s390x-linux-gnu, x86_64-linux-gnu.

-----
 nptl/descr.h          | 10 +++++++---
 nptl/pthread_create.c | 50 ++++++++++++++++++++++++++------------------------
 2 files changed, 33 insertions(+), 27 deletions(-)
Christian Brauner - Oct. 14, 2019, 1:32 p.m.
On Mon, Oct 14, 2019 at 02:33:43PM +0200, Florian Weimer wrote:
> New threads inherit the signal mask from the current thread.  This
> means that signal handlers can run on the newly created thread
> immediately after the kernel has created the userspace thread, even
> before glibc has initialized the TCB.  Consequently, new threads can
> observe uninitialized ctype data, among other things.
> 
> To address this, block all signals before starting the thread, and
> pass the original signal mask to the start routine wrapper.  On the
> new thread, first perform all thread initialization, and then unblock
> signals.
> 
> The cost of doing this is two rt_sigprocmask system calls on the old
> thread, and one rt_sigprocmask system call on the new thread.  (If
> there was a way to clone a new thread with a signals disabled, this

He, do I see a growing wishlist? :)

Christian
Florian Weimer - Oct. 15, 2019, 11:58 a.m.
* Christian Brauner:

> On Mon, Oct 14, 2019 at 02:33:43PM +0200, Florian Weimer wrote:
>> New threads inherit the signal mask from the current thread.  This
>> means that signal handlers can run on the newly created thread
>> immediately after the kernel has created the userspace thread, even
>> before glibc has initialized the TCB.  Consequently, new threads can
>> observe uninitialized ctype data, among other things.
>> 
>> To address this, block all signals before starting the thread, and
>> pass the original signal mask to the start routine wrapper.  On the
>> new thread, first perform all thread initialization, and then unblock
>> signals.
>> 
>> The cost of doing this is two rt_sigprocmask system calls on the old
>> thread, and one rt_sigprocmask system call on the new thread.  (If
>> there was a way to clone a new thread with a signals disabled, this
>
> He, do I see a growing wishlist? :)

Maybe.  I think the handler reset is more important because as
Adhemerval explained, it saves many more system calls.

Thanks,
Florian
Christian Brauner - Oct. 15, 2019, 12:03 p.m.
On Tue, Oct 15, 2019 at 01:58:53PM +0200, Florian Weimer wrote:
> * Christian Brauner:
> 
> > On Mon, Oct 14, 2019 at 02:33:43PM +0200, Florian Weimer wrote:
> >> New threads inherit the signal mask from the current thread.  This
> >> means that signal handlers can run on the newly created thread
> >> immediately after the kernel has created the userspace thread, even
> >> before glibc has initialized the TCB.  Consequently, new threads can
> >> observe uninitialized ctype data, among other things.
> >> 
> >> To address this, block all signals before starting the thread, and
> >> pass the original signal mask to the start routine wrapper.  On the
> >> new thread, first perform all thread initialization, and then unblock
> >> signals.
> >> 
> >> The cost of doing this is two rt_sigprocmask system calls on the old
> >> thread, and one rt_sigprocmask system call on the new thread.  (If
> >> there was a way to clone a new thread with a signals disabled, this
> >
> > He, do I see a growing wishlist? :)
> 
> Maybe.  I think the handler reset is more important because as
> Adhemerval explained, it saves many more system calls.

Right. If you feel comfortable reviewing it an Ack from you on the
kernel patch would be good.

Thanks!
Christian
Adhemerval Zanella Netto - Oct. 17, 2019, 6:33 p.m.
On 14/10/2019 09:33, Florian Weimer wrote:
> New threads inherit the signal mask from the current thread.  This
> means that signal handlers can run on the newly created thread
> immediately after the kernel has created the userspace thread, even
> before glibc has initialized the TCB.  Consequently, new threads can
> observe uninitialized ctype data, among other things.
> 
> To address this, block all signals before starting the thread, and
> pass the original signal mask to the start routine wrapper.  On the
> new thread, first perform all thread initialization, and then unblock
> signals.
> 
> The cost of doing this is two rt_sigprocmask system calls on the old
> thread, and one rt_sigprocmask system call on the new thread.  (If
> there was a way to clone a new thread with a signals disabled, this
> could be brought down to one system call each.)  The thread descriptor
> increases in size, too, and sigset_t is fairly large.  This increase
> could be brought down by reusing space the in the descriptor which is
> not needed before running user code, or by switching to an internal
> sigset_t definition which only covers the signals supported by the
> kernel definition.  (Part of the thread descriptor size increase is
> already offset by reduced stack usage in the thread start wrapper
> routine after this commit.)

I think this change worth parametrizing it on Linux to save some space
on the pthread_t structure, since it save about 120 bytes per thread
and it is unlikely Linux will eventually increase the signal size.

> 
> Tested on aarch64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu,
> s390x-linux-gnu, x86_64-linux-gnu.
> 
> -----
>  nptl/descr.h          | 10 +++++++---
>  nptl/pthread_create.c | 50 ++++++++++++++++++++++++++------------------------
>  2 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/nptl/descr.h b/nptl/descr.h
> index d3f863aa18..70d76bc63b 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -332,9 +332,8 @@ struct pthread
>    /* True if thread must stop at startup time.  */
>    bool stopped_start;
>  
> -  /* The parent's cancel handling at the time of the pthread_create
> -     call.  This might be needed to undo the effects of a cancellation.  */
> -  int parent_cancelhandling;
> +  /* Formerly used for dealing with cancellation.  */
> +  int parent_cancelhandling_unsed;

I think the idea of keeping the fields was that tools that abuse 
the ABI and access such metadata directly could work across glibc
versions. However, the C11 thread state already changed the internal
layout so I don't see much gain on keep this idea.  I would say to 
just remove the field altogether.

Based on this I send a patch to remove the pid_ununsed field
as well.

>  
>    /* Lock to synchronize access to the descriptor.  */
>    int lock;
> @@ -391,6 +390,11 @@ struct pthread
>    /* Resolver state.  */
>    struct __res_state res;
>  
> +  /* Signal mask for the new thread.  Used during thread startup to
> +     restore the signal mask.  (Threads are launched with all signals
> +     masked.)  */
> +  sigset_t sigmask;
> +
>    /* Indicates whether is a C11 thread created by thrd_creat.  */
>    bool c11;
>  
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 130937c3c4..940e5bdd4f 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -369,7 +369,6 @@ __free_tcb (struct pthread *pd)
>      }
>  }
>  
> -
>  /* Local function to start thread and handle cleanup.
>     createthread.c defines the macro START_THREAD_DEFN to the
>     declaration that its create_thread function will refer to, and
> @@ -385,10 +384,6 @@ START_THREAD_DEFN
>    /* Initialize pointers to locale data.  */
>    __ctype_init ();
>  
> -  /* Allow setxid from now onwards.  */
> -  if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
> -    futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
> -
>  #ifdef __NR_set_robust_list
>  # ifndef __ASSUME_SET_ROBUST_LIST
>    if (__set_robust_list_avail >= 0)

Ok.

> @@ -402,21 +397,6 @@ START_THREAD_DEFN
>      }
>  #endif
>  
> -#ifdef SIGCANCEL
> -  /* If the parent was running cancellation handlers while creating
> -     the thread the new thread inherited the signal mask.  Reset the
> -     cancellation signal mask.  */
> -  if (__glibc_unlikely (pd->parent_cancelhandling & CANCELING_BITMASK))
> -    {
> -      INTERNAL_SYSCALL_DECL (err);
> -      sigset_t mask;
> -      __sigemptyset (&mask);
> -      __sigaddset (&mask, SIGCANCEL);
> -      (void) INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_UNBLOCK, &mask,
> -			       NULL, _NSIG / 8);
> -    }
> -#endif
> -
>    /* This is where the try/finally block should be created.  For
>       compilers without that support we do use setjmp.  */
>    struct pthread_unwind_buf unwind_buf;

Ok.

> @@ -438,6 +418,12 @@ START_THREAD_DEFN
>    unwind_buf.priv.data.prev = NULL;
>    unwind_buf.priv.data.cleanup = NULL;
>  
> +  __libc_signal_restore_set (&pd->sigmask);
> +
> +  /* Allow setxid from now onwards.  */
> +  if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
> +    futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
> +
>    if (__glibc_likely (! not_first_call))
>      {
>        /* Store the new cleanup handler info.  */
> @@ -728,10 +714,6 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>    CHECK_THREAD_SYSINFO (pd);
>  #endif
>  
> -  /* Inform start_thread (above) about cancellation state that might
> -     translate into inherited signal state.  */
> -  pd->parent_cancelhandling = THREAD_GETMEM (THREAD_SELF, cancelhandling);
> -
>    /* Determine scheduling parameters for the thread.  */
>    if (__builtin_expect ((iattr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0, 0)
>        && (iattr->flags & (ATTR_FLAG_SCHED_SET | ATTR_FLAG_POLICY_SET)) != 0)
> @@ -777,6 +759,22 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>       ownership of PD (see CONCURRENCY NOTES above).  */
>    bool stopped_start = false; bool thread_ran = false;
>  
> +  /* Block alll signals, so that the new thread starts out with
> +     signals disabled.  This avoids race conditions in the thread
> +     startup.  */

s/alll/all

> +  sigset_t original_sigmask;
> +  __libc_signal_block_all (&original_sigmask);
> +
> +  /* Conceptually, the new thread needs to inherit the signal mask of
> +     this thread.  Therefore, it needs to restore the saved signal
> +     mask of this thread, so save it in the startup information.  */
> +  pd->sigmask = original_sigmask;
> +#ifdef SIGCANCEL
> +  /* Reset the cancellation signal mask in case this thread is running
> +     cancellation.  */
> +  __sigdelset (&pd->sigmask, SIGCANCEL);
> +#endif
> +

Do we still have a nptl target that does not have SIGCANCEL support?

>    /* Start the thread.  */
>    if (__glibc_unlikely (report_thread_creation (pd)))
>      {
> @@ -819,6 +817,10 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>      retval = create_thread (pd, iattr, &stopped_start,
>  			    STACK_VARIABLES_ARGS, &thread_ran);
>  
> +  /* Return to the previous signal mask, after creating the new
> +     thread.  */
> +  __libc_signal_restore_set (&original_sigmask);
> +
>    if (__glibc_unlikely (retval != 0))
>      {
>        if (thread_ran)
> 

Ok.
Florian Weimer - Oct. 17, 2019, 9:49 p.m.
* Adhemerval Zanella:

>> The cost of doing this is two rt_sigprocmask system calls on the old
>> thread, and one rt_sigprocmask system call on the new thread.  (If
>> there was a way to clone a new thread with a signals disabled, this
>> could be brought down to one system call each.)  The thread descriptor
>> increases in size, too, and sigset_t is fairly large.  This increase
>> could be brought down by reusing space the in the descriptor which is
>> not needed before running user code, or by switching to an internal
>> sigset_t definition which only covers the signals supported by the
>> kernel definition.  (Part of the thread descriptor size increase is
>> already offset by reduced stack usage in the thread start wrapper
>> routine after this commit.)
>
> I think this change worth parametrizing it on Linux to save some space
> on the pthread_t structure, since it save about 120 bytes per thread
> and it is unlikely Linux will eventually increase the signal size.

Do you see this as a precondition for this change?

> I think the idea of keeping the fields was that tools that abuse 
> the ABI and access such metadata directly could work across glibc
> versions. However, the C11 thread state already changed the internal
> layout so I don't see much gain on keep this idea.  I would say to 
> just remove the field altogether.

I can certainly do that.

>> +  /* Block alll signals, so that the new thread starts out with
>> +     signals disabled.  This avoids race conditions in the thread
>> +     startup.  */
>
> s/alll/all

Fixed locally.

>> +  sigset_t original_sigmask;
>> +  __libc_signal_block_all (&original_sigmask);
>> +
>> +  /* Conceptually, the new thread needs to inherit the signal mask of
>> +     this thread.  Therefore, it needs to restore the saved signal
>> +     mask of this thread, so save it in the startup information.  */
>> +  pd->sigmask = original_sigmask;
>> +#ifdef SIGCANCEL
>> +  /* Reset the cancellation signal mask in case this thread is running
>> +     cancellation.  */
>> +  __sigdelset (&pd->sigmask, SIGCANCEL);
>> +#endif
>> +
>
> Do we still have a nptl target that does not have SIGCANCEL support?

I don't think so.  I can remove all the #ifdef's and run a build with
build-many-glibcs.py tomorrow.

Thanks,
Florian
Adhemerval Zanella Netto - Oct. 18, 2019, 12:20 p.m.
On 17/10/2019 18:49, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> The cost of doing this is two rt_sigprocmask system calls on the old
>>> thread, and one rt_sigprocmask system call on the new thread.  (If
>>> there was a way to clone a new thread with a signals disabled, this
>>> could be brought down to one system call each.)  The thread descriptor
>>> increases in size, too, and sigset_t is fairly large.  This increase
>>> could be brought down by reusing space the in the descriptor which is
>>> not needed before running user code, or by switching to an internal
>>> sigset_t definition which only covers the signals supported by the
>>> kernel definition.  (Part of the thread descriptor size increase is
>>> already offset by reduced stack usage in the thread start wrapper
>>> routine after this commit.)
>>
>> I think this change worth parametrizing it on Linux to save some space
>> on the pthread_t structure, since it save about 120 bytes per thread
>> and it is unlikely Linux will eventually increase the signal size.
> 
> Do you see this as a precondition for this change?

I think we can add it as follow-up optimization, since it is orthogonal
to the change.

> 
>> I think the idea of keeping the fields was that tools that abuse 
>> the ABI and access such metadata directly could work across glibc
>> versions. However, the C11 thread state already changed the internal
>> layout so I don't see much gain on keep this idea.  I would say to 
>> just remove the field altogether.
> 
> I can certainly do that.
> 
>>> +  /* Block alll signals, so that the new thread starts out with
>>> +     signals disabled.  This avoids race conditions in the thread
>>> +     startup.  */
>>
>> s/alll/all
> 
> Fixed locally.
> 
>>> +  sigset_t original_sigmask;
>>> +  __libc_signal_block_all (&original_sigmask);
>>> +
>>> +  /* Conceptually, the new thread needs to inherit the signal mask of
>>> +     this thread.  Therefore, it needs to restore the saved signal
>>> +     mask of this thread, so save it in the startup information.  */
>>> +  pd->sigmask = original_sigmask;
>>> +#ifdef SIGCANCEL
>>> +  /* Reset the cancellation signal mask in case this thread is running
>>> +     cancellation.  */
>>> +  __sigdelset (&pd->sigmask, SIGCANCEL);
>>> +#endif
>>> +
>>
>> Do we still have a nptl target that does not have SIGCANCEL support?
> 
> I don't think so.  I can remove all the #ifdef's and run a build with
> build-many-glibcs.py tomorrow.
> 
> Thanks,
> Florian
>

Patch

diff --git a/nptl/descr.h b/nptl/descr.h
index d3f863aa18..70d76bc63b 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -332,9 +332,8 @@  struct pthread
   /* True if thread must stop at startup time.  */
   bool stopped_start;
 
-  /* The parent's cancel handling at the time of the pthread_create
-     call.  This might be needed to undo the effects of a cancellation.  */
-  int parent_cancelhandling;
+  /* Formerly used for dealing with cancellation.  */
+  int parent_cancelhandling_unsed;
 
   /* Lock to synchronize access to the descriptor.  */
   int lock;
@@ -391,6 +390,11 @@  struct pthread
   /* Resolver state.  */
   struct __res_state res;
 
+  /* Signal mask for the new thread.  Used during thread startup to
+     restore the signal mask.  (Threads are launched with all signals
+     masked.)  */
+  sigset_t sigmask;
+
   /* Indicates whether is a C11 thread created by thrd_creat.  */
   bool c11;
 
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 130937c3c4..940e5bdd4f 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -369,7 +369,6 @@  __free_tcb (struct pthread *pd)
     }
 }
 
-
 /* Local function to start thread and handle cleanup.
    createthread.c defines the macro START_THREAD_DEFN to the
    declaration that its create_thread function will refer to, and
@@ -385,10 +384,6 @@  START_THREAD_DEFN
   /* Initialize pointers to locale data.  */
   __ctype_init ();
 
-  /* Allow setxid from now onwards.  */
-  if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
-    futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
-
 #ifdef __NR_set_robust_list
 # ifndef __ASSUME_SET_ROBUST_LIST
   if (__set_robust_list_avail >= 0)
@@ -402,21 +397,6 @@  START_THREAD_DEFN
     }
 #endif
 
-#ifdef SIGCANCEL
-  /* If the parent was running cancellation handlers while creating
-     the thread the new thread inherited the signal mask.  Reset the
-     cancellation signal mask.  */
-  if (__glibc_unlikely (pd->parent_cancelhandling & CANCELING_BITMASK))
-    {
-      INTERNAL_SYSCALL_DECL (err);
-      sigset_t mask;
-      __sigemptyset (&mask);
-      __sigaddset (&mask, SIGCANCEL);
-      (void) INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_UNBLOCK, &mask,
-			       NULL, _NSIG / 8);
-    }
-#endif
-
   /* This is where the try/finally block should be created.  For
      compilers without that support we do use setjmp.  */
   struct pthread_unwind_buf unwind_buf;
@@ -438,6 +418,12 @@  START_THREAD_DEFN
   unwind_buf.priv.data.prev = NULL;
   unwind_buf.priv.data.cleanup = NULL;
 
+  __libc_signal_restore_set (&pd->sigmask);
+
+  /* Allow setxid from now onwards.  */
+  if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
+    futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
+
   if (__glibc_likely (! not_first_call))
     {
       /* Store the new cleanup handler info.  */
@@ -728,10 +714,6 @@  __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
   CHECK_THREAD_SYSINFO (pd);
 #endif
 
-  /* Inform start_thread (above) about cancellation state that might
-     translate into inherited signal state.  */
-  pd->parent_cancelhandling = THREAD_GETMEM (THREAD_SELF, cancelhandling);
-
   /* Determine scheduling parameters for the thread.  */
   if (__builtin_expect ((iattr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0, 0)
       && (iattr->flags & (ATTR_FLAG_SCHED_SET | ATTR_FLAG_POLICY_SET)) != 0)
@@ -777,6 +759,22 @@  __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
      ownership of PD (see CONCURRENCY NOTES above).  */
   bool stopped_start = false; bool thread_ran = false;
 
+  /* Block alll signals, so that the new thread starts out with
+     signals disabled.  This avoids race conditions in the thread
+     startup.  */
+  sigset_t original_sigmask;
+  __libc_signal_block_all (&original_sigmask);
+
+  /* Conceptually, the new thread needs to inherit the signal mask of
+     this thread.  Therefore, it needs to restore the saved signal
+     mask of this thread, so save it in the startup information.  */
+  pd->sigmask = original_sigmask;
+#ifdef SIGCANCEL
+  /* Reset the cancellation signal mask in case this thread is running
+     cancellation.  */
+  __sigdelset (&pd->sigmask, SIGCANCEL);
+#endif
+
   /* Start the thread.  */
   if (__glibc_unlikely (report_thread_creation (pd)))
     {
@@ -819,6 +817,10 @@  __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
     retval = create_thread (pd, iattr, &stopped_start,
 			    STACK_VARIABLES_ARGS, &thread_ran);
 
+  /* Return to the previous signal mask, after creating the new
+     thread.  */
+  __libc_signal_restore_set (&original_sigmask);
+
   if (__glibc_unlikely (retval != 0))
     {
       if (thread_ran)