Message ID | 303f46939bafa347bae62f9740837955ebbf43e9.1620838411.git.fweimer@redhat.com |
---|---|
State | New |
Headers | show |
Series | nptl: Move almost all remaining functions into libc | expand |
On 12/05/2021 13:58, Florian Weimer via Libc-alpha wrote: > This function is called once by pthread_create, before spawning > the first thread, and also within pthread_cancel. Since it sets up > cancellation handlers, place it along with the pthread_cancel > implementation within libc. Why does it need to be initialized by pthread_create as well? I think moving the initialization to pthread_cancel and only setting once when pthread_cancel is called (and not tying to __libc_single_threaded) is slight better since it avoids the setup of SIGCANCEL for programs that do not use cancellation. Also, it seems wrong that pthread_cancel.c it the implementation that setups both SIGCANCEL and SIGSETXID. It also seems wrong the rt_sigprocmask, SIG_UNBLOCK for pthread_cancel. I think it would be better to just setup the SIGCANCEL on pthread_cancel.c and move the the SIGSETXID to nptl/nptl_setxid.c.
* Adhemerval Zanella: > On 12/05/2021 13:58, Florian Weimer via Libc-alpha wrote: >> This function is called once by pthread_create, before spawning >> the first thread, and also within pthread_cancel. Since it sets up >> cancellation handlers, place it along with the pthread_cancel >> implementation within libc. > > Why does it need to be initialized by pthread_create as well? I think > moving the initialization to pthread_cancel and only setting once > when pthread_cancel is called (and not tying to __libc_single_threaded) > is slight better since it avoids the setup of SIGCANCEL for programs > that do not use cancellation. We need to unblock SIGCANCEL anyway, so that we can send the signal. This part can't be deferred until pthread_cancel is called. We could delay installing the SIGCANCEL handler because it's a global resource, assuming that unshare (CLONE_SIGHAND) will fail in the kernel. > Also, it seems wrong that pthread_cancel.c it the implementation > that setups both SIGCANCEL and SIGSETXID. It also seems wrong > the rt_sigprocmask, SIG_UNBLOCK for pthread_cancel. Hmm. Should we have separate initialization functions then? > I think it would be better to just setup the SIGCANCEL on pthread_cancel.c > and move the the SIGSETXID to nptl/nptl_setxid.c. As I said, we have to unblock SIGCANCEL as part of pthread_create anyway (for both the old and the new thread). Thanks, Florian
On 13/05/2021 17:51, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 12/05/2021 13:58, Florian Weimer via Libc-alpha wrote: >>> This function is called once by pthread_create, before spawning >>> the first thread, and also within pthread_cancel. Since it sets up >>> cancellation handlers, place it along with the pthread_cancel >>> implementation within libc. >> >> Why does it need to be initialized by pthread_create as well? I think >> moving the initialization to pthread_cancel and only setting once >> when pthread_cancel is called (and not tying to __libc_single_threaded) >> is slight better since it avoids the setup of SIGCANCEL for programs >> that do not use cancellation. > > We need to unblock SIGCANCEL anyway, so that we can send the signal. > This part can't be deferred until pthread_cancel is called. Yes, what it seems to be wrong is unblocking it on both pthread_create *and* on pthread_cancel. > > We could delay installing the SIGCANCEL handler because it's a global > resource, assuming that unshare (CLONE_SIGHAND) will fail in the kernel. > >> Also, it seems wrong that pthread_cancel.c it the implementation >> that setups both SIGCANCEL and SIGSETXID. It also seems wrong >> the rt_sigprocmask, SIG_UNBLOCK for pthread_cancel. > > Hmm. Should we have separate initialization functions then? I think so, it is more logical (pthread_cancel only handles SIGCANCEL).
* Adhemerval Zanella: > On 13/05/2021 17:51, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> On 12/05/2021 13:58, Florian Weimer via Libc-alpha wrote: >>>> This function is called once by pthread_create, before spawning >>>> the first thread, and also within pthread_cancel. Since it sets up >>>> cancellation handlers, place it along with the pthread_cancel >>>> implementation within libc. >>> >>> Why does it need to be initialized by pthread_create as well? I think >>> moving the initialization to pthread_cancel and only setting once >>> when pthread_cancel is called (and not tying to __libc_single_threaded) >>> is slight better since it avoids the setup of SIGCANCEL for programs >>> that do not use cancellation. >> >> We need to unblock SIGCANCEL anyway, so that we can send the signal. >> This part can't be deferred until pthread_cancel is called. > > Yes, what it seems to be wrong is unblocking it on both pthread_create > *and* on pthread_cancel. We need to unblock in pthread_cancel too, to support self-cancel in a single-threaded process. Do you know if we ever change the signal mask to enable/disable asynchronous cancellation? I don't think so. Then we could support self-cancel simply by calling the signal handler. (It would not matter whether the process is multi-threaded or not.) If we do that, I think it is simpler to do all the initialization in pthread_create. Thanks, Florian
On 14/05/2021 09:22, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 13/05/2021 17:51, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> On 12/05/2021 13:58, Florian Weimer via Libc-alpha wrote: >>>>> This function is called once by pthread_create, before spawning >>>>> the first thread, and also within pthread_cancel. Since it sets up >>>>> cancellation handlers, place it along with the pthread_cancel >>>>> implementation within libc. >>>> >>>> Why does it need to be initialized by pthread_create as well? I think >>>> moving the initialization to pthread_cancel and only setting once >>>> when pthread_cancel is called (and not tying to __libc_single_threaded) >>>> is slight better since it avoids the setup of SIGCANCEL for programs >>>> that do not use cancellation. >>> >>> We need to unblock SIGCANCEL anyway, so that we can send the signal. >>> This part can't be deferred until pthread_cancel is called. >> >> Yes, what it seems to be wrong is unblocking it on both pthread_create >> *and* on pthread_cancel. > > We need to unblock in pthread_cancel too, to support self-cancel in a > single-threaded process. We don't really need in fact, pthread_cancel can call __pthread_exit directly if the argument is THREAD_SELF. And with my canceltype and cancelstate refactor we don't even need the whole atomic bit set support. > > Do you know if we ever change the signal mask to enable/disable > asynchronous cancellation? I don't think so. Then we could support > self-cancel simply by calling the signal handler. (It would not matter > whether the process is multi-threaded or not.) > > If we do that, I think it is simpler to do all the initialization in > pthread_create. > > Thanks, > Florian >
On 5/14/21 9:29 AM, Adhemerval Zanella via Libc-alpha wrote: > > > On 14/05/2021 09:22, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> On 13/05/2021 17:51, Florian Weimer wrote: >>>> * Adhemerval Zanella: >>>> >>>>> On 12/05/2021 13:58, Florian Weimer via Libc-alpha wrote: >>>>>> This function is called once by pthread_create, before spawning >>>>>> the first thread, and also within pthread_cancel. Since it sets up >>>>>> cancellation handlers, place it along with the pthread_cancel >>>>>> implementation within libc. >>>>> >>>>> Why does it need to be initialized by pthread_create as well? I think >>>>> moving the initialization to pthread_cancel and only setting once >>>>> when pthread_cancel is called (and not tying to __libc_single_threaded) >>>>> is slight better since it avoids the setup of SIGCANCEL for programs >>>>> that do not use cancellation. >>>> >>>> We need to unblock SIGCANCEL anyway, so that we can send the signal. >>>> This part can't be deferred until pthread_cancel is called. >>> >>> Yes, what it seems to be wrong is unblocking it on both pthread_create >>> *and* on pthread_cancel. >> >> We need to unblock in pthread_cancel too, to support self-cancel in a >> single-threaded process. > > We don't really need in fact, pthread_cancel can call __pthread_exit > directly if the argument is THREAD_SELF. And with my canceltype and > cancelstate refactor we don't even need the whole atomic bit set > support. And POSIX in the RATIONALE says exactly this: ~~~ The special function also means that implementations are not obliged to implement cancellation with signals. ~~~ https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cancel.html >> Do you know if we ever change the signal mask to enable/disable >> asynchronous cancellation? I don't think so. Then we could support >> self-cancel simply by calling the signal handler. (It would not matter >> whether the process is multi-threaded or not.) >> >> If we do that, I think it is simpler to do all the initialization in >> pthread_create. >> >> Thanks, >> Florian >> >
* Carlos O'Donell: >> We don't really need in fact, pthread_cancel can call __pthread_exit >> directly if the argument is THREAD_SELF. And with my canceltype and >> cancelstate refactor we don't even need the whole atomic bit set >> support. > > And POSIX in the RATIONALE says exactly this: > ~~~ > The special function also means that implementations are not obliged > to implement cancellation with signals. > ~~~ > https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cancel.html My question was whether we use the signal mask to block cancellation, but I don't think that's the case (except in some cases where we block *all* signals for other reasons). But calling pthread_exit is probably not the right thing to do because pthread_cancel is itself not a cancellation point. In any case, I really don't want to change the implementation as part of this series. Thnaks, Florian
On 5/14/21 3:27 PM, Florian Weimer wrote: > * Carlos O'Donell: > >>> We don't really need in fact, pthread_cancel can call __pthread_exit >>> directly if the argument is THREAD_SELF. And with my canceltype and >>> cancelstate refactor we don't even need the whole atomic bit set >>> support. >> >> And POSIX in the RATIONALE says exactly this: >> ~~~ >> The special function also means that implementations are not obliged >> to implement cancellation with signals. >> ~~~ >> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cancel.html > > My question was whether we use the signal mask to block cancellation, > but I don't think that's the case (except in some cases where we block > *all* signals for other reasons). > > But calling pthread_exit is probably not the right thing to do because > pthread_cancel is itself not a cancellation point. > > In any case, I really don't want to change the implementation as part of > this series. I agree that not changing things during a refactor is an important part of avoiding regression. Adhemerval, Do we agree that this can be changed later?
On 14/05/2021 23:02, Carlos O'Donell wrote: > On 5/14/21 3:27 PM, Florian Weimer wrote: >> * Carlos O'Donell: >> >>>> We don't really need in fact, pthread_cancel can call __pthread_exit >>>> directly if the argument is THREAD_SELF. And with my canceltype and >>>> cancelstate refactor we don't even need the whole atomic bit set >>>> support. >>> >>> And POSIX in the RATIONALE says exactly this: >>> ~~~ >>> The special function also means that implementations are not obliged >>> to implement cancellation with signals. >>> ~~~ >>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cancel.html >> >> My question was whether we use the signal mask to block cancellation, >> but I don't think that's the case (except in some cases where we block >> *all* signals for other reasons). >> >> But calling pthread_exit is probably not the right thing to do because >> pthread_cancel is itself not a cancellation point. >> >> In any case, I really don't want to change the implementation as part of >> this series. > > I agree that not changing things during a refactor is an important > part of avoiding regression. > > Adhemerval, Do we agree that this can be changed later? > Fair enough, I have some patches I am working to try simplify the cancellation handling (and hopefully get the long-standing BZ#12683 fix).
diff --git a/elf/static-stubs.c b/elf/static-stubs.c index a8f24b9028..1752bc7372 100644 --- a/elf/static-stubs.c +++ b/elf/static-stubs.c @@ -33,6 +33,20 @@ _Unwind_Resume (struct _Unwind_Exception *exc __attribute__ ((unused))) abort (); } +_Unwind_Word +_Unwind_GetCFA (struct _Unwind_Context *ctx __attribute__ ((unused))) +{ + abort (); +} + +_Unwind_Reason_Code +_Unwind_ForcedUnwind (struct _Unwind_Exception *exc __attribute__ ((unused)), + _Unwind_Stop_Fn fn __attribute__ ((unused)), + void *ptr __attribute__ ((unused))) +{ + abort (); +} + _Unwind_Reason_Code __gcc_personality_v0 (int version __attribute__ ((unused)), _Unwind_Action actions __attribute__ ((unused)), diff --git a/nptl/Versions b/nptl/Versions index 6e2def1b4c..200055cffa 100644 --- a/nptl/Versions +++ b/nptl/Versions @@ -351,8 +351,6 @@ libc { tss_set; } GLIBC_PRIVATE { - __nptl_create_event; - __nptl_death_event; __default_pthread_attr; __default_pthread_attr_lock; __futex_abstimed_wait64; @@ -370,8 +368,11 @@ libc { __lll_trylock_elision; __lll_unlock_elision; __mutex_aconf; + __nptl_create_event; __nptl_deallocate_stack; __nptl_deallocate_tsd; + __nptl_death_event; + __nptl_deferred_init; __nptl_free_tcb; __nptl_nthreads; __nptl_setxid_sighandler; diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c index f4b86fbfaf..bc4831ac89 100644 --- a/nptl/nptl-init.c +++ b/nptl/nptl-init.c @@ -44,84 +44,9 @@ size_t __static_tls_align_m1; /* Version of the library, used in libthread_db to detect mismatches. */ static const char nptl_version[] __attribute_used__ = VERSION; -/* For asynchronous cancellation we use a signal. This is the handler. */ -static void -sigcancel_handler (int sig, siginfo_t *si, void *ctx) -{ - /* Safety check. It would be possible to call this function for - other signals and send a signal from another process. This is not - correct and might even be a security problem. Try to catch as - many incorrect invocations as possible. */ - if (sig != SIGCANCEL - || si->si_pid != __getpid() - || si->si_code != SI_TKILL) - return; - - struct pthread *self = THREAD_SELF; - - int oldval = THREAD_GETMEM (self, cancelhandling); - while (1) - { - /* We are canceled now. When canceled by another thread this flag - is already set but if the signal is directly send (internally or - from another process) is has to be done here. */ - int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK; - - if (oldval == newval || (oldval & EXITING_BITMASK) != 0) - /* Already canceled or exiting. */ - break; - - int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, - oldval); - if (curval == oldval) - { - /* Set the return value. */ - THREAD_SETMEM (self, result, PTHREAD_CANCELED); - - /* Make sure asynchronous cancellation is still enabled. */ - if ((newval & CANCELTYPE_BITMASK) != 0) - /* Run the registered destructors and terminate the thread. */ - __do_cancel (); - - break; - } - - oldval = curval; - } -} - - -/* When using __thread for this, we do it in libc so as not - to give libpthread its own TLS segment just for this. */ -extern void **__libc_dl_error_tsd (void) __attribute__ ((const)); - - void __pthread_initialize_minimal_internal (void) { - struct sigaction sa; - __sigemptyset (&sa.sa_mask); - - /* Install the cancellation signal handler. If for some reason we - cannot install the handler we do not abort. Maybe we should, but - it is only asynchronous cancellation which is affected. */ - sa.sa_sigaction = sigcancel_handler; - sa.sa_flags = SA_SIGINFO; - (void) __libc_sigaction (SIGCANCEL, &sa, NULL); - - /* Install the handle to change the threads' uid/gid. */ - sa.sa_sigaction = __nptl_setxid_sighandler; - sa.sa_flags = SA_SIGINFO | SA_RESTART; - (void) __libc_sigaction (SIGSETXID, &sa, NULL); - - /* The parent process might have left the signals blocked. Just in - case, unblock it. We reuse the signal mask in the sigaction - structure. It is already cleared. */ - __sigaddset (&sa.sa_mask, SIGCANCEL); - __sigaddset (&sa.sa_mask, SIGSETXID); - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sa.sa_mask, - NULL, __NSIG_BYTES); - /* Get the size of the static and alignment requirements for the TLS block. */ size_t static_tls_align; diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index a4a87cb549..e33b071a4d 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -262,18 +262,10 @@ libc_hidden_proto (__pthread_current_priority) extern void __pthread_unwind (__pthread_unwind_buf_t *__buf) - __cleanup_fct_attribute __attribute ((__noreturn__)) -#if !defined SHARED && !IS_IN (libpthread) - weak_function -#endif - ; + __cleanup_fct_attribute __attribute ((__noreturn__)); libc_hidden_proto (__pthread_unwind) extern void __pthread_unwind_next (__pthread_unwind_buf_t *__buf) - __cleanup_fct_attribute __attribute ((__noreturn__)) -#ifndef SHARED - weak_function -#endif - ; + __cleanup_fct_attribute __attribute ((__noreturn__)); /* NB: No hidden proto for __pthread_unwind_next: inside glibc, the legacy unwinding mechanism is used. */ @@ -301,6 +293,11 @@ __do_cancel (void) /* Internal prototypes. */ +/* One-time initialization activities before pthread_create spawns a + new thread. */ +void __nptl_deferred_init (void); +libc_hidden_proto (__nptl_deferred_init) + /* Deallocate a thread's stack after optionally making sure the thread descriptor is still valid. */ extern void __nptl_free_tcb (struct pthread *pd); diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c index e4ad602900..76daa791ea 100644 --- a/nptl/pthread_cancel.c +++ b/nptl/pthread_cancel.c @@ -26,6 +26,81 @@ #include <unwind-link.h> #include <stdio.h> #include <gnu/lib-names.h> +#include <sys/single_threaded.h> + +/* For asynchronous cancellation we use a signal. This is the handler. */ +static void +sigcancel_handler (int sig, siginfo_t *si, void *ctx) +{ + /* Safety check. It would be possible to call this function for + other signals and send a signal from another process. This is not + correct and might even be a security problem. Try to catch as + many incorrect invocations as possible. */ + if (sig != SIGCANCEL + || si->si_pid != __getpid() + || si->si_code != SI_TKILL) + return; + + struct pthread *self = THREAD_SELF; + + int oldval = THREAD_GETMEM (self, cancelhandling); + while (1) + { + /* We are canceled now. When canceled by another thread this flag + is already set but if the signal is directly send (internally or + from another process) is has to be done here. */ + int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK; + + if (oldval == newval || (oldval & EXITING_BITMASK) != 0) + /* Already canceled or exiting. */ + break; + + int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval, + oldval); + if (curval == oldval) + { + /* Set the return value. */ + THREAD_SETMEM (self, result, PTHREAD_CANCELED); + + /* Make sure asynchronous cancellation is still enabled. */ + if ((newval & CANCELTYPE_BITMASK) != 0) + /* Run the registered destructors and terminate the thread. */ + __do_cancel (); + + break; + } + + oldval = curval; + } +} + +void +__nptl_deferred_init (void) +{ + struct sigaction sa; + __sigemptyset (&sa.sa_mask); + + /* Install the cancellation signal handler. If for some reason we + cannot install the handler we do not abort. Maybe we should, but + it is only asynchronous cancellation which is affected. */ + sa.sa_sigaction = sigcancel_handler; + sa.sa_flags = SA_SIGINFO; + (void) __libc_sigaction (SIGCANCEL, &sa, NULL); + + /* Install the handle to change the threads' uid/gid. */ + sa.sa_sigaction = __nptl_setxid_sighandler; + sa.sa_flags = SA_SIGINFO | SA_RESTART; + (void) __libc_sigaction (SIGSETXID, &sa, NULL); + + /* The parent process might have left the signals blocked. Just in + case, unblock it. We reuse the signal mask in the sigaction + structure. It is already cleared. */ + __sigaddset (&sa.sa_mask, SIGCANCEL); + __sigaddset (&sa.sa_mask, SIGSETXID); + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sa.sa_mask, + NULL, __NSIG_BYTES); +} +libc_hidden_def (__nptl_deferred_init) int __pthread_cancel (pthread_t th) @@ -46,6 +121,15 @@ __pthread_cancel (pthread_t th) " must be installed for pthread_cancel to work\n"); } #endif + + /* Perform the deferred initialization if necessary, to install the + signal handler for a potential self-cancellation below. */ + if (__libc_single_threaded) + { + __nptl_deferred_init (); + __libc_single_threaded = 0; + } + int result = 0; int oldval; int newval; diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index 770656453d..43f3722f57 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -459,9 +459,13 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, { STACK_VARIABLES; - /* Avoid a data race in the multi-threaded case. */ + /* Avoid a data race in the multi-threaded case, and call the + deferred initialization only once. */ if (__libc_single_threaded) - __libc_single_threaded = 0; + { + __nptl_deferred_init (); + __libc_single_threaded = 0; + } const struct pthread_attr *iattr = (struct pthread_attr *) attr; union pthread_attr_transparent default_attr; diff --git a/sysdeps/unix/sysv/linux/ia64/static-stubs.c b/sysdeps/unix/sysv/linux/ia64/static-stubs.c new file mode 100644 index 0000000000..8bd594c2f1 --- /dev/null +++ b/sysdeps/unix/sysv/linux/ia64/static-stubs.c @@ -0,0 +1,26 @@ +/* Stub implementations of functions to link into statically linked + programs without needing libgcc_eh. ia64 version. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <elf/static-stubs.c> + +_Unwind_Word +_Unwind_GetBSP (struct _Unwind_Context *ctx) +{ + abort (); +}