NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: lib/59125: pthread_once(3) races with concurrent fork
The following reply was made to PR lib/59125; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
Cc:
Subject: Re: lib/59125: pthread_once(3) races with concurrent fork
Date: Wed, 5 Mar 2025 22:56:40 +0000
This is a multi-part message in MIME format.
--=_QSVhpQW7wL2t2ZyM0lNb6kI+m4HN6cIl
My second attempt is to serialize pthread_once and fork. I think this
is unavoidable to guarantee that the parent and child both observe a
single call to the init routine; any fork concurrent with a call to
the init routine in the parent will necessarily break this contract,
unless we decide to prohibit pthread_once or make it return failure in
the child -- which makes it not particularly useful.
Serializing pthread_once and fork doesn't require serializing all init
routines, however: instead, if there's any pthread_once call in
progress, we hold up fork, but the init routines for different
pthread_once_t objects can run in parallel. And if you try to call
fork in a pthread_once init routine, it fails with EDEADLK. (I picked
EDEADLK mainly because without logic to detect this scenario, it would
deadlock. Maybe EBUSY or something would be better, no strong feeling
here.)
If we want, we could also implement `writer starvation' avoidance by
having fork (the `writer') block new pthread_once calls (the
`readers') until all the top-level pending calls have completed.
--=_QSVhpQW7wL2t2ZyM0lNb6kI+m4HN6cIl
Content-Type: text/plain; charset="ISO-8859-1"; name="pr59125-pthreadonceforklock"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr59125-pthreadonceforklock.patch"
# HG changeset patch
# User Taylor R Campbell <riastradh%NetBSD.org@localhost>
# Date 1741201110 0
# Wed Mar 05 18:58:30 2025 +0000
# Branch trunk
# Node ID 652c5f82aa0cacebc9d86ccd9618950fe579752a
# Parent cd6b61c1b621c4bb80fbe22e714ba188467be421
# EXP-Topic riastradh-pr59125-oncefork
pthread_once(3): Fix races with concurrent fork by blocking it.
If fork can copy any states that the pthread_once initialization
routine transitions through, then it is not possible to satisfy the
contract of pthread_once(3), which is that the initialization routine
has been called exactly once.
To avoid this, we hold up fork until any concurrent pthread_once(3)
calls that may do any initialization have completed.
It is also not possible for pthread_once(3) to satisfy its contract
if the initalization routine itself forks. To prevent this, we make
fork(3) fail with EDEADLK if you try to call it during that time.
Nested calls to pthread_once(3) count the call depth. (Without this
explicit detection, fork(3) would simply deadlock instead.)
The runtime cost to fork(3) is two predicted-not-taken branches, one
via indirect function call -- and in the unlikely event that there
are any concurrent pthread_once(3) calls, it waits for them to
complete.
The runtime cost to the first call to pthread_once(3) is potential
contention over the atfork_lock, which serializes fork(3) and
pthread_atfork(3). However, the initialization routines themselves
in concurrent calls to pthread_once(3) are not serialized; they can
run in parallel.
While here, protect pthread_once(3) from PTHREAD_CANCEL_ASYNCHRONOUS.
PR lib/59125: pthread_once(3) races with concurrent fork
diff -r cd6b61c1b621 -r 652c5f82aa0c lib/libc/gen/pthread_atfork.c
--- a/lib/libc/gen/pthread_atfork.c Wed Mar 05 15:29:16 2025 +0000
+++ b/lib/libc/gen/pthread_atfork.c Wed Mar 05 18:58:30 2025 +0000
@@ -41,6 +41,7 @@
#include <unistd.h>
#include <sys/queue.h>
#include "extern.h"
+#include "fork.h"
#include "reentrant.h"
=20
#ifdef __weak_alias
@@ -175,6 +176,28 @@ out: mutex_unlock(&atfork_lock);
return error;
}
=20
+static uint64_t block_fork_count;
+static cond_t block_fork_cv =3D COND_INITIALIZER;
+
+void
+__libc_block_fork(void)
+{
+
+ mutex_lock(&atfork_lock);
+ block_fork_count++; /* 64-bit, can't overflow in 500 years */
+ mutex_unlock(&atfork_lock);
+}
+
+void
+__libc_unblock_fork(void)
+{
+
+ mutex_lock(&atfork_lock);
+ if (--block_fork_count =3D=3D 0)
+ cond_broadcast(&block_fork_cv);
+ mutex_unlock(&atfork_lock);
+}
+
pid_t
fork(void)
{
@@ -182,8 +205,16 @@ fork(void)
pid_t ret;
=20
mutex_lock(&atfork_lock);
+ if (__predict_false(__libc_thr_in_once())) {
+ mutex_unlock(&atfork_lock);
+ errno =3D EDEADLK;
+ return -1;
+ }
+ while (__predict_false(block_fork_count))
+ cond_wait(&block_fork_cv, &atfork_lock);
+
SIMPLEQ_FOREACH(iter, &prepareq, next)
- (*iter->fn)();
+ (*iter->fn)();
_malloc_prefork();
=20
ret =3D __locked_fork(&errno);
diff -r cd6b61c1b621 -r 652c5f82aa0c lib/libc/include/fork.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/lib/libc/include/fork.h Wed Mar 05 18:58:30 2025 +0000
@@ -0,0 +1,36 @@
+/* $NetBSD$ */
+
+/*-
+ * Copyright (c) 2025 The NetBSD Foundation, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTO=
RS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIM=
ITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICU=
LAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTO=
RS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF =
THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _LIB_LIBC_FORK_H_
+#define _LIB_LIBC_FORK_H_
+
+void __libc_block_fork(void);
+void __libc_unblock_fork(void);
+int __libc_thr_in_once(void);
+
+#endif /* _LIB_LIBC_FORK_H_ */
diff -r cd6b61c1b621 -r 652c5f82aa0c lib/libc/include/reentrant.h
--- a/lib/libc/include/reentrant.h Wed Mar 05 15:29:16 2025 +0000
+++ b/lib/libc/include/reentrant.h Wed Mar 05 18:58:30 2025 +0000
@@ -262,6 +262,7 @@ void *__libc_thr_getspecific_stub(thread
int __libc_thr_keydelete_stub(thread_key_t);
=20
int __libc_thr_once_stub(once_t *, void (*)(void));
+int __libc_thr_in_once_stub(void);
int __libc_thr_sigsetmask_stub(int, const sigset_t *, sigset_t *);
thr_t __libc_thr_self_stub(void);
int __libc_thr_yield_stub(void);
diff -r cd6b61c1b621 -r 652c5f82aa0c lib/libc/thread-stub/thread-stub.c
--- a/lib/libc/thread-stub/thread-stub.c Wed Mar 05 15:29:16 2025 +0000
+++ b/lib/libc/thread-stub/thread-stub.c Wed Mar 05 18:58:30 2025 +0000
@@ -355,6 +355,7 @@ int
/* misc. */
=20
__weak_alias(__libc_thr_once,__libc_thr_once_stub)
+__weak_alias(__libc_thr_in_once,__libc_thr_in_once_stub)
__weak_alias(__libc_thr_sigsetmask,__libc_thr_sigsetmask_stub)
__weak_alias(__libc_thr_self,__libc_thr_self_stub)
__weak_alias(__libc_thr_yield,__libc_thr_yield_stub)
@@ -365,6 +366,8 @@ int
__weak_alias(__libc_thr_curcpu,__libc_thr_curcpu_stub)
=20
=20
+static size_t __libc_thr_once_depth;
+
int
__libc_thr_once_stub(once_t *o, void (*r)(void))
{
@@ -372,7 +375,9 @@ int
/* XXX Knowledge of libpthread types. */
=20
if (o->pto_done =3D=3D 0) {
+ __libc_thr_once_depth++;
(*r)();
+ __libc_thr_once_depth--;
o->pto_done =3D 1;
}
=20
@@ -380,6 +385,13 @@ int
}
=20
int
+__libc_thr_in_once_stub(void)
+{
+
+ return __libc_thr_once_depth !=3D 0;
+}
+
+int
__libc_thr_sigsetmask_stub(int h, const sigset_t *s, sigset_t *o)
{
=20
diff -r cd6b61c1b621 -r 652c5f82aa0c lib/libpthread/pthread_int.h
--- a/lib/libpthread/pthread_int.h Wed Mar 05 15:29:16 2025 +0000
+++ b/lib/libpthread/pthread_int.h Wed Mar 05 18:58:30 2025 +0000
@@ -106,6 +106,7 @@ struct __pthread_st {
struct pthread_lock_ops pt_lockops;/* Cached to avoid PIC overhead */
void *(*pt_func)(void *);/* Function to call at start. */
void *pt_arg; /* Argument to pass at start. */
+ size_t pt_oncedepth; /* pthread_once call depth */
=20
/* Stack of cancellation cleanup handlers and their arguments */
PTQ_HEAD(, pt_clean_t) pt_cleanup_stack;
diff -r cd6b61c1b621 -r 652c5f82aa0c lib/libpthread/pthread_mi.expsym
--- a/lib/libpthread/pthread_mi.expsym Wed Mar 05 15:29:16 2025 +0000
+++ b/lib/libpthread/pthread_mi.expsym Wed Mar 05 18:58:30 2025 +0000
@@ -29,6 +29,7 @@
__libc_thr_errno
__libc_thr_exit
__libc_thr_getspecific
+__libc_thr_in_once
__libc_thr_init
__libc_thr_keycreate
__libc_thr_keydelete
diff -r cd6b61c1b621 -r 652c5f82aa0c lib/libpthread/pthread_once.c
--- a/lib/libpthread/pthread_once.c Wed Mar 05 15:29:16 2025 +0000
+++ b/lib/libpthread/pthread_once.c Wed Mar 05 18:58:30 2025 +0000
@@ -1,12 +1,9 @@
-/* $NetBSD: pthread_once.c,v 1.5 2025/03/04 00:41:00 riastradh Exp $ */
+/* $NetBSD$ */
=20
/*-
- * Copyright (c) 2001, 2003 The NetBSD Foundation, Inc.
+ * Copyright (c) 2025 The NetBSD Foundation, Inc.
* All rights reserved.
*
- * This code is derived from software contributed to The NetBSD Foundation
- * by Nathan J. Williams, and by Jason R. Thorpe.
- *
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
@@ -15,13 +12,6 @@
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
- * 3. All advertising materials mentioning features or use of this software
- * must display the following acknowledgement:
- * This product includes software developed by the NetBSD
- * Foundation, Inc. and its contributors.
- * 4. Neither the name of The NetBSD Foundation nor the names of its
- * contributors may be used to endorse or promote products derived
- * from this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTO=
RS
* ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIM=
ITED
@@ -37,42 +27,123 @@
*/
=20
#include <sys/cdefs.h>
-__RCSID("$NetBSD: pthread_once.c,v 1.5 2025/03/04 00:41:00 riastradh Exp $=
");
+__RCSID("$NetBSD$");
=20
/* Need to use libc-private names for atomic operations. */
#include "../../common/lib/libc/atomic/atomic_op_namespace.h"
=20
+#include "../../lib/libc/include/fork.h"
+
+#include <stdatomic.h>
+
#include "pthread.h"
#include "pthread_int.h"
#include "reentrant.h"
=20
+#define atomic_load_acquire(p) \
+ atomic_load_explicit(p, memory_order_acquire)
+
+#define atomic_store_release(p, v) \
+ atomic_store_explicit(p, v, memory_order_release)
+
static void
-once_cleanup(void *closure)
+once_cleanup(void *cookie)
{
+ pthread_once_t *once_control =3D cookie;
=20
- pthread_mutex_unlock((pthread_mutex_t *)closure);
+ pthread_mutex_unlock(&once_control->pto_mutex);
+ __libc_unblock_fork();
}
=20
int
-pthread_once(pthread_once_t *once_control, void (*routine)(void))
+pthread_once(pthread_once_t *once_control, void (*init_routine)(void))
{
+ int cancelstate;
+
if (__predict_false(__uselibcstub))
- return __libc_thr_once_stub(once_control, routine);
+ return __libc_thr_once_stub(once_control, init_routine);
+
+ /*
+ * Optimistically check whether it's already done, with a
+ * load-acquire to synchronize with the store-release in
+ * whatever thread did the initialization.
+ */
+ if (__predict_true(atomic_load_acquire(&once_control->pto_done) =3D=3D 1))
+ return 0;
+
+ /*
+ * Carefully prepare to check and run the routine:
+ *
+ * 1. Defer cancellation while we synchronize in case the
+ * caller is configured as PTHREAD_CANCEL_ASYNCHRONOUS, so
+ * we are guaranteed to have a chance to clean up after
+ * ourselves if cancelled.
+ *
+ * 2. Block concurrent fork while we work so that children will
+ * never observe either the once_control state or the state
+ * that the user's init_routine runs partially initialized.
+ *
+ * 3. Serialize access to this once_control in particular by
+ * taking the mutex.
+ *
+ * 4. Push a cleanup action to unwind this preparation when
+ * we're done, or if we're cancelled during init_routine.
+ */
+ pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancelstate);
+ __libc_block_fork();
+ pthread_mutex_lock(&once_control->pto_mutex);
+ pthread_cleanup_push(&once_cleanup, &once_control->pto_mutex);
=20
- if (once_control->pto_done =3D=3D 0) {
- pthread_mutex_lock(&once_control->pto_mutex);
- pthread_cleanup_push(&once_cleanup, &once_control->pto_mutex);
- if (once_control->pto_done =3D=3D 0) {
- routine();
- membar_release();
- once_control->pto_done =3D 1;
- }
- pthread_cleanup_pop(1);
- } else {
- membar_acquire();
- }
+ /*
+ * It's possible that we raced with another caller to
+ * pthread_once with the same state. Don't run the routine
+ * again if so.
+ */
+ if (__predict_false(once_control->pto_done))
+ goto out;
+
+ /*
+ * Allow cancellation, if the caller allowed it, while we run
+ * the init routine. If cancelled during the initialization,
+ * we don't mark it done:
+ *
+ * The pthread_once() function is not a cancellation
+ * point. However, if init_routine is a cancellation
+ * point and is canceled, the effect on once_control shall
+ * be as if pthread_once() was never called.
+ *
+ * And mark ourselves in the middle of pthread_once so that if
+ * init_routine tries to use fork(), it will fail -- it is
+ * impossible to satisfy the requirements of pthread_once and
+ * fork.
+ */
+ pthread_setcancelstate(cancelstate, NULL);
+ pthread_self()->pt_oncedepth++; /* stack overflows before this can */
+ (*init_routine)();
+ pthread_self()->pt_oncedepth--;
+ pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL);
+
+ /*
+ * Done. Use a store-release to synchronize with the
+ * load-acquire in unlocked users of pthread_once.
+ */
+ atomic_store_release(&once_control->pto_done, 1);
+out:
+ /*
+ * Release the lock, allow fork again, and restore the caller's
+ * cancellation state.
+ */
+ pthread_cleanup_pop(/*execute*/1);
+ pthread_setcancelstate(cancelstate, NULL);
=20
return 0;
}
=20
+int
+__libc_thr_in_once(void)
+{
+
+ return __predict_false(pthread_self()->pt_oncedepth !=3D 0);
+}
+
__strong_alias(__libc_thr_once,pthread_once)
--=_QSVhpQW7wL2t2ZyM0lNb6kI+m4HN6cIl--
Home |
Main Index |
Thread Index |
Old Index