vici: Improve handling of start action when reloading configs

The previous code had some issues because it handled each child config
separately.  Not only was this quite inefficient because all IKE_SAs had
to be enumerated for every config, it also caused problems with the check
for other CHILD_SAs in order to decide whether to delete the IKE_SA or
not.  Because CHILD_SAs are deleted with an INFORMATIONAL exchange, they
are not immediately gone.  This caused a race condition and with more
than one child config and SAs the IKE_SA could be kept because it
could appear as if other, unrelated CHILD_SAs were still there.

Another race condition, which is fixed by the previous commit, occurred
when only changing child configs.  Then it could happen that the code
deemed the IKE_SA empty and a delete for it was queued.  If that happened
while the IKE_SA was deleting one of the CHILD_SAs (or was busy with some
other exchange), the IKE_SA was not switched to IKE_DELETING.  So it
looked usable and create-child tasks for the updated configs might have
gotten queued.  Unfortunately, once the ike-delete task is eventually
executed, these tasks would be gone and the replacement CHILD_SAs never
created.  This commit additionally avoids actually deleting the IKE_SA
even if all child configs change or get removed if any new CHILD_SAs are
to be initiated.

diff --git a/src/libcharon/plugins/vici/vici_config.c b/src/libcharon/plugins/vici/vici_config.c
index c858e9945c4fb957bfa086051936556f01bac958..c46b8872b9d0f9cc56cb5fecaba01c1a8334e86c 100644 (file)
--- a/src/libcharon/plugins/vici/vici_config.c
+++ b/src/libcharon/plugins/vici/vici_config.c
@@ -2257,7 +2257,7 @@ static void run_start_action(private_vici_config_t *this, peer_cfg_t *peer_cfg,
 
        if (action & ACTION_TRAP)
        {
-               DBG1(DBG_CFG, "installing '%s'", child_cfg->get_name(child_cfg));
+               DBG1(DBG_CFG, "vici installing '%s'", child_cfg->get_name(child_cfg));
                switch (child_cfg->get_mode(child_cfg))
                {
                        case MODE_PASS:
@@ -2274,7 +2274,7 @@ static void run_start_action(private_vici_config_t *this, peer_cfg_t *peer_cfg,
 
        if (action & ACTION_START)
        {
-               DBG1(DBG_CFG, "initiating '%s'", child_cfg->get_name(child_cfg));
+               DBG1(DBG_CFG, "vici initiating '%s'", child_cfg->get_name(child_cfg));
                charon->controller->initiate(charon->controller,
                                        peer_cfg->get_ref(peer_cfg), child_cfg->get_ref(child_cfg),
                                        NULL, NULL, 0, 0, FALSE);
@@ -2282,25 +2282,116 @@ static void run_start_action(private_vici_config_t *this, peer_cfg_t *peer_cfg,
 }
 
 /**
- * Undo start actions associated with a child config
+ * Type to keep track of unique IDs and names of CHILD_SAs to terminate.
  */
-static void clear_start_action(private_vici_config_t *this, char *peer_name,
-                                                          child_cfg_t *child_cfg)
+typedef struct {
+       uint32_t id;
+       char *name;
+} child_name_id_t;
+
+/**
+ * Terminate given CHILD_SAs and optionally terminate any IKE_SA without other
+ * children.
+ */
+static void terminate_for_action(private_vici_config_t *this, char *peer_name,
+                                                                hashtable_t *to_terminate, bool delete_ike)
 {
        enumerator_t *enumerator, *children;
        child_sa_t *child_sa;
        ike_sa_t *ike_sa;
-       uint32_t id = 0, others;
+       child_name_id_t child_id;
+       uint32_t id;
        array_t *ids = NULL, *ikeids = NULL;
+       bool others;
+
+       enumerator = charon->controller->create_ike_sa_enumerator(
+                                                                                                       charon->controller, TRUE);
+       while (enumerator->enumerate(enumerator, &ike_sa))
+       {
+               if (!streq(ike_sa->get_name(ike_sa), peer_name))
+               {
+                       continue;
+               }
+
+               others = FALSE;
+               children = ike_sa->create_child_sa_enumerator(ike_sa);
+               while (children->enumerate(children, &child_sa))
+               {
+                       if (child_sa->get_state(child_sa) != CHILD_DELETING &&
+                               child_sa->get_state(child_sa) != CHILD_DELETED &&
+                               !to_terminate->get(to_terminate, child_sa->get_name(child_sa)))
+                       {
+                               others = TRUE;
+                               break;
+                       }
+               }
+               children->destroy(children);
+
+               if (delete_ike && (!others || !ike_sa->get_child_count(ike_sa)))
+               {
+                       /* found no children or only matching, delete IKE_SA */
+                       id = ike_sa->get_unique_id(ike_sa);
+                       array_insert_create_value(&ikeids, sizeof(id),
+                                                                         ARRAY_TAIL, &id);
+                       continue;
+               }
+
+               /* otherwise, delete only the matching CHILD_SAs */
+               children = ike_sa->create_child_sa_enumerator(ike_sa);
+               while (children->enumerate(children, &child_sa))
+               {
+                       child_id.name = child_sa->get_name(child_sa);
+
+                       if (child_sa->get_state(child_sa) != CHILD_DELETING &&
+                               child_sa->get_state(child_sa) != CHILD_DELETED &&
+                               to_terminate->get(to_terminate, child_id.name))
+                       {
+                               child_id.id = child_sa->get_unique_id(child_sa);
+                               child_id.name = strdup(child_id.name);
+                               array_insert_create_value(&ids, sizeof(child_id),
+                                                                                 ARRAY_TAIL, &child_id);
+                       }
+               }
+               children->destroy(children);
+       }
+       enumerator->destroy(enumerator);
+
+       while (array_remove(ids, ARRAY_HEAD, &child_id))
+       {
+               DBG1(DBG_CFG, "vici closing CHILD_SA '%s' #%u", child_id.name,
+                        child_id.id);
+               charon->controller->terminate_child(charon->controller,
+                                                                                       child_id.id, NULL, NULL, 0, 0);
+               free(child_id.name);
+       }
+       array_destroy(ids);
+
+       while (array_remove(ikeids, ARRAY_HEAD, &id))
+       {
+               DBG1(DBG_CFG, "vici closing IKE_SA '%s' #%u", peer_name, id);
+               charon->controller->terminate_ike(charon->controller, id,
+                                                                                 FALSE, NULL, NULL, 0, 0);
+       }
+       array_destroy(ikeids);
+}
+
+/**
+ * Clear the start action associated with the given child config. To reduce the
+ * overhead when terminating active SAs, only collect the name.
+ *
+ * Note: The lock must be unlocked when calling this.
+ */
+static void clear_start_action(private_vici_config_t *this, char *peer_name,
+                                                          child_cfg_t *child_cfg, hashtable_t *to_terminate)
+{
        action_t action;
        char *name;
 
        name = child_cfg->get_name(child_cfg);
        action = child_cfg->get_start_action(child_cfg);
-
        if (action & ACTION_TRAP)
        {
-               DBG1(DBG_CFG, "uninstalling '%s'", name);
+               DBG1(DBG_CFG, "vici uninstalling '%s'", name);
                switch (child_cfg->get_mode(child_cfg))
                {
                        case MODE_PASS:
@@ -2313,111 +2404,51 @@ static void clear_start_action(private_vici_config_t *this, char *peer_name,
                                break;
                }
        }
-
        if (action & ACTION_START)
        {
-               enumerator = charon->controller->create_ike_sa_enumerator(
-                                                                                                       charon->controller, TRUE);
-               while (enumerator->enumerate(enumerator, &ike_sa))
-               {
-                       if (!streq(ike_sa->get_name(ike_sa), peer_name))
-                       {
-                               continue;
-                       }
-                       others = id = 0;
-                       children = ike_sa->create_child_sa_enumerator(ike_sa);
-                       while (children->enumerate(children, &child_sa))
-                       {
-                               if (child_sa->get_state(child_sa) != CHILD_DELETING &&
-                                       child_sa->get_state(child_sa) != CHILD_DELETED)
-                               {
-                                       if (streq(name, child_sa->get_name(child_sa)))
-                                       {
-                                               id = child_sa->get_unique_id(child_sa);
-                                       }
-                                       else
-                                       {
-                                               others++;
-                                       }
-                               }
-                       }
-                       children->destroy(children);
-
-                       if (!ike_sa->get_child_count(ike_sa) || (id && !others))
-                       {
-                               /* found no children or only matching, delete IKE_SA */
-                               id = ike_sa->get_unique_id(ike_sa);
-                               array_insert_create_value(&ikeids, sizeof(id),
-                                                                                 ARRAY_TAIL, &id);
-                       }
-                       else
-                       {
-                               children = ike_sa->create_child_sa_enumerator(ike_sa);
-                               while (children->enumerate(children, &child_sa))
-                               {
-                                       if (streq(name, child_sa->get_name(child_sa)))
-                                       {
-                                               id = child_sa->get_unique_id(child_sa);
-                                               array_insert_create_value(&ids, sizeof(id),
-                                                                                                 ARRAY_TAIL, &id);
-                                       }
-                               }
-                               children->destroy(children);
-                       }
-               }
-               enumerator->destroy(enumerator);
-
-               if (array_count(ids))
-               {
-                       while (array_remove(ids, ARRAY_HEAD, &id))
-                       {
-                               DBG1(DBG_CFG, "closing '%s' #%u", name, id);
-                               charon->controller->terminate_child(charon->controller,
-                                                                                                       id, NULL, NULL, 0, 0);
-                       }
-                       array_destroy(ids);
-               }
-               if (array_count(ikeids))
-               {
-                       while (array_remove(ikeids, ARRAY_HEAD, &id))
-                       {
-                               DBG1(DBG_CFG, "closing IKE_SA #%u", id);
-                               charon->controller->terminate_ike(charon->controller, id,
-                                                                                                 FALSE, NULL, NULL, 0, 0);
-                       }
-                       array_destroy(ikeids);
-               }
+               to_terminate->put(to_terminate, name, name);
        }
 }
 
 /**
- * Run or undo a start actions associated with a child config
+ * Clear start actions associated with a list of child configs, optionally
+ * deletes empty IKE_SAs.
  */
-static void handle_start_action(private_vici_config_t *this,
-                                                               peer_cfg_t *peer_cfg, child_cfg_t *child_cfg,
-                                                               bool undo)
+static void clear_start_actions(private_vici_config_t *this,
+                                                               peer_cfg_t *peer_cfg, array_t *child_cfgs,
+                                                               bool delete_ike)
 {
+       enumerator_t *enumerator;
+       hashtable_t *to_terminate;
+       child_cfg_t *child_cfg;
+       char *peer_name;
+
        this->handling_actions = TRUE;
        this->lock->unlock(this->lock);
 
-       if (undo)
-       {
-               clear_start_action(this, peer_cfg->get_name(peer_cfg), child_cfg);
-       }
-       else
+       to_terminate = hashtable_create(hashtable_hash_str,
+                                                                       hashtable_equals_str, 8);
+       peer_name = peer_cfg->get_name(peer_cfg);
+
+       enumerator = array_create_enumerator(child_cfgs);
+       while (enumerator->enumerate(enumerator, &child_cfg))
        {
-               run_start_action(this, peer_cfg, child_cfg);
+               clear_start_action(this, peer_name, child_cfg, to_terminate);
        }
+       enumerator->destroy(enumerator);
+
+       terminate_for_action(this, peer_name, to_terminate, delete_ike);
+       to_terminate->destroy(to_terminate);
 
        this->lock->write_lock(this->lock);
        this->handling_actions = FALSE;
 }
 
 /**
- * Run or undo start actions associated with all child configs of a peer config
+ * Run start actions associated with a list of child configs.
  */
-static void handle_start_actions(private_vici_config_t *this,
-                                                                peer_cfg_t *peer_cfg, bool undo)
+static void run_start_actions(private_vici_config_t *this,
+                                                         peer_cfg_t *peer_cfg, array_t *child_cfgs)
 {
        enumerator_t *enumerator;
        child_cfg_t *child_cfg;
@@ -2425,17 +2456,10 @@ static void handle_start_actions(private_vici_config_t *this,
        this->handling_actions = TRUE;
        this->lock->unlock(this->lock);
 
-       enumerator = peer_cfg->create_child_cfg_enumerator(peer_cfg);
+       enumerator = array_create_enumerator(child_cfgs);
        while (enumerator->enumerate(enumerator, &child_cfg))
        {
-               if (undo)
-               {
-                       clear_start_action(this, peer_cfg->get_name(peer_cfg), child_cfg);
-               }
-               else
-               {
-                       run_start_action(this, peer_cfg, child_cfg);
-               }
+               run_start_action(this, peer_cfg, child_cfg);
        }
        enumerator->destroy(enumerator);
 
@@ -2443,6 +2467,30 @@ static void handle_start_actions(private_vici_config_t *this,
        this->handling_actions = FALSE;
 }
 
+/**
+ * Run or undo start actions for all child configs of the given peer config
+ * after it has changed.
+ */
+static void handle_start_actions(private_vici_config_t *this,
+                                                                peer_cfg_t *peer_cfg, bool undo)
+{
+       array_t *child_cfgs;
+
+       child_cfgs = array_create(0, 0);
+       array_insert_enumerator(child_cfgs, ARRAY_TAIL,
+                                                       peer_cfg->create_child_cfg_enumerator(peer_cfg));
+       if (undo)
+       {
+               /* the peer config has changed, so allow IKE_SAs to get terminated */
+               clear_start_actions(this, peer_cfg, child_cfgs, TRUE);
+       }
+       else
+       {
+               run_start_actions(this, peer_cfg, child_cfgs);
+       }
+       array_destroy(child_cfgs);
+}
+
 /**
  * Replace children of a peer config by a new config
  */
@@ -2451,13 +2499,31 @@ static void replace_children(private_vici_config_t *this,
 {
        enumerator_t *enumerator;
        child_cfg_t *child;
-       bool added;
+       array_t *to_run = NULL, *to_clear = NULL;
+       bool added, any_to_initiate = FALSE;
 
        enumerator = to->replace_child_cfgs(to, from);
        while (enumerator->enumerate(enumerator, &child, &added))
        {
-               handle_start_action(this, to, child, !added);
+               if (added)
+               {
+                       array_insert_create(&to_run, ARRAY_TAIL, child);
+
+                       if (child->get_start_action(child) & ACTION_START)
+                       {
+                               any_to_initiate = TRUE;
+                       }
+               }
+               else
+               {
+                       array_insert_create(&to_clear, ARRAY_TAIL, child);
+               }
        }
+       /* keep empty IKE_SAs only if we are to initiate any CHILD_SAs */
+       clear_start_actions(this, to, to_clear, !any_to_initiate);
+       run_start_actions(this, to, to_run);
+       array_destroy(to_clear);
+       array_destroy(to_run);
        enumerator->destroy(enumerator);
 }
 
@@ -2796,6 +2862,7 @@ CALLBACK(unload_conn, vici_message_t*,
        cfg = this->conns->remove(this->conns, conn_name);
        if (cfg)
        {
+               DBG1(DBG_CFG, "removed vici connection: %s", cfg->get_name(cfg));
                handle_start_actions(this, cfg, TRUE);
                cfg->destroy(cfg);
        }