Message ID | 20200326155633.18236-5-mathieu.desnoyers@efficios.com |
---|---|
State | New |
Headers | show |
Series | Restartable Sequences enablement | expand |
On 3/26/20 11:56 AM, Mathieu Desnoyers wrote: > From: Florian Weimer <fweimer@redhat.com> > > 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 see no other reviews of this patch so I'm not integrating other comments. I do not think the cost of starting up the thread is too much to pay. You should not be restarting threads that frequently i.e. use thread pools. Outstanding items for Florian: Why don't we remove parent_cancelhandling? Technically the patch looks correct, but I'd like to know about the removal of this variable. This descriptor is internal to glibc and opaque, and the length is not ABI, so we should be able to just remove it. What am I missing? > --- > nptl/descr.h | 10 +++++++--- > nptl/pthread_create.c | 46 +++++++++++++++++++++++-------------------- > 2 files changed, 32 insertions(+), 24 deletions(-) > > diff --git a/nptl/descr.h b/nptl/descr.h > index 9dcf480bdf..e1c7db5473 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; Why don't we remove this? Likewise for pid_ununsed [sic]. What am I missing? > > /* 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; OK. > + > /* 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 7c752d0f99..afd379e89a 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); OK. > - > #ifndef __ASSUME_SET_ROBUST_LIST > if (__set_robust_list_avail >= 0) > #endif > @@ -399,18 +394,6 @@ START_THREAD_DEFN > sizeof (struct robust_list_head)); > } > > - /* 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)) > - { > - sigset_t mask; > - __sigemptyset (&mask); > - __sigaddset (&mask, SIGCANCEL); > - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &mask, > - NULL, _NSIG / 8); > - } OK. Remove the ability to get cancelled during state initialization. > - > /* 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; > @@ -432,6 +415,12 @@ START_THREAD_DEFN > unwind_buf.priv.data.prev = NULL; > unwind_buf.priv.data.cleanup = NULL; > OK. > + __libc_signal_restore_set (&pd->sigmask); OK. We restore the mask after doing all the other work, and *then* we enable setxid from there onwards. Again a little more latency in handling these requests but it removes the race conditions. We are just about to call the user start function so this is reasonable place to start accepting cancellation. > + > + /* Allow setxid from now onwards. */ > + if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2)) > + futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE); OK. > + > if (__glibc_likely (! not_first_call)) > { > /* Store the new cleanup handler info. */ > @@ -722,10 +711,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); OK. > - > /* 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) > @@ -771,6 +756,21 @@ __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 all 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); OK. Allows us to block everything. > + > + /* 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; OK. > + > + /* Reset the cancellation signal mask in case this thread is running > + cancellation. */ > + __sigdelset (&pd->sigmask, SIGCANCEL); OK. Safe to do. > + > /* Start the thread. */ > if (__glibc_unlikely (report_thread_creation (pd))) > { > @@ -813,6 +813,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); OK. We increase signal latency here for the duration of the the thread creation. > + > if (__glibc_unlikely (retval != 0)) > { > if (thread_ran) >
* Carlos O'Donell via Libc-alpha: > Outstanding items for Florian: > > Why don't we remove parent_cancelhandling? > > Technically the patch looks correct, but I'd like to know about the > removal of this variable. This descriptor is internal to glibc and > opaque, and the length is not ABI, so we should be able to just remove > it. What am I missing? I was following this precedent: /* Ununsed. */ pid_t pid_ununsed; I don't have a strong preference here. It may make sense to collect a couple of these changes and drop all of them together, along with the _res removal.
On 4/24/20 4:35 PM, Florian Weimer wrote: > * Carlos O'Donell via Libc-alpha: > >> Outstanding items for Florian: >> >> Why don't we remove parent_cancelhandling? >> >> Technically the patch looks correct, but I'd like to know about the >> removal of this variable. This descriptor is internal to glibc and >> opaque, and the length is not ABI, so we should be able to just remove >> it. What am I missing? > > I was following this precedent: > > /* Ununsed. */ > pid_t pid_ununsed; > > I don't have a strong preference here. It may make sense to collect a > couple of these changes and drop all of them together, along with the > _res removal. > Thanks for the clarification. I thought this was the case. OK for master. Here is my sign-off for the patch. Reviewed-by: Carlos O'Donell <carlos@redhat.com>
On 24/04/2020 17:35, Florian Weimer wrote: > * Carlos O'Donell via Libc-alpha: > >> Outstanding items for Florian: >> >> Why don't we remove parent_cancelhandling? >> >> Technically the patch looks correct, but I'd like to know about the >> removal of this variable. This descriptor is internal to glibc and >> opaque, and the length is not ABI, so we should be able to just remove >> it. What am I missing? > > I was following this precedent: > > /* Ununsed. */ > pid_t pid_ununsed; > > I don't have a strong preference here. It may make sense to collect a > couple of these changes and drop all of them together, along with the > _res removal. > I have added this to try minimize the sanitizer requirement to handle different struct pthread sizes (and yes, it is a ABI abuse). But now I am not sure if this is worth, since we already handle the ABI specific bits in tcbhead_t.
diff --git a/nptl/descr.h b/nptl/descr.h index 9dcf480bdf..e1c7db5473 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 7c752d0f99..afd379e89a 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); - #ifndef __ASSUME_SET_ROBUST_LIST if (__set_robust_list_avail >= 0) #endif @@ -399,18 +394,6 @@ START_THREAD_DEFN sizeof (struct robust_list_head)); } - /* 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)) - { - sigset_t mask; - __sigemptyset (&mask); - __sigaddset (&mask, SIGCANCEL); - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &mask, - NULL, _NSIG / 8); - } - /* 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; @@ -432,6 +415,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. */ @@ -722,10 +711,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) @@ -771,6 +756,21 @@ __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 all 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; + + /* Reset the cancellation signal mask in case this thread is running + cancellation. */ + __sigdelset (&pd->sigmask, SIGCANCEL); + /* Start the thread. */ if (__glibc_unlikely (report_thread_creation (pd))) { @@ -813,6 +813,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)
From: Florian Weimer <fweimer@redhat.com> 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.) --- nptl/descr.h | 10 +++++++--- nptl/pthread_create.c | 46 +++++++++++++++++++++++-------------------- 2 files changed, 32 insertions(+), 24 deletions(-)