ikev1: Prevent deadlock when checking for duplicate IKEv1 SAs

Previously, the current segment was held while checking for duplicate
SAs, which requires acquiring all segments.  If multiple threads did this
concurrently this resulted in a deadlock as they couldn't acquire the
segments held by the other threads attempting to do the same.  With the
default configuration only one segment is used, which prevents the problem
as only one thread can check in an IKE SA concurrently.

Fixes: a064eaa8a63a ("Handling of initial contact")

diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c
index 37d69874d5079bf1be9b2103b6bf421194e72ac7..9ebdae7f69722132b50773cb1a563908c502a53f 100644 (file)
--- a/src/libcharon/sa/ike_sa_manager.c
+++ b/src/libcharon/sa/ike_sa_manager.c
@@ -1628,8 +1628,24 @@ METHOD(ike_sa_manager_t, checkin, void,
                         * delete any existing IKE_SAs with that peer. */
                        if (ike_sa->has_condition(ike_sa, COND_INIT_CONTACT_SEEN))
                        {
+                               /* We can't hold the segment locked while checking the
+                                * uniqueness as this could lead to deadlocks.  We mark the
+                                * entry as checked out while we release the lock so no other
+                                * thread can acquire it.  Since it is not yet in the list of
+                                * connected peers that will not cause a deadlock as no other
+                                * caller of check_unqiueness() will try to check out this SA */
+                               entry->checked_out = TRUE;
+                               unlock_single_segment(this, segment);
+
                                this->public.check_uniqueness(&this->public, ike_sa, TRUE);
                                ike_sa->set_condition(ike_sa, COND_INIT_CONTACT_SEEN, FALSE);
+
+                               /* The entry could have been modified in the mean time, e.g.
+                                * because another SA was added/removed next to it or another
+                                * thread is waiting, but it should still exist, so there is no
+                                * need for a lookup via get_entry_by... */
+                               lock_single_segment(this, segment);
+                               entry->checked_out = FALSE;
                        }
                }