[3/8] posix: Consolidate fork implementation
Commit Message
The Linux nptl implementation is used as base for generic fork
implementation to handle the internal locks and mutexes. The
system specific bits are moved a new internal _Fork symbol.
(This new implementation will be used to provide a async-signal-safe
_Fork now that POSIX has clarified that fork might not be
async-signal-safe [1]).
For Hurd it means hat the __nss_database_fork_prepare_parent and
__nss_database_fork_subprocess will be run in a slight different
order.
[1] https://austingroupbugs.net/view.php?id=62
---
include/unistd.h | 2 +
posix/Makefile | 3 +-
posix/_Fork.c | 34 ++++++
posix/fork.c | 106 ++++++++++++++---
sysdeps/mach/hurd/{fork.c => _Fork.c} | 23 +---
sysdeps/nptl/_Fork.c | 64 +++++++++++
sysdeps/nptl/fork.c | 159 --------------------------
sysdeps/unix/sysv/linux/arch-fork.h | 3 +
8 files changed, 200 insertions(+), 194 deletions(-)
create mode 100644 posix/_Fork.c
rename sysdeps/mach/hurd/{fork.c => _Fork.c} (97%)
create mode 100644 sysdeps/nptl/_Fork.c
delete mode 100644 sysdeps/nptl/fork.c
Comments
* Adhemerval Zanella via Libc-alpha:
> diff --git a/sysdeps/nptl/_Fork.c b/sysdeps/nptl/_Fork.c
> new file mode 100644
> index 0000000000..9c701bc3d7
> --- /dev/null
> +++ b/sysdeps/nptl/_Fork.c
> +pid_t
> +_Fork (void)
> +{
> + pid_t pid = arch_fork (&THREAD_SELF->tid);
> + if (pid == 0)
> + {
> + struct pthread *self = THREAD_SELF;
> +
> + /* See __pthread_once. */
> + if (__fork_generation_pointer != NULL)
> + *__fork_generation_pointer += __PTHREAD_ONCE_FORK_GEN_INCR;
This should not be part of _Fork, I think. There is no need to do that
for a new interface.
I think this means that there has to be _Fork, the generic fork
implementation, and something between _Fork and fork that performs the
architecture-specific bits.
Hurd may also need this for its internal fork handlers if they are
supposed to run for fork, but not for _Fork.
Given that this affects the way functionality is split in the
consolidation, I have not reviewed this version of the patch further.
Thanks,
Florian
On 09/03/2021 07:58, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> diff --git a/sysdeps/nptl/_Fork.c b/sysdeps/nptl/_Fork.c
>> new file mode 100644
>> index 0000000000..9c701bc3d7
>> --- /dev/null
>> +++ b/sysdeps/nptl/_Fork.c
>
>> +pid_t
>> +_Fork (void)
>> +{
>> + pid_t pid = arch_fork (&THREAD_SELF->tid);
>> + if (pid == 0)
>> + {
>> + struct pthread *self = THREAD_SELF;
>> +
>> + /* See __pthread_once. */
>> + if (__fork_generation_pointer != NULL)
>> + *__fork_generation_pointer += __PTHREAD_ONCE_FORK_GEN_INCR;
>
> This should not be part of _Fork, I think. There is no need to do that
> for a new interface.
I think you are right, this is used solely for pthread_once and it is not
marked as async-signal-safe (so there is no need to keep it on Linux
_Fork implementation).
>
> I think this means that there has to be _Fork, the generic fork
> implementation, and something between _Fork and fork that performs the
> architecture-specific bits.
It should be simpler to add a inline hook on fork instead, so the
sequence would be:
posix/fork.c
[...]
pid_t pid = _Fork ();
if (pid == 0)
{
__fork_system_setup ();
[...]
}
And __fork_system_setup () is implemented on Linux to setup the required
fork generation for pthread_once (with an empty generic one).
>
> Hurd may also need this for its internal fork handlers if they are
> supposed to run for fork, but not for _Fork.
I did not dig into Hurd implementation, but I tried to make its _Fork
to no run pthread_atfork as well. It does use the old callback
mechanism:
sysdeps/mach/hurd/_Fork.c
41 /* Things that want to be called before we fork, to prepare the parent for
42 task_create, when the new child task will inherit our address space. */
43 DEFINE_HOOK (_hurd_fork_prepare_hook, (void));
44
45 /* Things that want to be called when we are forking, with the above all
46 locked. They are passed the task port of the child. The child process
47 is all set up except for doing proc_child, and has no threads yet. */
48 DEFINE_HOOK (_hurd_fork_setup_hook, (void));
49
50 /* Things to be run in the child fork. */
51 DEFINE_HOOK (_hurd_fork_child_hook, (void));
52
53 /* Things to be run in the parent fork. */
54 DEFINE_HOOK (_hurd_fork_parent_hook, (void));
sysdeps/mach/hurd/profil.c:213:text_set_element (_hurd_fork_prepare_hook, fork_profil_prepare);
sysdeps/mach/hurd/profil.c:255:text_set_element (_hurd_fork_child_hook, fork_profil_child);
sysdeps/mach/hurd/setitimer.c:377:text_set_element (_hurd_fork_child_hook, fork_itimer);
sysdeps/mach/hurd/profil.c:221:text_set_element (_hurd_fork_parent_hook, fork_profil_parent);
The hooks seems to be self-contained and there are only a handful of them,
so maybe Hurd developers could cleanup this up and run in directly
instead.
The __run_fork_handlers is only called from fork.c now.
>
> Given that this affects the way functionality is split in the
> consolidation, I have not reviewed this version of the patch further.
Alright, I will update the patch.
On Feb 02 2021, Adhemerval Zanella via Libc-alpha wrote:
> +pid_t
> +__libc_fork (void)
> {
> - __set_errno (ENOSYS);
> - return -1;
> + /* Determine if we are running multiple threads. We skip some fork
> + handlers in the single-thread case, to make fork safer to use in
> + signal handlers. POSIX requires that fork is async-signal-safe,
> + but our current fork implementation is not. */
The last sentence is now obsolete, isn't it?
Andreas.
On 11/03/2021 10:56, Andreas Schwab wrote:
> On Feb 02 2021, Adhemerval Zanella via Libc-alpha wrote:
>
>> +pid_t
>> +__libc_fork (void)
>> {
>> - __set_errno (ENOSYS);
>> - return -1;
>> + /* Determine if we are running multiple threads. We skip some fork
>> + handlers in the single-thread case, to make fork safer to use in
>> + signal handlers. POSIX requires that fork is async-signal-safe,
>> + but our current fork implementation is not. */
>
> The last sentence is now obsolete, isn't it?
It is, I will remove it.
@@ -135,6 +135,8 @@ libc_hidden_proto (__setresuid)
libc_hidden_proto (__setresgid)
extern __pid_t __vfork (void);
libc_hidden_proto (__vfork)
+extern __pid_t _Fork (void);
+libc_hidden_proto (_Fork);
extern int __ttyname_r (int __fd, char *__buf, size_t __buflen)
attribute_hidden;
extern int __isatty (int __fd) attribute_hidden;
@@ -39,7 +39,7 @@ routines := \
times \
wait waitpid wait3 wait4 waitid \
alarm sleep pause nanosleep \
- fork vfork _exit register-atfork \
+ fork _Fork vfork _exit register-atfork \
execve fexecve execv execle execl execvp execlp execvpe \
getpid getppid \
getuid geteuid getgid getegid getgroups setuid setgid group_member \
@@ -249,6 +249,7 @@ CFLAGS-execl.os = -fomit-frame-pointer
CFLAGS-execvp.os = -fomit-frame-pointer
CFLAGS-execlp.os = -fomit-frame-pointer
CFLAGS-nanosleep.c += -fexceptions -fasynchronous-unwind-tables
+CFLAGS-fork.c = $(libio-mtsafe)
tstgetopt-ARGS = -a -b -cfoobar --required foobar --optional=bazbug \
--none random --col --color --colour
new file mode 100644
@@ -0,0 +1,34 @@
+/* _Fork implementation. Generic 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 <errno.h>
+#include <unistd.h>
+
+/* Clone the calling process, creating an exact copy. Return -1 for errors,
+ 0 to the new process, and the process ID of the new process to the
+ old process.
+ Different than fork, this functions is marked as async-signal-safe by
+ POSIX (by Austin Group issue 62). */
+pid_t
+_Fork (void)
+{
+ __set_errno (ENOSYS);
+ return -1;
+}
+libc_hidden_def (_Fork)
+stub_warning (_Fork)
@@ -1,4 +1,5 @@
-/* Copyright (C) 1991-2021 Free Software Foundation, Inc.
+/* fork - create a child process.
+ Copyright (C) 1991-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
@@ -15,20 +16,99 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <unistd.h>
+#include <fork.h>
+#include <libio/libioP.h>
+#include <ldsodefs.h>
+#include <malloc/malloc-internal.h>
+#include <nss/nss_database.h>
+#include <register-atfork.h>
+#include <stdio-lock.h>
+#include <sys/single_threaded.h>
+static void
+fresetlockfiles (void)
+{
+ _IO_ITER i;
+
+ for (i = _IO_iter_begin(); i != _IO_iter_end(); i = _IO_iter_next(i))
+ if ((_IO_iter_file (i)->_flags & _IO_USER_LOCK) == 0)
+ _IO_lock_init (*((_IO_lock_t *) _IO_iter_file(i)->_lock));
+}
-/* Clone the calling process, creating an exact copy.
- Return -1 for errors, 0 to the new process,
- and the process ID of the new process to the old process. */
-int
-__fork (void)
+pid_t
+__libc_fork (void)
{
- __set_errno (ENOSYS);
- return -1;
+ /* Determine if we are running multiple threads. We skip some fork
+ handlers in the single-thread case, to make fork safer to use in
+ signal handlers. POSIX requires that fork is async-signal-safe,
+ but our current fork implementation is not. */
+ bool multiple_threads = __libc_single_threaded;
+
+ __run_fork_handlers (atfork_run_prepare, multiple_threads);
+
+ struct nss_database_data nss_database_data;
+
+ /* If we are not running multiple threads, we do not have to
+ preserve lock state. If fork runs from a signal handler, only
+ async-signal-safe functions can be used in the child. These data
+ structures are only used by unsafe functions, so their state does
+ not matter if fork was called from a signal handler. */
+ if (multiple_threads)
+ {
+ call_function_static_weak (__nss_database_fork_prepare_parent,
+ &nss_database_data);
+
+ _IO_list_lock ();
+
+ /* Acquire malloc locks. This needs to come last because fork
+ handlers may use malloc, and the libio list lock has an
+ indirect malloc dependency as well (via the getdelim
+ function). */
+ call_function_static_weak (__malloc_fork_lock_parent);
+ }
+
+ pid_t pid = _Fork ();
+
+ if (pid == 0)
+ {
+ /* Reset the lock state in the multi-threaded case. */
+ if (multiple_threads)
+ {
+ /* Release malloc locks. */
+ call_function_static_weak (__malloc_fork_unlock_child);
+
+ /* Reset the file list. These are recursive mutexes. */
+ fresetlockfiles ();
+
+ /* Reset locks in the I/O code. */
+ _IO_list_resetlock ();
+
+ call_function_static_weak (__nss_database_fork_subprocess,
+ &nss_database_data);
+ }
+
+ /* Reset the lock the dynamic loader uses to protect its data. */
+ __rtld_lock_initialize (GL(dl_load_lock));
+ }
+ else
+ {
+ /* Release acquired locks in the multi-threaded case. */
+ if (multiple_threads)
+ {
+ /* Release malloc locks, parent process variant. */
+ call_function_static_weak (__malloc_fork_unlock_parent);
+
+ /* We execute this even if the 'fork' call failed. */
+ _IO_list_unlock ();
+ }
+ }
+
+ /* Run the handlers registered for the parent. */
+ __run_fork_handlers (pid == 0 ? atfork_run_child : atfork_run_parent,
+ multiple_threads);
+
+ return pid;
}
+weak_alias (__libc_fork, __fork)
libc_hidden_def (__fork)
-stub_warning (fork)
-
-weak_alias (__fork, fork)
+weak_alias (__libc_fork, fork)
similarity index 97%
rename from sysdeps/mach/hurd/fork.c
rename to sysdeps/mach/hurd/_Fork.c
@@ -28,8 +28,6 @@
#include "hurdmalloc.h" /* XXX */
#include <tls.h>
#include <malloc/malloc-internal.h>
-#include <nss/nss_database.h>
-#include <register-atfork.h>
#undef __fork
@@ -57,16 +55,13 @@ DEFINE_HOOK (_hurd_fork_parent_hook, (void));
Return -1 for errors, 0 to the new process,
and the process ID of the new process to the old process. */
pid_t
-__fork (void)
+_Fork (void)
{
jmp_buf env;
pid_t pid;
size_t i;
error_t err;
struct hurd_sigstate *volatile ss;
- struct nss_database_data nss_database_data;
-
- __run_fork_handlers (atfork_run_prepare, true);
ss = _hurd_self_sigstate ();
__spin_lock (&ss->critical_section_lock);
@@ -106,9 +101,6 @@ __fork (void)
/* Run things that prepare for forking before we create the task. */
RUN_HOOK (_hurd_fork_prepare_hook, ());
- call_function_static_weak (__nss_database_fork_prepare_parent,
- &nss_database_data);
-
/* Lock things that want to be locked before we fork. */
{
void *const *p;
@@ -666,9 +658,6 @@ __fork (void)
_hurd_malloc_fork_child ();
call_function_static_weak (__malloc_fork_unlock_child);
- call_function_static_weak (__nss_database_fork_subprocess,
- &nss_database_data);
-
/* Run things that want to run in the child task to set up. */
RUN_HOOK (_hurd_fork_child_hook, ());
@@ -716,14 +705,6 @@ __fork (void)
_hurd_critical_section_unlock (ss);
- if (!err)
- {
- __run_fork_handlers (pid == 0 ? atfork_run_child : atfork_run_parent,
- true);
- }
-
return err ? __hurd_fail (err) : pid;
}
-libc_hidden_def (__fork)
-
-weak_alias (__fork, fork)
+libc_hidden_def (_Fork)
new file mode 100644
@@ -0,0 +1,64 @@
+/* _Fork implementation. Linux version.
+ Copyright (C) 2021 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
+
+ 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 <arch-fork.h>
+#include <nptl/pthreadP.h>
+
+/* Pointer to the fork generation counter in the thread library. */
+extern unsigned long int *__fork_generation_pointer attribute_hidden;
+
+pid_t
+_Fork (void)
+{
+ pid_t pid = arch_fork (&THREAD_SELF->tid);
+ if (pid == 0)
+ {
+ struct pthread *self = THREAD_SELF;
+
+ /* See __pthread_once. */
+ if (__fork_generation_pointer != NULL)
+ *__fork_generation_pointer += __PTHREAD_ONCE_FORK_GEN_INCR;
+
+ /* Initialize the robust mutex list setting in the kernel which has
+ been reset during the fork. We do not check for errors because if
+ it fails here, it must have failed at process startup as well and
+ nobody could have used robust mutexes.
+ Before we do that, we have to clear the list of robust mutexes
+ because we do not inherit ownership of mutexes from the parent.
+ We do not have to set self->robust_head.futex_offset since we do
+ inherit the correct value from the parent. We do not need to clear
+ the pending operation because it must have been zero when fork was
+ called. */
+#if __PTHREAD_MUTEX_HAVE_PREV
+ self->robust_prev = &self->robust_head;
+#endif
+ self->robust_head.list = &self->robust_head;
+#ifdef SHARED
+ if (__builtin_expect (__libc_pthread_functions_init, 0))
+ PTHFCT_CALL (ptr_set_robust, (self));
+#else
+ extern __typeof (__nptl_set_robust) __nptl_set_robust
+ __attribute__((weak));
+ if (__builtin_expect (__nptl_set_robust != NULL, 0))
+ __nptl_set_robust (self);
+#endif
+ }
+ return pid;
+}
+libc_hidden_def (_Fork)
deleted file mode 100644
@@ -1,159 +0,0 @@
-/* Copyright (C) 2002-2021 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
-
- 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 <assert.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <sys/types.h>
-#include <sysdep.h>
-#include <libio/libioP.h>
-#include <tls.h>
-#include <hp-timing.h>
-#include <ldsodefs.h>
-#include <stdio-lock.h>
-#include <atomic.h>
-#include <nptl/pthreadP.h>
-#include <fork.h>
-#include <arch-fork.h>
-#include <futex-internal.h>
-#include <malloc/malloc-internal.h>
-#include <nss/nss_database.h>
-#include <sys/single_threaded.h>
-
-static void
-fresetlockfiles (void)
-{
- _IO_ITER i;
-
- for (i = _IO_iter_begin(); i != _IO_iter_end(); i = _IO_iter_next(i))
- if ((_IO_iter_file (i)->_flags & _IO_USER_LOCK) == 0)
- _IO_lock_init (*((_IO_lock_t *) _IO_iter_file(i)->_lock));
-}
-
-
-pid_t
-__libc_fork (void)
-{
- pid_t pid;
-
- /* Determine if we are running multiple threads. We skip some fork
- handlers in the single-thread case, to make fork safer to use in
- signal handlers. POSIX requires that fork is async-signal-safe,
- but our current fork implementation is not. */
- bool multiple_threads = __libc_single_threaded;
-
- __run_fork_handlers (atfork_run_prepare, multiple_threads);
-
- struct nss_database_data nss_database_data;
-
- /* If we are not running multiple threads, we do not have to
- preserve lock state. If fork runs from a signal handler, only
- async-signal-safe functions can be used in the child. These data
- structures are only used by unsafe functions, so their state does
- not matter if fork was called from a signal handler. */
- if (multiple_threads)
- {
- call_function_static_weak (__nss_database_fork_prepare_parent,
- &nss_database_data);
-
- _IO_list_lock ();
-
- /* Acquire malloc locks. This needs to come last because fork
- handlers may use malloc, and the libio list lock has an
- indirect malloc dependency as well (via the getdelim
- function). */
- call_function_static_weak (__malloc_fork_lock_parent);
- }
-
- pid = arch_fork (&THREAD_SELF->tid);
-
- if (pid == 0)
- {
- struct pthread *self = THREAD_SELF;
-
- /* See __pthread_once. */
- if (__fork_generation_pointer != NULL)
- *__fork_generation_pointer += __PTHREAD_ONCE_FORK_GEN_INCR;
-
- /* Initialize the robust mutex list setting in the kernel which has
- been reset during the fork. We do not check for errors because if
- it fails here, it must have failed at process startup as well and
- nobody could have used robust mutexes.
- Before we do that, we have to clear the list of robust mutexes
- because we do not inherit ownership of mutexes from the parent.
- We do not have to set self->robust_head.futex_offset since we do
- inherit the correct value from the parent. We do not need to clear
- the pending operation because it must have been zero when fork was
- called. */
-#if __PTHREAD_MUTEX_HAVE_PREV
- self->robust_prev = &self->robust_head;
-#endif
- self->robust_head.list = &self->robust_head;
-#ifdef SHARED
- if (__builtin_expect (__libc_pthread_functions_init, 0))
- PTHFCT_CALL (ptr_set_robust, (self));
-#else
- extern __typeof (__nptl_set_robust) __nptl_set_robust
- __attribute__((weak));
- if (__builtin_expect (__nptl_set_robust != NULL, 0))
- __nptl_set_robust (self);
-#endif
-
- /* Reset the lock state in the multi-threaded case. */
- if (multiple_threads)
- {
- /* Release malloc locks. */
- call_function_static_weak (__malloc_fork_unlock_child);
-
- /* Reset the file list. These are recursive mutexes. */
- fresetlockfiles ();
-
- /* Reset locks in the I/O code. */
- _IO_list_resetlock ();
-
- call_function_static_weak (__nss_database_fork_subprocess,
- &nss_database_data);
- }
-
- /* Reset the lock the dynamic loader uses to protect its data. */
- __rtld_lock_initialize (GL(dl_load_lock));
-
- /* Run the handlers registered for the child. */
- __run_fork_handlers (atfork_run_child, multiple_threads);
- }
- else
- {
- /* Release acquired locks in the multi-threaded case. */
- if (multiple_threads)
- {
- /* Release malloc locks, parent process variant. */
- call_function_static_weak (__malloc_fork_unlock_parent);
-
- /* We execute this even if the 'fork' call failed. */
- _IO_list_unlock ();
- }
-
- /* Run the handlers registered for the parent. */
- __run_fork_handlers (atfork_run_parent, multiple_threads);
- }
-
- return pid;
-}
-weak_alias (__libc_fork, __fork)
-libc_hidden_def (__fork)
-weak_alias (__libc_fork, fork)
@@ -19,6 +19,9 @@
#ifndef __ARCH_FORK_H
#define __ARCH_FORK_H
+#include <sysdep.h>
+#include <sched.h>
+#include <signal.h>
#include <unistd.h>
/* Call the clone syscall with fork semantic. The CTID address is used