opal/sync: fix race condition

This commit fixes a race condition discovered by @artpol84. The race
happens when a signalling thread decrements the sync count to 0 then
goes to sleep. If the waiting thread runs and detects the count == 0
before going to sleep on the condition variable it will destroy the
condition variable while the signalling thread is potentially still
processing the completion. The fix is to add a non-atomic member to
the sync structure that indicates another process is handling
completion. Since the member will only be set to false by the
initiating thread and the completing thread the variable does not need
to be protected. When destoying a condition variable the waiting
thread needs to wait until the singalling thread is finished.

Thanks to @artpol84 for tracking this down.

Fixes open-mpi#1813

Signed-off-by: Nathan Hjelm <[email protected]>

Author: hjelmn

opal/threads/wait_sync.h

@@ -24,24 +24,37 @@ typedef struct ompi_wait_sync_t {
     pthread_mutex_t lock;
     struct ompi_wait_sync_t *next;
     struct ompi_wait_sync_t *prev;
+    volatile bool signaling;
 } ompi_wait_sync_t;
 
 #define REQUEST_PENDING        (void*)0L
 #define REQUEST_COMPLETED      (void*)1L
 
 #define SYNC_WAIT(sync)                 (opal_using_threads() ? sync_wait_mt (sync) : sync_wait_st (sync))
 
+/* The loop in release handles a race condition between the signaling
+ * thread and the destruction of the condition variable. The signaling
+ * member will be set to false after the final signaling thread has
+ * finished opertating on the sync object. This is done to avoid
+ * extra atomics in the singalling function and keep it as fast
+ * as possible. Note that the race window is small so spinning here
+ * is more optimal than sleeping since this macro is called in
+ * the critical path. */
 #define WAIT_SYNC_RELEASE(sync)                       \
     if (opal_using_threads()) {                       \
-       pthread_cond_destroy(&(sync)->condition);      \
-       pthread_mutex_destroy(&(sync)->lock);          \
+        while ((sync)->signaling) {                  \
+            continue;                                 \
+        }                                             \
+        pthread_cond_destroy(&(sync)->condition);     \
+        pthread_mutex_destroy(&(sync)->lock);         \
     }
 
 #define WAIT_SYNC_SIGNAL(sync)                        \
     if (opal_using_threads()) {                       \
         pthread_mutex_lock(&(sync->lock));            \
         pthread_cond_signal(&sync->condition);        \
         pthread_mutex_unlock(&(sync->lock));          \
+        sync->signaling = false;                     \
     }
 
 OPAL_DECLSPEC int sync_wait_mt(ompi_wait_sync_t *sync);
@@ -61,6 +74,7 @@ static inline int sync_wait_st (ompi_wait_sync_t *sync)
         (sync)->next = NULL;                                    \
         (sync)->prev = NULL;                                    \
         (sync)->status = 0;                                     \
+        (sync)->signaling = true;                               \
         if (opal_using_threads()) {                             \
             pthread_cond_init (&(sync)->condition, NULL);       \
             pthread_mutex_init (&(sync)->lock, NULL);           \
@@ -81,8 +95,9 @@ static inline void wait_sync_update(ompi_wait_sync_t *sync, int updates, int sta
         }
     } else {
         /* this is an error path so just use the atomic */
-        opal_atomic_swap_32 (&sync->count, 0);
         sync->status = OPAL_ERROR;
+        opal_atomic_wmb ();
+        opal_atomic_swap_32 (&sync->count, 0);
     }
     WAIT_SYNC_SIGNAL(sync);
 }