Hurd: make sigstates hold a reference on thread ports

This change is required in order to correctly release per-thread
resources. Directly reusing the threading library reference isn't
possible since the sigstate is also used early in the main thread,
before threading is initialized.

* hurd/hurd/signal.h (_hurd_self_sigstate): Drop thread reference after
calling _hurd_thread_sigstate.
(_hurd_critical_section_lock): Likewise.
* hurd/hurdsig.c (_hurd_thread_sigstate): Add a reference on the thread.
(_hurd_sigstate_delete): Drop thread reference.

4 files changed, 42 insertions, 77 deletions

diff --git a/.topdeps b/.topdeps
index 54fb4356a1..e83f8a04b8 100644
--- a/.topdeps
+++ b/.topdeps
@@ -1 +1 @@
-t/hurdsig-fixes
+t/hurdsig-global-dispositions
diff --git a/.topmsg b/.topmsg
index 3353306c4d..4036303c98 100644
--- a/.topmsg
+++ b/.topmsg
@@ -1,70 +1,13 @@
-Subject: [PATCH] Global signal dispositions.
-
-Although they should not change the
-default behaviors of signals for cthread programs, these patches add
-new functions which can be used by libpthread to enable
-POSIX-conforming behavior of signals on a per-thread basis.
-
-YYYY-MM-DD  Jeremie Koenig  <jk@jk.fr.eu.org>
-
-    e407ae3 Hurd signals: implement global signal dispositions
-    38eb4b3 Hurd signals: provide a sigstate destructor
-    344dfd6 Hurd signals: fix sigwait() for global signals
-    fb055f2 Hurd signals: fix global untraced signals.
-
-YYYY-MM-DD  Thomas Schwinge  <thomas@codesourcery.com>
-
-	* sysdeps/mach/hurd/fork.c (__fork): In the child, reinitialize
-	the global sigstate's lock.
-
-This is work in progress.
-
-This cures an issue that would very rarely cause a deadlock in the child
-in fork, tries to unlock ss' critical section lock at the end of fork.
-This will typically (always?) be observed in /bin/sh, which is not
-surprising as that is the foremost caller of fork.
-
-To reproduce an intermediate state, add an endless loop if
-_hurd_global_sigstate is locked after __proc_dostop (cast through
-volatile); that is, while still being in the fork's parent process.
-
-When that triggers (use the libtool testsuite), the signal thread has
-already locked ss (which is _hurd_global_sigstate), and is stuck at
-hurdsig.c:685 in post_signal, trying to lock _hurd_siglock (which the
-main thread already has locked and keeps locked until after
-__task_create).  This is the case that ss->thread == MACH_PORT_NULL, that
-is, a global signal.  In the main thread, between __proc_dostop and
-__task_create is the __thread_abort call on the signal thread which would
-abort any current kernel operation (but leave ss locked).  Later in fork,
-in the parent, when _hurd_siglock is unlocked in fork, the parent's
-signal thread can proceed and will unlock eventually the global sigstate.
-In the client, _hurd_siglock will likewise be unlocked, but the global
-sigstate never will be, as the client's signal thread has been configured
-to restart execution from _hurd_msgport_receive.  Thus, when the child
-tries to unlock ss' critical section lock at the end of fork, it will
-first lock the global sigstate, will spin trying to lock it, which can
-never be successful, and we get our deadlock.
-
-Options seem to be:
-
-  * Move the locking of _hurd_siglock earlier in post_signal -- but that
-    may generally impact performance, if this locking isn't generally
-    needed anyway?
-
-    On the other hand, would it actually make sense to wait here until we
-    are not any longer in a critical section (which is meant to disable
-    signal delivery anway (but not for preempted signals?))?
-
-  * Clear the global sigstate in the fork's child with the rationale that
-    we're anyway restarting the signal thread from a clean state.  This
-    has now been implemented.
-
-Why has this problem not been observed before Jérémie's patches?  (Or has
-it?  Perhaps even more rarely?)  In _S_msg_sig_post, the signal is now
-posted to a *global receiver thread*, whereas previously it was posted to
-the *designated signal-receiving thread*.  The latter one was in a
-critical section in fork, so didn't try to handle the signal until after
-leaving the critical section?  (Not completely analyzed and verified.)
-
-Another question is what the signal is that is being received
-during/around the time __proc_dostop executes.
+From: Richard Braun <rbraun@sceen.net>
+Subject: [PATCH] Hurd: make sigstates hold a reference on thread ports
+
+This change is required in order to correctly release per-thread
+resources. Directly reusing the threading library reference isn't
+possible since the sigstate is also used early in the main thread,
+before threading is initialized.
+
+* hurd/hurd/signal.h (_hurd_self_sigstate): Drop thread reference after
+calling _hurd_thread_sigstate.
+(_hurd_critical_section_lock): Likewise.
+* hurd/hurdsig.c (_hurd_thread_sigstate): Add a reference on the thread.
+(_hurd_sigstate_delete): Drop thread reference.
diff --git a/hurd/hurd/signal.h b/hurd/hurd/signal.h
index db60e6f0a1..1d1218a88a 100644
--- a/hurd/hurd/signal.h
+++ b/hurd/hurd/signal.h
@@ -64,7 +64,9 @@ struct hurd_sigstate
 
     spin_lock_t lock;		/* Locks most of the rest of the structure.  */
 
