trap-manager: Prevent deadlock when installing trap policies

Because the write lock was held while calling add_policies() on
child_sa_t, which finishes with a call to child_state_change() on bus_t,
a deadlock would ensue if CHILD_SAs are concurrently being established,
which also causes a call to child_state_change() that will require
the read lock in trap_manager_t.

No locks are now being held while creating the CHILD_SA and installing the
trap policies.

diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c
index 1f66d6ceb0196d8e900192e64c2da13542af9917..9f8068ef13a0b44826d206611a2712cfe594a121 100644 (file)
--- a/src/libcharon/sa/trap_manager.c
+++ b/src/libcharon/sa/trap_manager.c
@@ -19,7 +19,6 @@
 #include <hydra.h>
 #include <daemon.h>
 #include <threading/rwlock.h>
-#include <threading/thread_value.h>
 #include <collections/linked_list.h>
 
 
@@ -62,11 +61,6 @@ struct private_trap_manager_t {
         */
        rwlock_t *lock;
 
-       /**
-        * track if the current thread is installing a trap policy
-        */
-       thread_value_t *installing;
-
        /**
         * listener to track acquiring IKE_SAs
         */
@@ -77,6 +71,8 @@ struct private_trap_manager_t {
  * A installed trap entry
  */
 typedef struct {
+       /** name of the trapped CHILD_SA */
+       char *name;
        /** ref to peer_cfg to initiate */
        peer_cfg_t *peer_cfg;
        /** ref to instanciated CHILD_SA */
@@ -94,6 +90,7 @@ static void destroy_entry(entry_t *entry)
 {
        entry->child_sa->destroy(entry->child_sa);
        entry->peer_cfg->destroy(entry->peer_cfg);
+       free(entry->name);
        free(entry);
 }
 
@@ -137,27 +134,42 @@ METHOD(trap_manager_t, install, u_int32_t,
        }
 
        this->lock->write_lock(this->lock);
-       this->installing->set(this->installing, this);
        enumerator = this->traps->create_enumerator(this->traps);
        while (enumerator->enumerate(enumerator, &entry))
        {
-               if (streq(entry->child_sa->get_name(entry->child_sa),
-                                 child->get_name(child)))
+               if (streq(entry->name, child->get_name(child)))
                {
-                       this->traps->remove_at(this->traps, enumerator);
                        found = entry;
+                       if (entry->child_sa)
+                       {       /* replace it with an updated version, if already installed */
+                               this->traps->remove_at(this->traps, enumerator);
+                       }
                        break;
                }
        }
        enumerator->destroy(enumerator);
 
        if (found)
-       {       /* config might have changed so update everything */
-               DBG1(DBG_CFG, "updating already routed CHILD_SA '%s'",
-                        child->get_name(child));
+       {
+               if (!found->child_sa)
+               {
+                       DBG1(DBG_CFG, "CHILD_SA '%s' is already being routed", found->name);
+                       this->lock->unlock(this->lock);
+                       return 0;
+               }
+               /* config might have changed so update everything */
+               DBG1(DBG_CFG, "updating already routed CHILD_SA '%s'", found->name);
                reqid = found->child_sa->get_reqid(found->child_sa);
        }
 
+       INIT(entry,
+               .name = strdup(child->get_name(child)),
+               .peer_cfg = peer->get_ref(peer),
+       );
+       this->traps->insert_first(this->traps, entry);
+       /* don't hold lock while creating CHILD_SA and installing policies */
+       this->lock->unlock(this->lock);
+
        /* create and route CHILD_SA */
        child_sa = child_sa_create(me, other, child, reqid, FALSE);
 
@@ -185,24 +197,19 @@ METHOD(trap_manager_t, install, u_int32_t,
        if (status != SUCCESS)
        {
                DBG1(DBG_CFG, "installing trap failed");
+               this->lock->write_lock(this->lock);
+               this->traps->remove(this->traps, entry, NULL);
+               this->lock->unlock(this->lock);
+               entry->child_sa = child_sa;
+               destroy_entry(entry);
                reqid = 0;
-               /* hold off destroying the CHILD_SA until we released the lock */
        }
        else
        {
-               INIT(entry,
-                       .child_sa = child_sa,
-                       .peer_cfg = peer->get_ref(peer),
-               );
-               this->traps->insert_last(this->traps, entry);
                reqid = child_sa->get_reqid(child_sa);
-       }
-       this->installing->set(this->installing, NULL);
-       this->lock->unlock(this->lock);
-
-       if (status != SUCCESS)
-       {
-               child_sa->destroy(child_sa);
+               this->lock->write_lock(this->lock);
+               entry->child_sa = child_sa;
+               this->lock->unlock(this->lock);
        }
        if (found)
        {
@@ -221,7 +228,8 @@ METHOD(trap_manager_t, uninstall, bool,
        enumerator = this->traps->create_enumerator(this->traps);
        while (enumerator->enumerate(enumerator, &entry))
        {
-               if (entry->child_sa->get_reqid(entry->child_sa) == reqid)
+               if (entry->child_sa &&
+                       entry->child_sa->get_reqid(entry->child_sa) == reqid)
                {
                        this->traps->remove_at(this->traps, enumerator);
                        found = entry;
@@ -236,7 +244,6 @@ METHOD(trap_manager_t, uninstall, bool,
                DBG1(DBG_CFG, "trap %d not found to uninstall", reqid);
                return FALSE;
        }
-
        destroy_entry(found);
        return TRUE;
 }
@@ -247,6 +254,10 @@ METHOD(trap_manager_t, uninstall, bool,
 static bool trap_filter(rwlock_t *lock, entry_t **entry, peer_cfg_t **peer_cfg,
                                                void *none, child_sa_t **child_sa)
 {
+       if (!(*entry)->child_sa)
+       {       /* skip entries that are currently being installed */
+               return FALSE;
+       }
        if (peer_cfg)
        {
                *peer_cfg = (*entry)->peer_cfg;
@@ -271,28 +282,24 @@ METHOD(trap_manager_t, find_reqid, u_int32_t,
        private_trap_manager_t *this, child_cfg_t *child)
 {
        enumerator_t *enumerator;
-       child_cfg_t *current;
        entry_t *entry;
        u_int32_t reqid = 0;
 
-       if (this->installing->get(this->installing))
-       {       /* current thread holds the lock */
-               return reqid;
-       }
        this->lock->read_lock(this->lock);
        enumerator = this->traps->create_enumerator(this->traps);
        while (enumerator->enumerate(enumerator, &entry))
        {
-               current = entry->child_sa->get_config(entry->child_sa);
-               if (streq(current->get_name(current), child->get_name(child)))
+               if (streq(entry->name, child->get_name(child)))
                {
-                       reqid = entry->child_sa->get_reqid(entry->child_sa);
+                       if (entry->child_sa)
+                       {
+                               reqid = entry->child_sa->get_reqid(entry->child_sa);
+                       }
                        break;
                }
        }
        enumerator->destroy(enumerator);
        this->lock->unlock(this->lock);
-
        return reqid;
 }
 
@@ -310,7 +317,8 @@ METHOD(trap_manager_t, acquire, void,
        enumerator = this->traps->create_enumerator(this->traps);
        while (enumerator->enumerate(enumerator, &entry))
        {
-               if (entry->child_sa->get_reqid(entry->child_sa) == reqid)
+               if (entry->child_sa &&
+                       entry->child_sa->get_reqid(entry->child_sa) == reqid)
                {
                        found = entry;
                        break;
@@ -445,7 +453,6 @@ METHOD(trap_manager_t, destroy, void,
 {
        charon->bus->remove_listener(charon->bus, &this->listener.listener);
        this->traps->destroy_function(this->traps, (void*)destroy_entry);
-       this->installing->destroy(this->installing);
        this->lock->destroy(this->lock);
        free(this);
 }
@@ -476,7 +483,6 @@ trap_manager_t *trap_manager_create(void)
                },
                .traps = linked_list_create(),
                .lock = rwlock_create(RWLOCK_TYPE_DEFAULT),
-               .installing = thread_value_create(NULL),
        );
        charon->bus->add_listener(charon->bus, &this->listener.listener);