From a229bdce625338117966a53efd0475b2c7c84566 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 9 Jul 2015 12:00:56 +0200 Subject: [PATCH] trap-manager: Changed how acquires we acted on are tracked This fixes potential race conditions in case complete() or flush() is executed before or concurrently with a thread that handles an acquire. It will also simplify tracking multiple acquires created for the same trap policy in the future. Also fixes the behavior in some error situations. --- src/libcharon/sa/trap_manager.c | 122 ++++++++++++++++++++++++++++------------ 1 file changed, 86 insertions(+), 36 deletions(-) diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c index 3a70bd1..83b6d6a 100644 --- a/src/libcharon/sa/trap_manager.c +++ b/src/libcharon/sa/trap_manager.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2011-2013 Tobias Brunner + * Copyright (C) 2011-2015 Tobias Brunner * Copyright (C) 2009 Martin Willi * Hochschule fuer Technik Rapperswil * @@ -18,10 +18,10 @@ #include #include +#include #include #include - typedef struct private_trap_manager_t private_trap_manager_t; typedef struct trap_listener_t trap_listener_t; @@ -67,6 +67,16 @@ struct private_trap_manager_t { trap_listener_t listener; /** + * list of acquires we currently handle + */ + linked_list_t *acquires; + + /** + * mutex for list of acquires + */ + mutex_t *mutex; + + /** * Whether to ignore traffic selectors from acquires */ bool ignore_acquire_ts; @@ -80,23 +90,45 @@ typedef struct { char *name; /** ref to peer_cfg to initiate */ peer_cfg_t *peer_cfg; - /** ref to instanciated CHILD_SA */ + /** ref to instantiated CHILD_SA (i.e the trap policy) */ child_sa_t *child_sa; - /** TRUE if an acquire is pending */ - bool pending; +} entry_t; + +/** + * A handled acquire + */ +typedef struct { /** pending IKE_SA connecting upon acquire */ ike_sa_t *ike_sa; -} entry_t; + /** reqid of pending trap policy */ + u_int32_t reqid; +} acquire_t; /** * actually uninstall and destroy an installed entry */ -static void destroy_entry(entry_t *entry) +static void destroy_entry(entry_t *this) +{ + this->child_sa->destroy(this->child_sa); + this->peer_cfg->destroy(this->peer_cfg); + free(this->name); + free(this); +} + +/** + * destroy a cached acquire entry + */ +static void destroy_acquire(acquire_t *this) { - entry->child_sa->destroy(entry->child_sa); - entry->peer_cfg->destroy(entry->peer_cfg); - free(entry->name); - free(entry); + free(this); +} + +/** + * match an acquire entry by reqid + */ +static bool acquire_by_reqid(acquire_t *this, u_int32_t *reqid) +{ + return this->reqid == *reqid; } METHOD(trap_manager_t, install, u_int32_t, @@ -314,6 +346,7 @@ METHOD(trap_manager_t, acquire, void, { enumerator_t *enumerator; entry_t *entry, *found = NULL; + acquire_t *acquire; peer_cfg_t *peer; child_cfg_t *child; ike_sa_t *ike_sa; @@ -337,16 +370,29 @@ METHOD(trap_manager_t, acquire, void, this->lock->unlock(this->lock); return; } - if (!cas_bool(&found->pending, FALSE, TRUE)) + reqid = found->child_sa->get_reqid(found->child_sa); + + this->mutex->lock(this->mutex); + if (this->acquires->find_first(this->acquires, (void*)acquire_by_reqid, + (void**)&acquire, &reqid) == SUCCESS) { DBG1(DBG_CFG, "ignoring acquire, connection attempt pending"); + this->mutex->unlock(this->mutex); this->lock->unlock(this->lock); return; } + else + { + INIT(acquire, + .reqid = reqid, + ); + this->acquires->insert_last(this->acquires, acquire); + } + this->mutex->unlock(this->mutex); + peer = found->peer_cfg->get_ref(found->peer_cfg); child = found->child_sa->get_config(found->child_sa); child = child->get_ref(child); - reqid = found->child_sa->get_reqid(found->child_sa); /* don't hold the lock while checking out the IKE_SA */ this->lock->unlock(this->lock); @@ -363,16 +409,13 @@ METHOD(trap_manager_t, acquire, void, * have a single TS that we can establish in a Quick Mode. */ src = dst = NULL; } + + this->mutex->lock(this->mutex); + acquire->ike_sa = ike_sa; + this->mutex->unlock(this->mutex); + if (ike_sa->initiate(ike_sa, child, reqid, src, dst) != DESTROY_ME) { - /* make sure the entry is still there */ - this->lock->read_lock(this->lock); - if (this->traps->find_first(this->traps, NULL, - (void**)&found) == SUCCESS) - { - found->ike_sa = ike_sa; - } - this->lock->unlock(this->lock); charon->ike_sa_manager->checkin(charon->ike_sa_manager, ike_sa); } else @@ -381,6 +424,14 @@ METHOD(trap_manager_t, acquire, void, ike_sa); } } + else + { + this->mutex->lock(this->mutex); + this->acquires->remove(this->acquires, acquire, NULL); + this->mutex->unlock(this->mutex); + destroy_acquire(acquire); + child->destroy(child); + } peer->destroy(peer); } @@ -391,26 +442,25 @@ static void complete(private_trap_manager_t *this, ike_sa_t *ike_sa, child_sa_t *child_sa) { enumerator_t *enumerator; - entry_t *entry; + acquire_t *acquire; - this->lock->read_lock(this->lock); - enumerator = this->traps->create_enumerator(this->traps); - while (enumerator->enumerate(enumerator, &entry)) + this->mutex->lock(this->mutex); + enumerator = this->acquires->create_enumerator(this->acquires); + while (enumerator->enumerate(enumerator, &acquire)) { - if (entry->ike_sa != ike_sa) + if (!acquire->ike_sa || acquire->ike_sa != ike_sa) { continue; } - if (child_sa && child_sa->get_reqid(child_sa) != - entry->child_sa->get_reqid(entry->child_sa)) + if (child_sa && child_sa->get_reqid(child_sa) != acquire->reqid) { continue; } - entry->ike_sa = NULL; - entry->pending = FALSE; + this->acquires->remove_at(this->acquires, enumerator); + destroy_acquire(acquire); } enumerator->destroy(enumerator); - this->lock->unlock(this->lock); + this->mutex->unlock(this->mutex); } METHOD(listener_t, ike_state_change, bool, @@ -444,14 +494,10 @@ METHOD(listener_t, child_state_change, bool, METHOD(trap_manager_t, flush, void, private_trap_manager_t *this) { - linked_list_t *traps; - /* since destroying the CHILD_SA results in events which require a read - * lock we cannot destroy the list while holding the write lock */ this->lock->write_lock(this->lock); - traps = this->traps; + this->traps->destroy_function(this->traps, (void*)destroy_entry); this->traps = linked_list_create(); this->lock->unlock(this->lock); - traps->destroy_function(traps, (void*)destroy_entry); } METHOD(trap_manager_t, destroy, void, @@ -459,6 +505,8 @@ 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->acquires->destroy_function(this->acquires, (void*)destroy_acquire); + this->mutex->destroy(this->mutex); this->lock->destroy(this->lock); free(this); } @@ -488,6 +536,8 @@ trap_manager_t *trap_manager_create(void) }, }, .traps = linked_list_create(), + .acquires = linked_list_create(), + .mutex = mutex_create(MUTEX_TYPE_DEFAULT), .lock = rwlock_create(RWLOCK_TYPE_DEFAULT), .ignore_acquire_ts = lib->settings->get_bool(lib->settings, "%s.ignore_acquire_ts", FALSE, lib->ns), -- 2.4.6