+    /* The signal state holds a reference on the thread port.  */
     thread_t thread;
+
     struct hurd_sigstate *next; /* Linked-list of thread sigstates.  */
 
     sigset_t blocked;		/* What signals are blocked.  */
@@ -118,7 +120,9 @@ extern struct hurd_sigstate *_hurd_sigstates;
 
 extern struct mutex _hurd_siglock; /* Locks _hurd_sigstates.  */
 
-/* Get the sigstate of a given thread, taking its lock.  */
+/* Get the sigstate of a given thread.  If there was no sigstate for
+   the thread, one is created, and the thread gains a reference.  If
+   the given thread is MACH_PORT_NULL, return the global sigstate.  */
 
 extern struct hurd_sigstate *_hurd_thread_sigstate (thread_t);
 
@@ -161,7 +165,11 @@ _hurd_self_sigstate (void)
   struct hurd_sigstate **location =
     (void *) __hurd_threadvar_location (_HURD_THREADVAR_SIGSTATE);
   if (*location == NULL)
-    *location = _hurd_thread_sigstate (__mach_thread_self ());
+    {
+      thread_t self = __mach_thread_self ();
+      *location = _hurd_thread_sigstate (self);
+      __mach_port_deallocate (__mach_task_self (), self);
+    }
   return *location;
 }
 
@@ -192,11 +200,14 @@ _hurd_critical_section_lock (void)
   struct hurd_sigstate *ss = *location;
   if (ss == NULL)
     {
+      thread_t self = __mach_thread_self ();
+
       /* The thread variable is unset; this must be the first time we've
 	 asked for it.  In this case, the critical section flag cannot
 	 possible already be set.  Look up our sigstate structure the slow
 	 way; this locks the sigstate lock.  */
-      ss = *location = _hurd_thread_sigstate (__mach_thread_self ());
+      ss = *location = _hurd_thread_sigstate (self);
+      __mach_port_deallocate (__mach_task_self (), self);
       __spin_unlock (&ss->lock);
     }
 
diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
index 72c582f00b..486be7ca9d 100644
--- a/hurd/hurdsig.c
+++ b/hurd/hurdsig.c
@@ -105,6 +105,8 @@ _hurd_thread_sigstate (thread_t thread)
 	}
       else
 	{
+	  error_t err;
+
 	  /* Use the global actions as a default for new threads.  */
 	  struct hurd_sigstate *s = _hurd_global_sigstate;
 	  if (s)
@@ -118,6 +120,11 @@ _hurd_thread_sigstate (thread_t thread)
 
 	  ss->next = _hurd_sigstates;
 	  _hurd_sigstates = ss;
+
+	  err = __mach_port_mod_refs (__mach_task_self (), thread,
+				      MACH_PORT_RIGHT_SEND, 1);
+	  if (err)
+	    __libc_fatal ("hurd: Can't add reference on Mach thread\n");
 	}
     }
   __mutex_unlock (&_hurd_siglock);
@@ -125,8 +132,7 @@ _hurd_thread_sigstate (thread_t thread)
 }
 
 /* Destroy a sigstate structure.  Called by libpthread just before the
- * corresponding thread is terminated (the kernel thread port must remain valid
- * until this function is called.) */
+ * corresponding thread is terminated.  */
 void
 _hurd_sigstate_delete (thread_t thread)
 {
@@ -143,7 +149,12 @@ _hurd_sigstate_delete (thread_t thread)
 
   __mutex_unlock (&_hurd_siglock);
   if (ss)
-    free (ss);
+    {
+      if (ss->thread != MACH_PORT_NULL)
+	__mach_port_deallocate (__mach_task_self (), ss->thread);
+
+      free (ss);
+    }
 }
 
 /* Make SS a global receiver, with pthread signal semantics.  */