On 07/12/22(Wed) 22:17, Joel Knight wrote: > Hi. As first mentioned on misc[1], I've identified a deadlock in libc > when a process forks, the children are multi-threaded, and they set > one or more atfork callbacks. The diff below causes ATFORK_UNLOCK() to > release the lock even when the process isn't multi-threaded. This > avoids the deadlock. With this patch applied, the test case I have for > this issue succeeds and there are no new failures during a full 'make > regress'. > > Threading is outside my area of expertise so I've no idea if the fix > proposed here is appropriate. I'm happy to take or test feedback. > > The diff is below and a clean copy is here: > https://www.packetmischief.ca/files/patches/atfork_on_fork.diff.
This sounds legit to me. Anyone some time wants to take a look? > .joel > > > [1] https://marc.info/?l=openbsd-misc&m=166926508819790&w=2 > > > > Index: lib/libc/include/thread_private.h > =================================================================== > RCS file: /data/cvs-mirror/OpenBSD/src/lib/libc/include/thread_private.h,v > retrieving revision 1.36 > diff -p -u -r1.36 thread_private.h > --- lib/libc/include/thread_private.h 6 Jan 2021 19:54:17 -0000 1.36 > +++ lib/libc/include/thread_private.h 8 Dec 2022 04:28:45 -0000 > @@ -228,7 +228,7 @@ __END_HIDDEN_DECLS > } while (0) > #define _ATFORK_UNLOCK() \ > do { \ > - if (__isthreaded) \ > + if (_thread_cb.tc_atfork_unlock != NULL) \ > _thread_cb.tc_atfork_unlock(); \ > } while (0) > > Index: regress/lib/libpthread/pthread_atfork_on_fork/Makefile > =================================================================== > RCS file: regress/lib/libpthread/pthread_atfork_on_fork/Makefile > diff -N regress/lib/libpthread/pthread_atfork_on_fork/Makefile > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ regress/lib/libpthread/pthread_atfork_on_fork/Makefile 7 Dec 2022 > 04:38:39 -0000 > @@ -0,0 +1,9 @@ > +# $OpenBSD$ > + > +PROG= pthread_atfork_on_fork > + > +REGRESS_TARGETS= timeout > +timeout: > + timeout 10s ./${PROG} > + > +.include <bsd.regress.mk> > Index: regress/lib/libpthread/pthread_atfork_on_fork/pthread_atfork_on_fork.c > =================================================================== > RCS file: > regress/lib/libpthread/pthread_atfork_on_fork/pthread_atfork_on_fork.c > diff -N regress/lib/libpthread/pthread_atfork_on_fork/pthread_atfork_on_fork.c > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ regress/lib/libpthread/pthread_atfork_on_fork/pthread_atfork_on_fork.c > 7 Dec 2022 04:59:10 -0000 > @@ -0,0 +1,94 @@ > +/* $OpenBSD$ */ > + > +/* > + * Copyright (c) 2022 Joel Knight <j...@openbsd.org> > + * > + * Permission to use, copy, modify, and distribute this software for any > + * purpose with or without fee is hereby granted, provided that the above > + * copyright notice and this permission notice appear in all copies. > + * > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ > + > +/* > + * This test exercises atfork lock/unlock through multiple generations of > + * forked child processes where each child also becomes multi-threaded. > + */ > + > +#include <pthread.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <unistd.h> > + > +#include <sys/wait.h> > + > +#define NUMBER_OF_GENERATIONS 4 > +#define NUMBER_OF_TEST_THREADS 2 > + > +#define SAY(...) do { \ > + fprintf(stderr, "pid %5i ", getpid()); \ > + fprintf(stderr, __VA_ARGS__); \ > +} while (0) > + > +void > +prepare(void) > +{ > + /* Do nothing */ > +} > + > +void * > +thread(void *arg) > +{ > + return (NULL); > +} > + > +int > +test(int fork_level) > +{ > + pid_t proc_pid; > + size_t thread_index; > + pthread_t threads[NUMBER_OF_TEST_THREADS]; > + > + proc_pid = fork(); > + fork_level = fork_level - 1; > + > + if (proc_pid == 0) { > + SAY("generation %i\n", fork_level); > + pthread_atfork(prepare, NULL, NULL); > + > + for (thread_index = 0; thread_index < NUMBER_OF_TEST_THREADS; > thread_index++) { > + pthread_create(&threads[thread_index], NULL, thread, NULL); > + } > + > + for (thread_index = 0; thread_index < NUMBER_OF_TEST_THREADS; > thread_index++) { > + pthread_join(threads[thread_index], NULL); > + } > + > + if (fork_level > 0) { > + test(fork_level); > + } > + > + SAY("exiting\n"); > + exit(0); > + } > + else { > + SAY("parent waiting on child %i\n", proc_pid); > + waitpid(proc_pid, 0, 0); > + } > + > + return (0); > +} > + > +int > +main(int argc, char *argv[]) > +{ > + test(NUMBER_OF_GENERATIONS); > + > + return (0); > +} >
