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.
2024-07-26 16:40:57 +02:00
parent da00a04f60
commit 7bfaa9acb6
1 changed files with 171 additions and 104 deletions
@@ -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)
+	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))
 	{
-		clear_start_action(this, peer_cfg->get_name(peer_cfg), child_cfg);
-	}
-	else
-	{
-		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);
 	}