[hurd, commited] hurd: calling alarm() whilst handling SIGALRM can deadlock.

From: Mike Kelly <[email protected]>

alarm() and restart_itimer() can attempt to acquire _hurd_siglock and
_hurd_itimer_lock in opposite sequence resulting in occasional
deadlock. Rearranged to always acquire the locks in the same sequence
with a new pre-condition that setitimer_locked() must be called with
both locks already acquired.
Message-ID: <[email protected]>

Reviewed-by: Samuel Thibault <[email protected]>
---
 sysdeps/mach/hurd/setitimer.c | 42 +++++++++++++++++------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/sysdeps/mach/hurd/setitimer.c b/sysdeps/mach/hurd/setitimer.c
index 5a57280e2c..265576e00e 100644
--- a/sysdeps/mach/hurd/setitimer.c
+++ b/sysdeps/mach/hurd/setitimer.c
@@ -131,8 +131,7 @@ timer_thread (void)
 
 /* Forward declaration.  */
 static int setitimer_locked (const struct itimerval *new,
-			     struct itimerval *old, void *crit,
-			     int hurd_siglocked);
+			     struct itimerval *old);
 
 static sighandler_t
 restart_itimer (struct hurd_signal_preemptor *preemptor,
@@ -144,21 +143,25 @@ restart_itimer (struct hurd_signal_preemptor *preemptor,
   struct itimerval it;
 
   /* Either reload or disable the itimer.  */
+  /* _hurd_siglock is already locked by the caller. */
   __spin_lock (&_hurd_itimer_lock);
   it.it_value = it.it_interval = _hurd_itimerval.it_interval;
-  setitimer_locked (&it, NULL, NULL, 1);
+  setitimer_locked (&it, NULL);
+  __spin_unlock (&_hurd_itimer_lock);
 
   /* Continue with normal delivery (or hold, etc.) of SIGALRM.  */
   return SIG_ERR;
 }
 
 
-/* Called before any normal SIGALRM signal is delivered.
-   Reload the itimer, or disable the itimer.  */
+/* Called before any normal SIGALRM signal is delivered.  Reload the
+   itimer, or disable the itimer. _hurd_siglock and _hurd_itimer_lock
+   must be locked before entry noting that it is important to acquire
+   the _hurd_siglock before the _hurd_itimer_lock. Both of these
+   remain locked on exit. */
 
 static int
-setitimer_locked (const struct itimerval *new, struct itimerval *old,
-		  void *crit, int hurd_siglocked)
+setitimer_locked (const struct itimerval *new, struct itimerval *old)
 {
   struct itimerval newval;
   struct timeval now, remaining, elapsed;
@@ -180,8 +183,6 @@ setitimer_locked (const struct itimerval *new, struct itimerval *old,
 	 This is what BSD does, even though it's not documented. */
       if (old)
 	*old = _hurd_itimerval;
-      spin_unlock (&_hurd_itimer_lock);
-      _hurd_critical_section_unlock (crit);
       return 0;
     }
 
@@ -199,16 +200,12 @@ setitimer_locked (const struct itimerval *new, struct itimerval *old,
 	  __sigmask (SIGALRM), SI_TIMER, SI_TIMER,
 	  &restart_itimer,
 	};
-      if (!hurd_siglocked)
-	__mutex_lock (&_hurd_siglock);
       if (! preemptor.next && _hurdsig_preemptors != &preemptor)
 	{
 	  preemptor.next = _hurdsig_preemptors;
 	  _hurdsig_preemptors = &preemptor;
 	  _hurdsig_preempted_set |= preemptor.signals;
 	}
-      if (!hurd_siglocked)
-	__mutex_unlock (&_hurd_siglock);
 
       if (_hurd_itimer_port == MACH_PORT_NULL)
 	{
@@ -316,9 +313,6 @@ setitimer_locked (const struct itimerval *new, struct itimerval *old,
       _hurd_itimer_thread_suspended = 0;
     }
 
-  __spin_unlock (&_hurd_itimer_lock);
-  _hurd_critical_section_unlock (crit);
-
   if (old != NULL)
     {
       old->it_value = remaining;
@@ -327,8 +321,6 @@ setitimer_locked (const struct itimerval *new, struct itimerval *old,
   return 0;
 
  out:
-  __spin_unlock (&_hurd_itimer_lock);
-  _hurd_critical_section_unlock (crit);
   return __hurd_fail (err);
 }
 
@@ -339,7 +331,6 @@ int
 __setitimer (enum __itimer_which which, const struct itimerval *new,
 	     struct itimerval *old)
 {
-  void *crit;
   int ret;
 
   switch (which)
@@ -356,9 +347,13 @@ __setitimer (enum __itimer_which which, const struct itimerval *new,
     }
 
 retry:
-  crit = _hurd_critical_section_lock ();
+  HURD_CRITICAL_BEGIN;
+  __mutex_lock (&_hurd_siglock);
   __spin_lock (&_hurd_itimer_lock);
-  ret = setitimer_locked (new, old, crit, 0);
+  ret = setitimer_locked (new, old);
+  __spin_unlock (&_hurd_itimer_lock);
+  __mutex_unlock (&_hurd_siglock);
+  HURD_CRITICAL_END;
   if (ret == -1 && errno == EINTR)
     /* Got a signal while inside an RPC of the critical section, retry again */
     goto retry;
@@ -373,12 +368,15 @@ fork_itimer (void)
 
   struct itimerval it;
 
+  __mutex_lock (&_hurd_siglock);
   __spin_lock (&_hurd_itimer_lock);
   _hurd_itimer_thread = MACH_PORT_NULL;
   it = _hurd_itimerval;
   it.it_value = it.it_interval;
 
-  setitimer_locked (&it, NULL, NULL, 0);
+  setitimer_locked (&it, NULL);
+  __spin_unlock (&_hurd_itimer_lock);
+  __mutex_unlock (&_hurd_siglock);
 
   (void) &fork_itimer;		/* Avoid gcc optimizing out the function.  */
 }
-- 
2.51.0