nptl: Avoid fork handler lock for async-signal-safe fork [BZ #24161]

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]
Related show

Commit Message

Florian Weimer Feb. 4, 2019, 9 a.m.
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.

Comments

Florian Weimer Feb. 4, 2019, 9:07 a.m. | #1
* 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
Adhemerval Zanella Feb. 4, 2019, 8:26 p.m. | #2
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),
>
Florian Weimer Feb. 4, 2019, 8:33 p.m. | #3
* 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
Adhemerval Zanella Feb. 5, 2019, 1:08 p.m. | #4
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.
Adhemerval Zanella Feb. 6, 2019, 5:43 p.m. | #5
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.
Florian Weimer Feb. 8, 2019, 11:05 a.m. | #6
* 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),
Adhemerval Zanella Feb. 8, 2019, 11:34 a.m. | #7
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),
>

Patch

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),