From: Tobias Brunner Date: Thu, 4 Jul 2024 14:17:43 +0000 (+0200) Subject: vici: Improve handling of start action when reloading configs X-Git-Tag: android-2.5.2~4^2~1 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7bfaa9acb6f533d68da389d38d2aceb3c6f29daa;p=thirdparty%2Fstrongswan.git 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 c858e9945c..c46b8872b9 100644 --- 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); }