| Message ID | 87o97sx70e.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nptl: Avoid fork handler lock for async-signal-safe fork [BZ #24161] | expand |
* Florian Weimer: > Commit 27761a1042daf01987e7d79636d0c41511c6df3c ("Refactor atfork > handlers") introduced a lock, atfork_lock, around fork handler list > accesses. It turns out that this lock occasionally results in > self-deadlocks in malloc/tst-mallocfork2: > > (gdb) bt > #0 __lll_lock_wait_private () > at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:63 > #1 0x00007f160c6f927a in __run_fork_handlers (who=(unknown: 209394016), > who@entry=atfork_run_prepare) at register-atfork.c:116 > #2 0x00007f160c6b7897 in __libc_fork () at ../sysdeps/nptl/fork.c:58 > #3 0x00000000004027d6 in sigusr1_handler (signo=<optimized out>) > at tst-mallocfork2.c:80 > #4 sigusr1_handler (signo=<optimized out>) at tst-mallocfork2.c:64 > #5 <signal handler called> > #6 0x00007f160c6f92e4 in __run_fork_handlers (who=who@entry=atfork_run_parent) > at register-atfork.c:136 > #7 0x00007f160c6b79a2 in __libc_fork () at ../sysdeps/nptl/fork.c:152 > #8 0x0000000000402567 in do_test () at tst-mallocfork2.c:156 > #9 0x0000000000402dd2 in support_test_main (argc=1, argv=0x7ffc81ef1ab0, > config=config@entry=0x7ffc81ef1970) at support_test_main.c:350 > #10 0x0000000000402362 in main (argc=<optimized out>, argv=<optimized out>) > at ../support/test-driver.c:168 > > If no locking happens in the single-threaded case (where fork is > expected to be async-signal-safe), this deadlock is avoided. > (pthread_atfork is not required to be async-signal-safe, so a fork > call from a signal handler interrupting pthread_atfork is not > a problem.) I should have mentioned that prior to this change, running a few copies of malloc/tst-mallocfork2 --direct would eventually result in a hang in one or two of them, within a few minutes. I ran the test in the same way for about an hour with the patch applied, and the hang did not occur annymore. Thanks, Florian
On 04/02/2019 07:00, Florian Weimer wrote: > Commit 27761a1042daf01987e7d79636d0c41511c6df3c ("Refactor atfork > handlers") introduced a lock, atfork_lock, around fork handler list > accesses. It turns out that this lock occasionally results in > self-deadlocks in malloc/tst-mallocfork2: > > (gdb) bt > #0 __lll_lock_wait_private () > at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:63 > #1 0x00007f160c6f927a in __run_fork_handlers (who=(unknown: 209394016), > who@entry=atfork_run_prepare) at register-atfork.c:116 > #2 0x00007f160c6b7897 in __libc_fork () at ../sysdeps/nptl/fork.c:58 > #3 0x00000000004027d6 in sigusr1_handler (signo=<optimized out>) > at tst-mallocfork2.c:80 > #4 sigusr1_handler (signo=<optimized out>) at tst-mallocfork2.c:64 > #5 <signal handler called> > #6 0x00007f160c6f92e4 in __run_fork_handlers (who=who@entry=atfork_run_parent) > at register-atfork.c:136 > #7 0x00007f160c6b79a2 in __libc_fork () at ../sysdeps/nptl/fork.c:152 > #8 0x0000000000402567 in do_test () at tst-mallocfork2.c:156 > #9 0x0000000000402dd2 in support_test_main (argc=1, argv=0x7ffc81ef1ab0, > config=config@entry=0x7ffc81ef1970) at support_test_main.c:350 > #10 0x0000000000402362 in main (argc=<optimized out>, argv=<optimized out>) > at ../support/test-driver.c:168 > > If no locking happens in the single-threaded case (where fork is > expected to be async-signal-safe), this deadlock is avoided. > (pthread_atfork is not required to be async-signal-safe, so a fork > call from a signal handler interrupting pthread_atfork is not > a problem.) Now it has bitten us again, how should we proceed to handle async-safeness for fork? Should we add signal handler wrapper so we can detect if fork is called in a signal handler and handle the locks properly? Or should we follow Rich suggestion in comment #21 at BZ#4737 and use a recursive and reentrant lock? > > 2019-02-04 Florian Weimer <fweimer@redhat.com> > > [BZ #24161] > * sysdeps/nptl/fork.h (__run_fork_handlers): Add multiple_threads > argument. > * nptl/register-atfork.c (__run_fork_handlers): Only perform > locking if the new multiple_threads argument is true. > * sysdeps/nptl/fork.c (__libc_fork): Pass multiple_threads to > __run_fork_handlers. > > diff --git a/nptl/register-atfork.c b/nptl/register-atfork.c > index bc797b761a..7759d50bd0 100644 > --- a/nptl/register-atfork.c > +++ b/nptl/register-atfork.c > @@ -107,13 +107,14 @@ __unregister_atfork (void *dso_handle) > } > > void > -__run_fork_handlers (enum __run_fork_handler_type who) > +__run_fork_handlers (enum __run_fork_handler_type who, _Bool multiple_threads) > { > struct fork_handler *runp; > > if (who == atfork_run_prepare) > { > - lll_lock (atfork_lock, LLL_PRIVATE); > + if (multiple_threads) > + lll_lock (atfork_lock, LLL_PRIVATE); > size_t sl = fork_handler_list_size (&fork_handlers); > for (size_t i = sl; i > 0; i--) > { > @@ -133,7 +134,8 @@ __run_fork_handlers (enum __run_fork_handler_type who) > else if (who == atfork_run_parent && runp->parent_handler) > runp->parent_handler (); > } > - lll_unlock (atfork_lock, LLL_PRIVATE); > + if (multiple_threads) > + lll_unlock (atfork_lock, LLL_PRIVATE); > } > } > I would prefer if handle it on fork instead, since the logic and requirement of locking depends on how we define the fork semantics regarding atfork handlers (and it already handler other locks in same manner). > diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c > index bd68f18b45..14b69a6f89 100644 > --- a/sysdeps/nptl/fork.c > +++ b/sysdeps/nptl/fork.c > @@ -55,7 +55,7 @@ __libc_fork (void) > but our current fork implementation is not. */ > bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads); > > - __run_fork_handlers (atfork_run_prepare); > + __run_fork_handlers (atfork_run_prepare, multiple_threads); > > /* If we are not running multiple threads, we do not have to > preserve lock state. If fork runs from a signal handler, only > @@ -134,7 +134,7 @@ __libc_fork (void) > __rtld_lock_initialize (GL(dl_load_lock)); > > /* Run the handlers registered for the child. */ > - __run_fork_handlers (atfork_run_child); > + __run_fork_handlers (atfork_run_child, multiple_threads); > } > else > { > @@ -149,7 +149,7 @@ __libc_fork (void) > } > > /* Run the handlers registered for the parent. */ > - __run_fork_handlers (atfork_run_parent); > + __run_fork_handlers (atfork_run_parent, multiple_threads); > } > > return pid; > diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h > index a1c3b26b68..e52b814e99 100644 > --- a/sysdeps/nptl/fork.h > +++ b/sysdeps/nptl/fork.h > @@ -52,9 +52,11 @@ enum __run_fork_handler_type > - atfork_run_child: run all the CHILD_HANDLER and unlocks the internal > lock. > - atfork_run_parent: run all the PARENT_HANDLER and unlocks the internal > - lock. */ > -extern void __run_fork_handlers (enum __run_fork_handler_type who) > - attribute_hidden; > + lock. > + > + Skip locking if !MULTIPLE_THREADS. */ > +extern void __run_fork_handlers (enum __run_fork_handler_type who, > + _Bool multiple_threads) attribute_hidden; > > /* C library side function to register new fork handlers. */ > extern int __register_atfork (void (*__prepare) (void), >
* Adhemerval Zanella: > Now it has bitten us again, how should we proceed to handle async-safeness > for fork? Should we add signal handler wrapper so we can detect if fork > is called in a signal handler and handle the locks properly? Or should > we follow Rich suggestion in comment #21 at BZ#4737 and use a recursive > and reentrant lock? For what? This particular lock, or all potentially relevant locks? The list of locks we'd need to fix is quite long. I think it's more promising to eliminate locks through STM techniques and also provide async-signal-safety this way. > I would prefer if handle it on fork instead, since the logic and requirement > of locking depends on how we define the fork semantics regarding atfork > handlers (and it already handler other locks in same manner). Do you suggest to expose the lock as a global variable, and lock and unlock it in the fork implementation, as appropriate? I'm not sure if that's an improvement. Thanks, Florian
On 04/02/2019 18:33, Florian Weimer wrote: > * Adhemerval Zanella: > >> Now it has bitten us again, how should we proceed to handle async-safeness >> for fork? Should we add signal handler wrapper so we can detect if fork >> is called in a signal handler and handle the locks properly? Or should >> we follow Rich suggestion in comment #21 at BZ#4737 and use a recursive >> and reentrant lock? > > For what? This particular lock, or all potentially relevant locks? The > list of locks we'd need to fix is quite long. My questioning was to make fork async-signal-safe for the generic way, not only for single-thread as we currently aim to. Another question is whether we really care to make fork async-signal-safe in all possible context, more specially in multithread environments. > > I think it's more promising to eliminate locks through STM techniques > and also provide async-signal-safety this way. There is an option I haven't considered yet. Not sure if it would be easier to fix the reentracy issue with fork. > >> I would prefer if handle it on fork instead, since the logic and requirement >> of locking depends on how we define the fork semantics regarding atfork >> handlers (and it already handler other locks in same manner). > > Do you suggest to expose the lock as a global variable, and lock and > unlock it in the fork implementation, as appropriate? I'm not sure if > that's an improvement. > Thinking again, you are right. I will review the original patch.
On 04/02/2019 07:00, Florian Weimer wrote: > Commit 27761a1042daf01987e7d79636d0c41511c6df3c ("Refactor atfork > handlers") introduced a lock, atfork_lock, around fork handler list > accesses. It turns out that this lock occasionally results in > self-deadlocks in malloc/tst-mallocfork2: > > (gdb) bt > #0 __lll_lock_wait_private () > at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:63 > #1 0x00007f160c6f927a in __run_fork_handlers (who=(unknown: 209394016), > who@entry=atfork_run_prepare) at register-atfork.c:116 > #2 0x00007f160c6b7897 in __libc_fork () at ../sysdeps/nptl/fork.c:58 > #3 0x00000000004027d6 in sigusr1_handler (signo=<optimized out>) > at tst-mallocfork2.c:80 > #4 sigusr1_handler (signo=<optimized out>) at tst-mallocfork2.c:64 > #5 <signal handler called> > #6 0x00007f160c6f92e4 in __run_fork_handlers (who=who@entry=atfork_run_parent) > at register-atfork.c:136 > #7 0x00007f160c6b79a2 in __libc_fork () at ../sysdeps/nptl/fork.c:152 > #8 0x0000000000402567 in do_test () at tst-mallocfork2.c:156 > #9 0x0000000000402dd2 in support_test_main (argc=1, argv=0x7ffc81ef1ab0, > config=config@entry=0x7ffc81ef1970) at support_test_main.c:350 > #10 0x0000000000402362 in main (argc=<optimized out>, argv=<optimized out>) > at ../support/test-driver.c:168 > > If no locking happens in the single-threaded case (where fork is > expected to be async-signal-safe), this deadlock is avoided. > (pthread_atfork is not required to be async-signal-safe, so a fork > call from a signal handler interrupting pthread_atfork is not > a problem.) > > 2019-02-04 Florian Weimer <fweimer@redhat.com> > > [BZ #24161] > * sysdeps/nptl/fork.h (__run_fork_handlers): Add multiple_threads > argument. > * nptl/register-atfork.c (__run_fork_handlers): Only perform > locking if the new multiple_threads argument is true. > * sysdeps/nptl/fork.c (__libc_fork): Pass multiple_threads to > __run_fork_handlers. > > diff --git a/nptl/register-atfork.c b/nptl/register-atfork.c > index bc797b761a..7759d50bd0 100644 > --- a/nptl/register-atfork.c > +++ b/nptl/register-atfork.c > @@ -107,13 +107,14 @@ __unregister_atfork (void *dso_handle) > } > > void > -__run_fork_handlers (enum __run_fork_handler_type who) > +__run_fork_handlers (enum __run_fork_handler_type who, _Bool multiple_threads) I think we should rename to enable_lock or something similar. > { > struct fork_handler *runp; > > if (who == atfork_run_prepare) > { > - lll_lock (atfork_lock, LLL_PRIVATE); > + if (multiple_threads) > + lll_lock (atfork_lock, LLL_PRIVATE); > size_t sl = fork_handler_list_size (&fork_handlers); > for (size_t i = sl; i > 0; i--) > { > @@ -133,7 +134,8 @@ __run_fork_handlers (enum __run_fork_handler_type who) > else if (who == atfork_run_parent && runp->parent_handler) > runp->parent_handler (); > } > - lll_unlock (atfork_lock, LLL_PRIVATE); > + if (multiple_threads) > + lll_unlock (atfork_lock, LLL_PRIVATE); > } > } > Ok. > diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c > index bd68f18b45..14b69a6f89 100644 > --- a/sysdeps/nptl/fork.c > +++ b/sysdeps/nptl/fork.c > @@ -55,7 +55,7 @@ __libc_fork (void) > but our current fork implementation is not. */ > bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads); We have the macro SINGLE_THREAD_P to select the best strategy depending of the architecture/ABI. > > - __run_fork_handlers (atfork_run_prepare); > + __run_fork_handlers (atfork_run_prepare, multiple_threads); > > /* If we are not running multiple threads, we do not have to > preserve lock state. If fork runs from a signal handler, only > @@ -134,7 +134,7 @@ __libc_fork (void) > __rtld_lock_initialize (GL(dl_load_lock)); > > /* Run the handlers registered for the child. */ > - __run_fork_handlers (atfork_run_child); > + __run_fork_handlers (atfork_run_child, multiple_threads); > } > else > { > @@ -149,7 +149,7 @@ __libc_fork (void) > } > > /* Run the handlers registered for the parent. */ > - __run_fork_handlers (atfork_run_parent); > + __run_fork_handlers (atfork_run_parent, multiple_threads); > } > > return pid; Ok. > diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h > index a1c3b26b68..e52b814e99 100644 > --- a/sysdeps/nptl/fork.h > +++ b/sysdeps/nptl/fork.h > @@ -52,9 +52,11 @@ enum __run_fork_handler_type > - atfork_run_child: run all the CHILD_HANDLER and unlocks the internal > lock. > - atfork_run_parent: run all the PARENT_HANDLER and unlocks the internal > - lock. */ > -extern void __run_fork_handlers (enum __run_fork_handler_type who) > - attribute_hidden; > + lock. > + > + Skip locking if !MULTIPLE_THREADS. */ > +extern void __run_fork_handlers (enum __run_fork_handler_type who, > + _Bool multiple_threads) attribute_hidden; > > /* C library side function to register new fork handlers. */ > extern int __register_atfork (void (*__prepare) (void), > Ok.
* Adhemerval Zanella: >> void >> -__run_fork_handlers (enum __run_fork_handler_type who) >> +__run_fork_handlers (enum __run_fork_handler_type who, _Bool multiple_threads) > > I think we should rename to enable_lock or something similar. Please consider the patch below. >> diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c >> index bd68f18b45..14b69a6f89 100644 >> --- a/sysdeps/nptl/fork.c >> +++ b/sysdeps/nptl/fork.c >> @@ -55,7 +55,7 @@ __libc_fork (void) >> but our current fork implementation is not. */ >> bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads); > > We have the macro SINGLE_THREAD_P to select the best strategy depending > of the architecture/ABI. Is this a suggestion for a separate change? Thanks, Florian nptl: Avoid fork handler lock for async-signal-safe fork [BZ #24161] Commit 27761a1042daf01987e7d79636d0c41511c6df3c ("Refactor atfork handlers") introduced a lock, atfork_lock, around fork handler list accesses. It turns out that this lock occasionally results in self-deadlocks in malloc/tst-mallocfork2: (gdb) bt #0 __lll_lock_wait_private () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:63 #1 0x00007f160c6f927a in __run_fork_handlers (who=(unknown: 209394016), who@entry=atfork_run_prepare) at register-atfork.c:116 #2 0x00007f160c6b7897 in __libc_fork () at ../sysdeps/nptl/fork.c:58 #3 0x00000000004027d6 in sigusr1_handler (signo=<optimized out>) at tst-mallocfork2.c:80 #4 sigusr1_handler (signo=<optimized out>) at tst-mallocfork2.c:64 #5 <signal handler called> #6 0x00007f160c6f92e4 in __run_fork_handlers (who=who@entry=atfork_run_parent) at register-atfork.c:136 #7 0x00007f160c6b79a2 in __libc_fork () at ../sysdeps/nptl/fork.c:152 #8 0x0000000000402567 in do_test () at tst-mallocfork2.c:156 #9 0x0000000000402dd2 in support_test_main (argc=1, argv=0x7ffc81ef1ab0, config=config@entry=0x7ffc81ef1970) at support_test_main.c:350 #10 0x0000000000402362 in main (argc=<optimized out>, argv=<optimized out>) at ../support/test-driver.c:168 If no locking happens in the single-threaded case (where fork is expected to be async-signal-safe), this deadlock is avoided. (pthread_atfork is not required to be async-signal-safe, so a fork call from a signal handler interrupting pthread_atfork is not a problem.) 2019-02-04 Florian Weimer <fweimer@redhat.com> [BZ #24161] * sysdeps/nptl/fork.h (__run_fork_handlers): Add multiple_threads argument. * nptl/register-atfork.c (__run_fork_handlers): Only perform locking if the new multiple_threads argument is true. * sysdeps/nptl/fork.c (__libc_fork): Pass multiple_threads to __run_fork_handlers. diff --git a/nptl/register-atfork.c b/nptl/register-atfork.c index bc797b761a..80a1becb5f 100644 --- a/nptl/register-atfork.c +++ b/nptl/register-atfork.c @@ -107,13 +107,14 @@ __unregister_atfork (void *dso_handle) } void -__run_fork_handlers (enum __run_fork_handler_type who) +__run_fork_handlers (enum __run_fork_handler_type who, _Bool do_locking) { struct fork_handler *runp; if (who == atfork_run_prepare) { - lll_lock (atfork_lock, LLL_PRIVATE); + if (do_locking) + lll_lock (atfork_lock, LLL_PRIVATE); size_t sl = fork_handler_list_size (&fork_handlers); for (size_t i = sl; i > 0; i--) { @@ -133,7 +134,8 @@ __run_fork_handlers (enum __run_fork_handler_type who) else if (who == atfork_run_parent && runp->parent_handler) runp->parent_handler (); } - lll_unlock (atfork_lock, LLL_PRIVATE); + if (do_locking) + lll_unlock (atfork_lock, LLL_PRIVATE); } } diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c index bd68f18b45..14b69a6f89 100644 --- a/sysdeps/nptl/fork.c +++ b/sysdeps/nptl/fork.c @@ -55,7 +55,7 @@ __libc_fork (void) but our current fork implementation is not. */ bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads); - __run_fork_handlers (atfork_run_prepare); + __run_fork_handlers (atfork_run_prepare, multiple_threads); /* If we are not running multiple threads, we do not have to preserve lock state. If fork runs from a signal handler, only @@ -134,7 +134,7 @@ __libc_fork (void) __rtld_lock_initialize (GL(dl_load_lock)); /* Run the handlers registered for the child. */ - __run_fork_handlers (atfork_run_child); + __run_fork_handlers (atfork_run_child, multiple_threads); } else { @@ -149,7 +149,7 @@ __libc_fork (void) } /* Run the handlers registered for the parent. */ - __run_fork_handlers (atfork_run_parent); + __run_fork_handlers (atfork_run_parent, multiple_threads); } return pid; diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h index a1c3b26b68..99ed76034b 100644 --- a/sysdeps/nptl/fork.h +++ b/sysdeps/nptl/fork.h @@ -52,9 +52,11 @@ enum __run_fork_handler_type - atfork_run_child: run all the CHILD_HANDLER and unlocks the internal lock. - atfork_run_parent: run all the PARENT_HANDLER and unlocks the internal - lock. */ -extern void __run_fork_handlers (enum __run_fork_handler_type who) - attribute_hidden; + lock. + + Perform locking only if DO_LOCKING. */ +extern void __run_fork_handlers (enum __run_fork_handler_type who, + _Bool do_locking) attribute_hidden; /* C library side function to register new fork handlers. */ extern int __register_atfork (void (*__prepare) (void),
On 08/02/2019 09:05, Florian Weimer wrote: > * Adhemerval Zanella: > >>> void >>> -__run_fork_handlers (enum __run_fork_handler_type who) >>> +__run_fork_handlers (enum __run_fork_handler_type who, _Bool multiple_threads) >> >> I think we should rename to enable_lock or something similar. > > Please consider the patch below. LGTM, thanks. > >>> diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c >>> index bd68f18b45..14b69a6f89 100644 >>> --- a/sysdeps/nptl/fork.c >>> +++ b/sysdeps/nptl/fork.c >>> @@ -55,7 +55,7 @@ __libc_fork (void) >>> but our current fork implementation is not. */ >>> bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads); >> >> We have the macro SINGLE_THREAD_P to select the best strategy depending >> of the architecture/ABI. > > Is this a suggestion for a separate change? Yeah, I will send this upstream. > > Thanks, > Florian > > nptl: Avoid fork handler lock for async-signal-safe fork [BZ #24161] > > Commit 27761a1042daf01987e7d79636d0c41511c6df3c ("Refactor atfork > handlers") introduced a lock, atfork_lock, around fork handler list > accesses. It turns out that this lock occasionally results in > self-deadlocks in malloc/tst-mallocfork2: > > (gdb) bt > #0 __lll_lock_wait_private () > at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:63 > #1 0x00007f160c6f927a in __run_fork_handlers (who=(unknown: 209394016), > who@entry=atfork_run_prepare) at register-atfork.c:116 > #2 0x00007f160c6b7897 in __libc_fork () at ../sysdeps/nptl/fork.c:58 > #3 0x00000000004027d6 in sigusr1_handler (signo=<optimized out>) > at tst-mallocfork2.c:80 > #4 sigusr1_handler (signo=<optimized out>) at tst-mallocfork2.c:64 > #5 <signal handler called> > #6 0x00007f160c6f92e4 in __run_fork_handlers (who=who@entry=atfork_run_parent) > at register-atfork.c:136 > #7 0x00007f160c6b79a2 in __libc_fork () at ../sysdeps/nptl/fork.c:152 > #8 0x0000000000402567 in do_test () at tst-mallocfork2.c:156 > #9 0x0000000000402dd2 in support_test_main (argc=1, argv=0x7ffc81ef1ab0, > config=config@entry=0x7ffc81ef1970) at support_test_main.c:350 > #10 0x0000000000402362 in main (argc=<optimized out>, argv=<optimized out>) > at ../support/test-driver.c:168 > > If no locking happens in the single-threaded case (where fork is > expected to be async-signal-safe), this deadlock is avoided. > (pthread_atfork is not required to be async-signal-safe, so a fork > call from a signal handler interrupting pthread_atfork is not > a problem.) > > 2019-02-04 Florian Weimer <fweimer@redhat.com> > > [BZ #24161] > * sysdeps/nptl/fork.h (__run_fork_handlers): Add multiple_threads > argument. > * nptl/register-atfork.c (__run_fork_handlers): Only perform > locking if the new multiple_threads argument is true. > * sysdeps/nptl/fork.c (__libc_fork): Pass multiple_threads to > __run_fork_handlers. > > diff --git a/nptl/register-atfork.c b/nptl/register-atfork.c > index bc797b761a..80a1becb5f 100644 > --- a/nptl/register-atfork.c > +++ b/nptl/register-atfork.c > @@ -107,13 +107,14 @@ __unregister_atfork (void *dso_handle) > } > > void > -__run_fork_handlers (enum __run_fork_handler_type who) > +__run_fork_handlers (enum __run_fork_handler_type who, _Bool do_locking) > { > struct fork_handler *runp; > > if (who == atfork_run_prepare) > { > - lll_lock (atfork_lock, LLL_PRIVATE); > + if (do_locking) > + lll_lock (atfork_lock, LLL_PRIVATE); > size_t sl = fork_handler_list_size (&fork_handlers); > for (size_t i = sl; i > 0; i--) > { > @@ -133,7 +134,8 @@ __run_fork_handlers (enum __run_fork_handler_type who) > else if (who == atfork_run_parent && runp->parent_handler) > runp->parent_handler (); > } > - lll_unlock (atfork_lock, LLL_PRIVATE); > + if (do_locking) > + lll_unlock (atfork_lock, LLL_PRIVATE); > } > } > > diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c > index bd68f18b45..14b69a6f89 100644 > --- a/sysdeps/nptl/fork.c > +++ b/sysdeps/nptl/fork.c > @@ -55,7 +55,7 @@ __libc_fork (void) > but our current fork implementation is not. */ > bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads); > > - __run_fork_handlers (atfork_run_prepare); > + __run_fork_handlers (atfork_run_prepare, multiple_threads); > > /* If we are not running multiple threads, we do not have to > preserve lock state. If fork runs from a signal handler, only > @@ -134,7 +134,7 @@ __libc_fork (void) > __rtld_lock_initialize (GL(dl_load_lock)); > > /* Run the handlers registered for the child. */ > - __run_fork_handlers (atfork_run_child); > + __run_fork_handlers (atfork_run_child, multiple_threads); > } > else > { > @@ -149,7 +149,7 @@ __libc_fork (void) > } > > /* Run the handlers registered for the parent. */ > - __run_fork_handlers (atfork_run_parent); > + __run_fork_handlers (atfork_run_parent, multiple_threads); > } > > return pid; > diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h > index a1c3b26b68..99ed76034b 100644 > --- a/sysdeps/nptl/fork.h > +++ b/sysdeps/nptl/fork.h > @@ -52,9 +52,11 @@ enum __run_fork_handler_type > - atfork_run_child: run all the CHILD_HANDLER and unlocks the internal > lock. > - atfork_run_parent: run all the PARENT_HANDLER and unlocks the internal > - lock. */ > -extern void __run_fork_handlers (enum __run_fork_handler_type who) > - attribute_hidden; > + lock. > + > + Perform locking only if DO_LOCKING. */ > +extern void __run_fork_handlers (enum __run_fork_handler_type who, > + _Bool do_locking) attribute_hidden; > > /* C library side function to register new fork handlers. */ > extern int __register_atfork (void (*__prepare) (void), >
diff --git a/nptl/register-atfork.c b/nptl/register-atfork.c index bc797b761a..7759d50bd0 100644 --- a/nptl/register-atfork.c +++ b/nptl/register-atfork.c @@ -107,13 +107,14 @@ __unregister_atfork (void *dso_handle) } void -__run_fork_handlers (enum __run_fork_handler_type who) +__run_fork_handlers (enum __run_fork_handler_type who, _Bool multiple_threads) { struct fork_handler *runp; if (who == atfork_run_prepare) { - lll_lock (atfork_lock, LLL_PRIVATE); + if (multiple_threads) + lll_lock (atfork_lock, LLL_PRIVATE); size_t sl = fork_handler_list_size (&fork_handlers); for (size_t i = sl; i > 0; i--) { @@ -133,7 +134,8 @@ __run_fork_handlers (enum __run_fork_handler_type who) else if (who == atfork_run_parent && runp->parent_handler) runp->parent_handler (); } - lll_unlock (atfork_lock, LLL_PRIVATE); + if (multiple_threads) + lll_unlock (atfork_lock, LLL_PRIVATE); } } diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c index bd68f18b45..14b69a6f89 100644 --- a/sysdeps/nptl/fork.c +++ b/sysdeps/nptl/fork.c @@ -55,7 +55,7 @@ __libc_fork (void) but our current fork implementation is not. */ bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads); - __run_fork_handlers (atfork_run_prepare); + __run_fork_handlers (atfork_run_prepare, multiple_threads); /* If we are not running multiple threads, we do not have to preserve lock state. If fork runs from a signal handler, only @@ -134,7 +134,7 @@ __libc_fork (void) __rtld_lock_initialize (GL(dl_load_lock)); /* Run the handlers registered for the child. */ - __run_fork_handlers (atfork_run_child); + __run_fork_handlers (atfork_run_child, multiple_threads); } else { @@ -149,7 +149,7 @@ __libc_fork (void) } /* Run the handlers registered for the parent. */ - __run_fork_handlers (atfork_run_parent); + __run_fork_handlers (atfork_run_parent, multiple_threads); } return pid; diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h index a1c3b26b68..e52b814e99 100644 --- a/sysdeps/nptl/fork.h +++ b/sysdeps/nptl/fork.h @@ -52,9 +52,11 @@ enum __run_fork_handler_type - atfork_run_child: run all the CHILD_HANDLER and unlocks the internal lock. - atfork_run_parent: run all the PARENT_HANDLER and unlocks the internal - lock. */ -extern void __run_fork_handlers (enum __run_fork_handler_type who) - attribute_hidden; + lock. + + Skip locking if !MULTIPLE_THREADS. */ +extern void __run_fork_handlers (enum __run_fork_handler_type who, + _Bool multiple_threads) attribute_hidden; /* C library side function to register new fork handlers. */ extern int __register_atfork (void (*__prepare) (void),
Commit 27761a1042daf01987e7d79636d0c41511c6df3c ("Refactor atfork handlers") introduced a lock, atfork_lock, around fork handler list accesses. It turns out that this lock occasionally results in self-deadlocks in malloc/tst-mallocfork2: (gdb) bt #0 __lll_lock_wait_private () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:63 #1 0x00007f160c6f927a in __run_fork_handlers (who=(unknown: 209394016), who@entry=atfork_run_prepare) at register-atfork.c:116 #2 0x00007f160c6b7897 in __libc_fork () at ../sysdeps/nptl/fork.c:58 #3 0x00000000004027d6 in sigusr1_handler (signo=<optimized out>) at tst-mallocfork2.c:80 #4 sigusr1_handler (signo=<optimized out>) at tst-mallocfork2.c:64 #5 <signal handler called> #6 0x00007f160c6f92e4 in __run_fork_handlers (who=who@entry=atfork_run_parent) at register-atfork.c:136 #7 0x00007f160c6b79a2 in __libc_fork () at ../sysdeps/nptl/fork.c:152 #8 0x0000000000402567 in do_test () at tst-mallocfork2.c:156 #9 0x0000000000402dd2 in support_test_main (argc=1, argv=0x7ffc81ef1ab0, config=config@entry=0x7ffc81ef1970) at support_test_main.c:350 #10 0x0000000000402362 in main (argc=<optimized out>, argv=<optimized out>) at ../support/test-driver.c:168 If no locking happens in the single-threaded case (where fork is expected to be async-signal-safe), this deadlock is avoided. (pthread_atfork is not required to be async-signal-safe, so a fork call from a signal handler interrupting pthread_atfork is not a problem.) 2019-02-04 Florian Weimer <fweimer@redhat.com> [BZ #24161] * sysdeps/nptl/fork.h (__run_fork_handlers): Add multiple_threads argument. * nptl/register-atfork.c (__run_fork_handlers): Only perform locking if the new multiple_threads argument is true. * sysdeps/nptl/fork.c (__libc_fork): Pass multiple_threads to __run_fork_handlers.