ompi/group: fix race condition in group operations

AboorvaDevarajan · AboorvaDevarajan · commit 92ac56b30c51 · 2021-03-04T08:29:42.000-05:00
In group operation (eg, MPI_Group_union) each proc pointers in one group
is compared with other to check if it is equal, but during the execution of
the inner loop if a thread from the same process updates the proc pointer
(because of sentinels, same proc can have two different pointers) this will
result in inconsistent / erroneous comparison result. so take a proc lock
in group operations to avoid race condition when comparing the proc pointers
in groups.

In order to avoid recursive locks in ompi_group_peer_lookup pass a flag
with `OMPI_GROUP_LOCK` bit unset, if the `OMPI_GROUP_LOCK` bit is set, proc
lock is taken else the proc lock is avoided in ompi_group_peer_lookup path.

Signed-off-by: Aboorva Devarajan &lt;abodevar@in.ibm.com&gt;
diff --git a/ompi/group/group.c b/ompi/group/group.c
@@ -495,7 +495,7 @@ int ompi_group_compare(ompi_group_t *group1,
     bool similar, identical;
     ompi_group_t *group1_pointer, *group2_pointer;
     ompi_proc_t *proc1_pointer, *proc2_pointer;
-
+    opal_mutex_lock (&ompi_proc_lock); 
     /* check for same groups */
     if( group1 == group2 ) {
         *result=MPI_IDENT;
@@ -505,6 +505,7 @@ int ompi_group_compare(ompi_group_t *group1,
     /* check to see if either is MPI_GROUP_NULL or MPI_GROUP_EMPTY */
     if( ( MPI_GROUP_EMPTY == group1 ) || ( MPI_GROUP_EMPTY == group2 ) ) {
         *result=MPI_UNEQUAL;
+        opal_mutex_unlock (&ompi_proc_lock); 
         return return_value;
     }
 
@@ -516,6 +517,7 @@ int ompi_group_compare(ompi_group_t *group1,
     if( group1_pointer->grp_proc_count != group2_pointer->grp_proc_count ) {
         /* if not same size - return */
         *result=MPI_UNEQUAL;
+        opal_mutex_unlock (&ompi_proc_lock); 
         return return_value;
     }
 
@@ -524,11 +526,11 @@ int ompi_group_compare(ompi_group_t *group1,
     similar=true;
     identical=true;
     for(proc1=0 ; proc1 < group1_pointer->grp_proc_count ; proc1++ ) {
-        proc1_pointer= ompi_group_peer_lookup(group1_pointer,proc1);
+        proc1_pointer= ompi_group_peer_lookup_nolock(group1_pointer,proc1);
         /* loop over group2 processes to find "match" */
         match=-1;
         for(proc2=0 ; proc2 < group2_pointer->grp_proc_count ; proc2++ ) {
-            proc2_pointer=ompi_group_peer_lookup(group2_pointer,proc2);
+            proc2_pointer=ompi_group_peer_lookup_nolock(group2_pointer,proc2);
             if( proc1_pointer == proc2_pointer ) {
                 if(proc1 != proc2 ) {
                     identical=false;
@@ -552,7 +554,7 @@ int ompi_group_compare(ompi_group_t *group1,
     } else {
         *result=MPI_UNEQUAL;
     }
-
+    opal_mutex_unlock (&ompi_proc_lock); 
     return return_value;
 }
 
diff --git a/ompi/group/group.h b/ompi/group/group.h
@@ -68,6 +68,9 @@ struct ompi_group_bitmap_data_t
     int            grp_bitmap_array_len; /* length of the bit array */
 };
 
+#define OMPI_GROUP_ALLOCATE 0x01
+#define OMPI_GROUP_LOCK 0x02
+
 /**
  * Group structure
  * Currently we have four formats for storing the process pointers that are members
@@ -337,15 +340,14 @@ int ompi_group_minloc (int list[], int length);
  * @brief Helper function for retreiving the proc of a group member in a dense group
  *
  * This function exists to handle the translation of sentinel group members to real
- * ompi_proc_t's. If a sentinel value is found and allocate is true then this function
+ * ompi_proc_t's. If a sentinel value is found and allocate bit is true then this function
  * looks for an existing ompi_proc_t using ompi_proc_for_name which will allocate a
- * ompi_proc_t if one does not exist. If allocate is false then sentinel values translate
+ * ompi_proc_t if one does not exist. If allocate bit is false then sentinel values translate
  * to NULL.
  */
-static inline struct ompi_proc_t *ompi_group_dense_lookup (ompi_group_t *group, const int peer_id, const bool allocate)
+static inline struct ompi_proc_t *ompi_group_dense_lookup (ompi_group_t *group, const int peer_id, unsigned char flags)
 {
-    ompi_proc_t *proc;
-
+    ompi_proc_t *proc, *real_proc;
 #if OPAL_ENABLE_DEBUG
     if (peer_id >= group->grp_proc_count) {
         opal_output(0, "ompi_group_dense_lookup: invalid peer index (%d)", peer_id);
@@ -356,13 +358,17 @@ static inline struct ompi_proc_t *ompi_group_dense_lookup (ompi_group_t *group,
     proc = group->grp_proc_pointers[peer_id];
 
     if (OPAL_UNLIKELY(ompi_proc_is_sentinel (proc))) {
-        if (!allocate) {
+        if (flags == 0) {
             return NULL;
         }
-
         /* replace sentinel value with an actual ompi_proc_t */
-        ompi_proc_t *real_proc =
-            (ompi_proc_t *) ompi_proc_for_name (ompi_proc_sentinel_to_name ((uintptr_t) proc));
+        if ((flags & OMPI_GROUP_LOCK) == OMPI_GROUP_LOCK) {
+            real_proc =
+                (ompi_proc_t *) ompi_proc_for_name (ompi_proc_sentinel_to_name ((uintptr_t) proc));
+        } else {
+            real_proc =
+                (ompi_proc_t *) ompi_proc_for_name_nolock (ompi_proc_sentinel_to_name ((uintptr_t) proc));
+        }
 
         if (opal_atomic_compare_exchange_strong_ptr ((opal_atomic_intptr_t *)(group->grp_proc_pointers + peer_id),
                                                      (intptr_t *) &proc, (intptr_t) real_proc)) {
@@ -379,19 +385,19 @@ static inline struct ompi_proc_t *ompi_group_dense_lookup (ompi_group_t *group,
  * This is the function that iterates through the sparse groups to the dense group
  * to reach the process pointer
  */
-static inline ompi_proc_t *ompi_group_get_proc_ptr (ompi_group_t *group, int rank, const bool allocate)
+static inline ompi_proc_t *ompi_group_get_proc_ptr (ompi_group_t *group, int rank, unsigned char flags)
 {
 #if OMPI_GROUP_SPARSE
     do {
         if (OMPI_GROUP_IS_DENSE(group)) {
-            return ompi_group_dense_lookup (group, rank, allocate);
+            return ompi_group_dense_lookup (group, rank, flags);
         }
         int ranks1 = rank;
         ompi_group_translate_ranks (group, 1, &ranks1, group->grp_parent_group_ptr, &rank);
         group = group->grp_parent_group_ptr;
     } while (1);
 #else
-    return ompi_group_dense_lookup (group, rank, allocate);
+    return ompi_group_dense_lookup (group, rank, flags);
 #endif
 }
 
@@ -420,12 +426,17 @@ static inline opal_process_name_t ompi_group_get_proc_name (ompi_group_t *group,
  */
 static inline struct ompi_proc_t* ompi_group_peer_lookup(ompi_group_t *group, int peer_id)
 {
-    return ompi_group_get_proc_ptr (group, peer_id, true);
+    return ompi_group_get_proc_ptr (group, peer_id, (unsigned char) (OMPI_GROUP_ALLOCATE | OMPI_GROUP_LOCK));
+}
+
+static inline struct ompi_proc_t* ompi_group_peer_lookup_nolock(ompi_group_t *group, int peer_id)
+{
+    return ompi_group_get_proc_ptr (group, peer_id, (unsigned char) OMPI_GROUP_ALLOCATE);
 }
 
 static inline struct ompi_proc_t *ompi_group_peer_lookup_existing (ompi_group_t *group, int peer_id)
 {
-    return ompi_group_get_proc_ptr (group, peer_id, false);
+    return ompi_group_get_proc_ptr (group, peer_id, (unsigned char) 0);
 }
 
 #if OPAL_ENABLE_FT_MPI
diff --git a/ompi/group/group_plist.c b/ompi/group/group_plist.c
@@ -163,11 +163,12 @@ int ompi_group_union (ompi_group_t* group1, ompi_group_t* group2,
     if (OPAL_SUCCESS != rc) {
         return rc;
     }
-
+    opal_mutex_lock (&ompi_proc_lock);
     /* check group2 elements to see if they need to be included in the list */
     overlap_count = ompi_group_dense_overlap (group1, group2, &bitmap);
     if (0 > overlap_count) {
         OBJ_DESTRUCT(&bitmap);
+        opal_mutex_unlock (&ompi_proc_lock); 
         return overlap_count;
     }
 
@@ -176,13 +177,15 @@ int ompi_group_union (ompi_group_t* group1, ompi_group_t* group2,
         *new_group = MPI_GROUP_EMPTY;
         OBJ_RETAIN(MPI_GROUP_EMPTY);
         OBJ_DESTRUCT(&bitmap);
+        opal_mutex_unlock (&ompi_proc_lock); 
         return MPI_SUCCESS;
     }
 
     /* get new group struct */
     new_group_pointer = ompi_group_allocate(new_group_size);
     if (NULL == new_group_pointer) {
         OBJ_DESTRUCT(&bitmap);
+        opal_mutex_unlock (&ompi_proc_lock); 
         return MPI_ERR_GROUP;
     }
 
@@ -218,7 +221,7 @@ int ompi_group_union (ompi_group_t* group1, ompi_group_t* group2,
     }
 
     *new_group = (MPI_Group) new_group_pointer;
-
+    opal_mutex_unlock (&ompi_proc_lock); 
     return OMPI_SUCCESS;
 }
 
@@ -245,11 +248,12 @@ int ompi_group_difference(ompi_group_t* group1, ompi_group_t* group2,
     if (OPAL_SUCCESS != rc) {
         return rc;
     }
-
+    opal_mutex_lock (&ompi_proc_lock);     
     /* check group2 elements to see if they need to be included in the list */
     overlap_count = ompi_group_dense_overlap (group2, group1, &bitmap);
     if (0 > overlap_count) {
         OBJ_DESTRUCT(&bitmap);
+        opal_mutex_unlock (&ompi_proc_lock); 
         return overlap_count;
     }
 
@@ -258,13 +262,15 @@ int ompi_group_difference(ompi_group_t* group1, ompi_group_t* group2,
         *new_group = MPI_GROUP_EMPTY;
         OBJ_RETAIN(MPI_GROUP_EMPTY);
         OBJ_DESTRUCT(&bitmap);
+        opal_mutex_unlock (&ompi_proc_lock); 
         return MPI_SUCCESS;
     }
 
     /* allocate a new ompi_group_t structure */
     new_group_pointer = ompi_group_allocate(new_group_size);
     if( NULL == new_group_pointer ) {
         OBJ_DESTRUCT(&bitmap);
+        opal_mutex_unlock (&ompi_proc_lock);    
         return MPI_ERR_GROUP;
     }
 
@@ -292,6 +298,6 @@ int ompi_group_difference(ompi_group_t* group1, ompi_group_t* group2,
     }
 
     *new_group = (MPI_Group)new_group_pointer;
-
+    opal_mutex_unlock (&ompi_proc_lock); 
     return OMPI_SUCCESS;
 }
diff --git a/ompi/mca/common/monitoring/common_monitoring.h b/ompi/mca/common/monitoring/common_monitoring.h
@@ -75,7 +75,7 @@ static inline int mca_common_monitoring_get_world_rank(int dest, ompi_group_t *g
     opal_process_name_t tmp;
 
     /* find the processor of the destination */
-    ompi_proc_t *proc = ompi_group_get_proc_ptr(group, dest, true);
+    ompi_proc_t *proc = ompi_group_get_proc_ptr(group, dest, OMPI_GROUP_ALLOCATE | OMPI_GROUP_LOCK);
     if( ompi_proc_is_sentinel(proc) ) {
         tmp = ompi_proc_sentinel_to_name((uintptr_t)proc);
     } else {
diff --git a/ompi/proc/proc.c b/ompi/proc/proc.c
@@ -48,14 +48,12 @@
 #include "ompi/mca/pml/pml.h"
 
 opal_list_t  ompi_proc_list = {{0}};
-static opal_mutex_t ompi_proc_lock;
 static opal_hash_table_t ompi_proc_hash;
-
+opal_mutex_t ompi_proc_lock; 
 ompi_proc_t* ompi_proc_local_proc = NULL;
 
 static void ompi_proc_construct(ompi_proc_t* proc);
 static void ompi_proc_destruct(ompi_proc_t* proc);
-static ompi_proc_t *ompi_proc_for_name_nolock (const opal_process_name_t proc_name);
 
 OBJ_CLASS_INSTANCE(
     ompi_proc_t,
@@ -191,7 +189,7 @@ opal_proc_t *ompi_proc_lookup (const opal_process_name_t proc_name)
     return NULL;
 }
 
-static ompi_proc_t *ompi_proc_for_name_nolock (const opal_process_name_t proc_name)
+ompi_proc_t *ompi_proc_for_name_nolock (const opal_process_name_t proc_name)
 {
     ompi_proc_t *proc = NULL;
     int ret;
diff --git a/ompi/proc/proc.h b/ompi/proc/proc.h
@@ -100,7 +100,7 @@ OBJ_CLASS_DECLARATION(ompi_proc_t);
  */
 OMPI_DECLSPEC extern ompi_proc_t* ompi_proc_local_proc;
 OMPI_DECLSPEC extern opal_list_t  ompi_proc_list;
-
+OMPI_DECLSPEC extern opal_mutex_t  ompi_proc_lock; 
 /* ******************************************************************** */
 
 
@@ -381,7 +381,7 @@ OMPI_DECLSPEC int ompi_proc_refresh(void);
  * the ompi_proc_t on a communicator.
  */
 OMPI_DECLSPEC opal_proc_t *ompi_proc_for_name (const opal_process_name_t proc_name);
-
+OMPI_DECLSPEC ompi_proc_t *ompi_proc_for_name_nolock (const opal_process_name_t proc_name);
 
 OMPI_DECLSPEC opal_proc_t *ompi_proc_lookup (const opal_process_name_t proc_name);
 
diff --git a/ompi/runtime/ompi_mpi_abort.c b/ompi/runtime/ompi_mpi_abort.c
@@ -100,15 +100,17 @@ static void try_kill_peers(ompi_communicator_t *comm,
         } else {
             assert(count <= nprocs);
             procs[count++] =
-                *OMPI_CAST_RTE_NAME(&ompi_group_get_proc_ptr(comm->c_remote_group, i, true)->super.proc_name);
+                *OMPI_CAST_RTE_NAME(&ompi_group_get_proc_ptr(comm->c_remote_group, i, 
+                                     OMPI_GROUP_ALLOCATE | OMPI_GROUP_LOCK)->super.proc_name);
         }
     }
 
     /* if requested, kill off remote group procs too */
     for (i = 0; i < ompi_comm_remote_size(comm); ++i) {
         assert(count <= nprocs);
         procs[count++] =
-            *OMPI_CAST_RTE_NAME(&ompi_group_get_proc_ptr(comm->c_remote_group, i, true)->super.proc_name);
+            *OMPI_CAST_RTE_NAME(&ompi_group_get_proc_ptr(comm->c_remote_group, i, 
+                                     OMPI_GROUP_ALLOCATE | OMPI_GROUP_LOCK)->super.proc_name);
     }
 
     if (nprocs > 0) {

Original file line number	Diff line number	Diff line change
`@@ -163,11 +163,12 @@ int ompi_group_union (ompi_group_t* group1, ompi_group_t* group2,`
`163`	`163`	`if (OPAL_SUCCESS != rc) {`
`164`	`164`	`return rc;`
`165`	`165`	`}`
`166`		`-`
	`166`	`+ opal_mutex_lock (&ompi_proc_lock);`
`167`	`167`	`/* check group2 elements to see if they need to be included in the list */`
`168`	`168`	`overlap_count = ompi_group_dense_overlap (group1, group2, &bitmap);`
`169`	`169`	`if (0 > overlap_count) {`
`170`	`170`	`OBJ_DESTRUCT(&bitmap);`
	`171`	`+ opal_mutex_unlock (&ompi_proc_lock);`
`171`	`172`	`return overlap_count;`
`172`	`173`	`}`
`173`	`174`
`@@ -176,13 +177,15 @@ int ompi_group_union (ompi_group_t* group1, ompi_group_t* group2,`
`176`	`177`	`*new_group = MPI_GROUP_EMPTY;`
`177`	`178`	`OBJ_RETAIN(MPI_GROUP_EMPTY);`
`178`	`179`	`OBJ_DESTRUCT(&bitmap);`
	`180`	`+ opal_mutex_unlock (&ompi_proc_lock);`
`179`	`181`	`return MPI_SUCCESS;`
`180`	`182`	`}`
`181`	`183`
`182`	`184`	`/* get new group struct */`
`183`	`185`	`new_group_pointer = ompi_group_allocate(new_group_size);`
`184`	`186`	`if (NULL == new_group_pointer) {`
`185`	`187`	`OBJ_DESTRUCT(&bitmap);`
	`188`	`+ opal_mutex_unlock (&ompi_proc_lock);`
`186`	`189`	`return MPI_ERR_GROUP;`
`187`	`190`	`}`
`188`	`191`
`@@ -218,7 +221,7 @@ int ompi_group_union (ompi_group_t* group1, ompi_group_t* group2,`
`218`	`221`	`}`
`219`	`222`
`220`	`223`	`*new_group = (MPI_Group) new_group_pointer;`
`221`		`-`
	`224`	`+ opal_mutex_unlock (&ompi_proc_lock);`
`222`	`225`	`return OMPI_SUCCESS;`
`223`	`226`	`}`
`224`	`227`
`@@ -245,11 +248,12 @@ int ompi_group_difference(ompi_group_t* group1, ompi_group_t* group2,`
`245`	`248`	`if (OPAL_SUCCESS != rc) {`
`246`	`249`	`return rc;`
`247`	`250`	`}`
`248`		`-`
	`251`	`+ opal_mutex_lock (&ompi_proc_lock);`
`249`	`252`	`/* check group2 elements to see if they need to be included in the list */`
`250`	`253`	`overlap_count = ompi_group_dense_overlap (group2, group1, &bitmap);`
`251`	`254`	`if (0 > overlap_count) {`
`252`	`255`	`OBJ_DESTRUCT(&bitmap);`
	`256`	`+ opal_mutex_unlock (&ompi_proc_lock);`
`253`	`257`	`return overlap_count;`
`254`	`258`	`}`
`255`	`259`
`@@ -258,13 +262,15 @@ int ompi_group_difference(ompi_group_t* group1, ompi_group_t* group2,`
`258`	`262`	`*new_group = MPI_GROUP_EMPTY;`
`259`	`263`	`OBJ_RETAIN(MPI_GROUP_EMPTY);`
`260`	`264`	`OBJ_DESTRUCT(&bitmap);`
	`265`	`+ opal_mutex_unlock (&ompi_proc_lock);`
`261`	`266`	`return MPI_SUCCESS;`
`262`	`267`	`}`
`263`	`268`
`264`	`269`	`/* allocate a new ompi_group_t structure */`
`265`	`270`	`new_group_pointer = ompi_group_allocate(new_group_size);`
`266`	`271`	`if( NULL == new_group_pointer ) {`
`267`	`272`	`OBJ_DESTRUCT(&bitmap);`
	`273`	`+ opal_mutex_unlock (&ompi_proc_lock);`
`268`	`274`	`return MPI_ERR_GROUP;`
`269`	`275`	`}`
`270`	`276`
`@@ -292,6 +298,6 @@ int ompi_group_difference(ompi_group_t* group1, ompi_group_t* group2,`
`292`	`298`	`}`
`293`	`299`
`294`	`300`	`*new_group = (MPI_Group)new_group_pointer;`
`295`		`-`
	`301`	`+ opal_mutex_unlock (&ompi_proc_lock);`
`296`	`302`	`return OMPI_SUCCESS;`
`297`	`303`	`}`
Original file line number	Diff line number	Diff line change
`@@ -100,15 +100,17 @@ static void try_kill_peers(ompi_communicator_t *comm,`
`100`	`100`	`} else {`
`101`	`101`	`assert(count <= nprocs);`
`102`	`102`	`procs[count++] =`
`103`		`- *OMPI_CAST_RTE_NAME(&ompi_group_get_proc_ptr(comm->c_remote_group, i, true)->super.proc_name);`
	`103`	`+ *OMPI_CAST_RTE_NAME(&ompi_group_get_proc_ptr(comm->c_remote_group, i,`
	`104`	`+ OMPI_GROUP_ALLOCATE \| OMPI_GROUP_LOCK)->super.proc_name);`
`104`	`105`	`}`
`105`	`106`	`}`
`106`	`107`
`107`	`108`	`/* if requested, kill off remote group procs too */`
`108`	`109`	`for (i = 0; i < ompi_comm_remote_size(comm); ++i) {`
`109`	`110`	`assert(count <= nprocs);`
`110`	`111`	`procs[count++] =`
`111`		`- *OMPI_CAST_RTE_NAME(&ompi_group_get_proc_ptr(comm->c_remote_group, i, true)->super.proc_name);`
	`112`	`+ *OMPI_CAST_RTE_NAME(&ompi_group_get_proc_ptr(comm->c_remote_group, i,`
	`113`	`+ OMPI_GROUP_ALLOCATE \| OMPI_GROUP_LOCK)->super.proc_name);`
`112`	`114`	`}`
`113`	`115`
`114`	`116`	`if (nprocs > 0) {`