From: Tobias Brunner Date: Tue, 17 Mar 2020 08:39:17 +0000 (+0100) Subject: ike-sa-manager: Make checkout_by_config() atomic X-Git-Tag: 5.9.2rc1~22 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c46c40ef24ce0f2c60393e19ea62094a0c6957bc;p=thirdparty%2Fstrongswan.git ike-sa-manager: Make checkout_by_config() atomic These changes should ensure that concurrent calls to checkout_by_config() result in a single IKE_SA. For instance, when acquires for different children of the same connection are triggered concurrently. There are two major changes to the interface: 1) The peer config object is now always set on the returned IKE_SA. That was previously only the case if an existing IKE_SA was returned. 2) The IKE_SA is now always registered with the manager and properly checked out, which also was only the case for existing IKE_SAs before. --- diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c index f95ff19af7..2a72794cff 100644 --- a/src/libcharon/sa/ike_sa_manager.c +++ b/src/libcharon/sa/ike_sa_manager.c @@ -2,7 +2,7 @@ * Copyright (C) 2005-2011 Martin Willi * Copyright (C) 2011 revosec AG * - * Copyright (C) 2008-2018 Tobias Brunner + * Copyright (C) 2008-2021 Tobias Brunner * Copyright (C) 2005 Jan Hutter * HSR Hochschule fuer Technik Rapperswil * @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -391,10 +392,25 @@ struct private_ike_sa_manager_t { table_item_t **init_hashes_table; /** - * Segments of the "hashes" hash table. + * Segments of the "hashes" hash table. */ segment_t *init_hashes_segments; + /** + * Configs for which an SA is currently being checked out. + */ + array_t *config_checkouts; + + /** + * Mutex to protect access to configs. + */ + mutex_t *config_mutex; + + /** + * Condvar to indicate changes in checkout configs. + */ + condvar_t *config_condvar; + /** * RNG to get random SPIs for our side */ @@ -871,6 +887,29 @@ static void remove_half_open(private_ike_sa_manager_t *this, entry_t *entry) lock->unlock(lock); } +/** + * Create an entry and put it into the hash table. + * Note: The caller has to unlock the segment. + */ +static u_int create_and_put_entry(private_ike_sa_manager_t *this, + ike_sa_t *ike_sa, entry_t **entry) +{ + ike_sa_id_t *ike_sa_id = ike_sa->get_id(ike_sa); + host_t *other = ike_sa->get_other_host(ike_sa); + + *entry = entry_create(); + (*entry)->ike_sa_id = ike_sa_id->clone(ike_sa_id); + (*entry)->ike_sa = ike_sa; + + if (ike_sa->get_state(ike_sa) == IKE_CONNECTING) + { + (*entry)->half_open = TRUE; + (*entry)->other = other->clone(other); + put_half_open(this, *entry); + } + return put_entry(this, *entry); +} + CALLBACK(id_matches, bool, ike_sa_id_t *a, va_list args) { @@ -1422,6 +1461,18 @@ out: return ike_sa; } +/** + * Data used to track checkouts by config. + */ +typedef struct { + /** The peer config for which an IKE_SA is being checked out. */ + peer_cfg_t *cfg; + /** Number of threads checking out SAs for the same config. */ + int threads; + /** A thread is currently creating/finding an SA for this config. */ + bool working; +} config_entry_t; + METHOD(ike_sa_manager_t, checkout_by_config, ike_sa_t*, private_ike_sa_manager_t *this, peer_cfg_t *peer_cfg) { @@ -1430,17 +1481,54 @@ METHOD(ike_sa_manager_t, checkout_by_config, ike_sa_t*, ike_sa_t *ike_sa = NULL; peer_cfg_t *current_peer; ike_cfg_t *current_ike; + config_entry_t *config_entry, *found = NULL; u_int segment; + int i; DBG2(DBG_MGR, "checkout IKE_SA by config"); if (!this->reuse_ikesa && peer_cfg->get_ike_version(peer_cfg) != IKEV1) { /* IKE_SA reuse disabled by config (not possible for IKEv1) */ ike_sa = checkout_new(this, peer_cfg->get_ike_version(peer_cfg), TRUE); + ike_sa->set_peer_cfg(ike_sa, peer_cfg); + + segment = create_and_put_entry(this, ike_sa, &entry); + entry->checked_out = thread_current(); + unlock_single_segment(this, segment); charon->bus->set_sa(charon->bus, ike_sa); goto out; } + this->config_mutex->lock(this->config_mutex); + for (i = 0; i < array_count(this->config_checkouts); i++) + { + array_get(this->config_checkouts, i, &config_entry); + if (config_entry->cfg->equals(config_entry->cfg, peer_cfg)) + { + current_ike = config_entry->cfg->get_ike_cfg(config_entry->cfg); + if (current_ike->equals(current_ike, + peer_cfg->get_ike_cfg(peer_cfg))) + { + found = config_entry; + break; + } + } + } + if (!found) + { + INIT(found, + .cfg = peer_cfg->get_ref(peer_cfg), + ); + array_insert_create(&this->config_checkouts, ARRAY_TAIL, found); + } + found->threads++; + while (found->working) + { + this->config_condvar->wait(this->config_condvar, this->config_mutex); + } + found->working = TRUE; + this->config_mutex->unlock(this->config_mutex); + enumerator = create_table_enumerator(this); while (enumerator->enumerate(enumerator, &entry, &segment)) { @@ -1463,7 +1551,7 @@ METHOD(ike_sa_manager_t, checkout_by_config, ike_sa_t*, { entry->checked_out = thread_current(); ike_sa = entry->ike_sa; - DBG2(DBG_MGR, "found existing IKE_SA %u with a '%s' config", + DBG2(DBG_MGR, "found existing IKE_SA %u with config '%s'", ike_sa->get_unique_id(ike_sa), current_peer->get_name(current_peer)); break; @@ -1475,11 +1563,36 @@ METHOD(ike_sa_manager_t, checkout_by_config, ike_sa_t*, enumerator->destroy(enumerator); if (!ike_sa) - { /* no IKE_SA using such a config, hand out a new */ + { ike_sa = checkout_new(this, peer_cfg->get_ike_version(peer_cfg), TRUE); + ike_sa->set_peer_cfg(ike_sa, peer_cfg); + + segment = create_and_put_entry(this, ike_sa, &entry); + entry->checked_out = thread_current(); + unlock_single_segment(this, segment); } charon->bus->set_sa(charon->bus, ike_sa); + this->config_mutex->lock(this->config_mutex); + found->working = FALSE; + found->threads--; + if (!found->threads) + { + for (i = 0; i < array_count(this->config_checkouts); i++) + { + array_get(this->config_checkouts, i, &config_entry); + if (config_entry == found) + { + array_remove(this->config_checkouts, i, NULL); + found->cfg->destroy(found->cfg); + free(found); + break; + } + } + } + this->config_condvar->signal(this->config_condvar); + this->config_mutex->unlock(this->config_mutex); + out: if (!ike_sa) { @@ -1784,16 +1897,7 @@ METHOD(ike_sa_manager_t, checkin, void, } else { - entry = entry_create(); - entry->ike_sa_id = ike_sa_id->clone(ike_sa_id); - entry->ike_sa = ike_sa; - if (ike_sa->get_state(ike_sa) == IKE_CONNECTING) - { - entry->half_open = TRUE; - entry->other = other->clone(other); - put_half_open(this, entry); - } - segment = put_entry(this, entry); + segment = create_and_put_entry(this, ike_sa, &entry); } DBG2(DBG_MGR, "checkin of IKE_SA successful"); @@ -2326,6 +2430,10 @@ METHOD(ike_sa_manager_t, destroy, void, free(this->connected_peers_segments); free(this->init_hashes_segments); + array_destroy(this->config_checkouts); + this->config_mutex->destroy(this->config_mutex); + this->config_condvar->destroy(this->config_condvar); + this->spi_lock->destroy(this->spi_lock); free(this); } @@ -2449,6 +2557,9 @@ ike_sa_manager_t *ike_sa_manager_create() this->init_hashes_segments[i].mutex = mutex_create(MUTEX_TYPE_RECURSIVE); } + this->config_mutex = mutex_create(MUTEX_TYPE_DEFAULT); + this->config_condvar = condvar_create(CONDVAR_TYPE_DEFAULT); + this->reuse_ikesa = lib->settings->get_bool(lib->settings, "%s.reuse_ikesa", TRUE, lib->ns); return &this->public; diff --git a/src/libcharon/sa/ike_sa_manager.h b/src/libcharon/sa/ike_sa_manager.h index efad2e4d61..58cde4d3be 100644 --- a/src/libcharon/sa/ike_sa_manager.h +++ b/src/libcharon/sa/ike_sa_manager.h @@ -99,14 +99,16 @@ struct ike_sa_manager_t { * This call checks for an existing IKE_SA by comparing the configuration. * If the CHILD_SA can be created in an existing IKE_SA, the matching SA * is returned. - * If no IKE_SA is found, a new one is created. This is also the case when - * the found IKE_SA is in the DELETING state. + * If no IKE_SA is found, a new one is created and registered in the + * manager. This is also the case when the found IKE_SA is in an unusable + * state (e.g. DELETING). + * + * @note The peer_config is always set on the returned IKE_SA. * * @param peer_cfg configuration used to find an existing IKE_SA * @return checked out/created IKE_SA */ - ike_sa_t* (*checkout_by_config) (ike_sa_manager_t* this, - peer_cfg_t *peer_cfg); + ike_sa_t *(*checkout_by_config)(ike_sa_manager_t* this, peer_cfg_t *peer_cfg); /** * Reset initiator SPI.