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
