Don't lock kq mutexes on fork

Author: arr2036

ChangeLog

@@ -1,41 +1,46 @@
 Unreleased
 ------------------------------------------------------------------------
- 
-2022-04-27  v2.6.2
+
+2022-11-02  v2.6.2
 ------------------------------------------------------------------------
 
    * Fix: Various build system and include fixes to allow libkqueue
      to build correctly with musl libc.
-     
+
    * Fix: Various macro and include fixes for Windows/MinGW builds.
      (Fixed by: neheb)
-     
-   * Always build both a static and a shared library. The STATIC_KQUEUE 
+
+   * Always build both a static and a shared library. The STATIC_KQUEUE
      build variable has been removed.
- 
+
+   * Fix: Don't attempt to lock kqueues on fork.  It's no longer needed
+     as we're not attempting to free memory in the kqueues, only close
+     file descriptors.  This may resolve stalling fork() calls where
+     kqueues were in a waiting state.
+
 2022-04-27  v2.6.1
 ------------------------------------------------------------------------
 
-  * Fix: Set the core dumped flag correctly in linux/proc on 
+  * Fix: Set the core dumped flag correctly in linux/proc on
     CLD_DUMPED.
 
   * Fix: Don't attempt to cleanup memory on fork as free() is not
     fork safe.  We still close open kqueue FDs on fork, and liberal
     use of CLOEXEC should clean up any lingering filter FDs though
     we may want to close these explicitly in future too.
- 
+
   * Fix: Double unlock in posix/proc.c
- 
+
   * Fix: Potential deadlock on kqueue initialisation failure.
 
   * Fix: Building for MSVC (Patch by Jan200101)
 
   * Fix: Sprinkle CLOEXEC around various places in the Linux and
     POSIX platforms to stop us leaking file descriptors on exec.
-    
-  * Add EVFILT_LIBKQUEUE/NOTE_THREAD_SAFE.  When set to 0, the 
-    global mutex is not locked on kqueue lookup.  The application 
-    must guarantee that kqueues must not be destroyed by a different 
+
+  * Add EVFILT_LIBKQUEUE/NOTE_THREAD_SAFE.  When set to 0, the
+    global mutex is not locked on kqueue lookup.  The application
+    must guarantee that kqueues must not be destroyed by a different
     thread to the one that created them.
 
 2022-03-21  v2.6.0

src/common/kqueue.c

@@ -141,18 +141,6 @@ libkqueue_pre_fork(void)
 {
     struct kqueue *kq, *kq_tmp;
 
-    /*
-     * Ensure that all global structures are in a
-     * consistent state before attempting the
-     * fork.
-     *
-     * If we don't do this then kq_mtx state will
-     * be undefined when the child starts cleaning
-     * up resources, and we could get deadlocks, nasty
-     * memory corruption and or crashes in the child.
-     */
-    tracing_mutex_lock(&kq_mtx);
-
     /*
      * Unfortunately there's no way to remove the atfork
      * handlers, so all we can do if cleanup is
@@ -166,35 +154,44 @@ libkqueue_pre_fork(void)
         return;
 
     /*
-     * Acquire locks for all the active kqueues,
-     * this ensures in the child all kqueues are in
-     * a consistent state, ready to be freed.
+     * Ensure that all global structures are in a consistent
+     * state before attempting the fork.
      *
-     * This has the potential to stall out the process
-     * whilst we attempt to acquire all the locks,
-     * so it might be a good idea to make this
-     * configurable in future.
+     * If we don't do this then kq_mtx state will
+     * be undefined when the child starts cleaning
+     * up resources, and we could get deadlocks, nasty
+     * memory corruption and/or crashes in the child.
+     *
+     * We always lock the kq_mtx even when
+     * !libkqueue_thread_safe because it's still locked when
+     * kqueues are being freed, and we don't want to access
+     * freed memory when attempting to perform cleanup.
+     */
+    tracing_mutex_lock(&kq_mtx);
+
+    /*
+     * We previously attempted to lock all the child
+     * kqueues, but when they were waiting in kevent_wait
+     * this caused fork to stall.
+     *
+     * The main reason for locking the child kqueues was
+     * to ensure filters and kqueues were in a consistent
+     * state at the time of the fork, to ensure we could
+     * free resources correctly... but as it was later
+     * discovered many systems really don't like free()
+     * being called in the forked child, so we were limited
+     * to async-safe calls like close... These should be
+     * safe no matter what the state of the kqueues so
+     * the locking was removed.
      */
-    dbg_puts("gathering kqueue locks on fork");
-    LIST_FOREACH_SAFE(kq, &kq_list, kq_entry, kq_tmp) {
-        kqueue_lock(kq);
-    }
 }
 
 void
 libkqueue_parent_fork(void)
 {
-    struct kqueue *kq, *kq_tmp;
-
-    if (!libkqueue_fork_cleanup_active) {
-        tracing_mutex_unlock(&kq_mtx);
+    if (!libkqueue_fork_cleanup_active)
         return;
-    }
 
-    dbg_puts("releasing kqueue locks in parent");
-    LIST_FOREACH_SAFE(kq, &kq_list, kq_entry, kq_tmp) {
-        kqueue_unlock(kq);
-    }
     tracing_mutex_unlock(&kq_mtx);
 }
 
@@ -205,15 +202,8 @@ libkqueue_child_fork(void)
 
     libkqueue_in_child = true;
 
-    if (!libkqueue_fork_cleanup_active) {
-        tracing_mutex_unlock(&kq_mtx);
+    if (!libkqueue_fork_cleanup_active)
         return;
-    }
-
-    dbg_puts("releasing kqueue locks in child");
-    LIST_FOREACH_SAFE(kq, &kq_list, kq_entry, kq_tmp) {
-        kqueue_unlock(kq);
-    }
 
     dbg_puts("cleaning up forked resources");
     filter_fork_all();