From 773fcb1605d413997450b59d114a1c035910cc58 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 9 Jul 2015 14:34:19 +0200 Subject: [PATCH 1/7] trap-manager: Properly check-in IKE_SA if initiating fails This basically reverts f4e822c1b422 ("trap-manager: don't check-in nonexisting IKE_SA if acquire fails"). As checkout_by_config() could return an already existing and established IKE_SA we have to properly destroy it, for instance, in case other threads are waiting to check it out. checkin_and_destroy() should handle the case of a new SA properly (it produces a log message on level 1, though). --- src/libcharon/sa/trap_manager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c index d6ff3c8c5..3a70bd135 100644 --- a/src/libcharon/sa/trap_manager.c +++ b/src/libcharon/sa/trap_manager.c @@ -377,8 +377,8 @@ METHOD(trap_manager_t, acquire, void, } else { - ike_sa->destroy(ike_sa); - charon->bus->set_sa(charon->bus, NULL); + charon->ike_sa_manager->checkin_and_destroy(charon->ike_sa_manager, + ike_sa); } } peer->destroy(peer); From a229bdce625338117966a53efd0475b2c7c84566 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 9 Jul 2015 12:00:56 +0200 Subject: [PATCH 2/7] 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 3a70bd135..83b6d6a9a 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; @@ -66,6 +66,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 */ @@ -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) { - entry->child_sa->destroy(entry->child_sa); - entry->peer_cfg->destroy(entry->peer_cfg); - free(entry->name); - free(entry); + 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) +{ + 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), From 12b3cdba7689113558f58a5265827f3086852bae Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 13 Jul 2015 13:20:14 +0200 Subject: [PATCH 3/7] trap-manager: Resolve race conditions between flush() and install() When flush() is called there might be threads in install() waiting for trap policies to get installed (without holding the lock). We have to wait until they updated the entries with the respective CHILD_SAs before destroying the list. We also have to prevent further trap policy installations (and wait until threads in install() are really finished), otherwise we might end up destroying CHILD_SA objects after the kernel interface implementations have already been unloaded (avoiding this is the whole point of calling flush() before unloading the plugins). --- src/libcharon/sa/trap_manager.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c index 83b6d6a9a..424d9e7c7 100644 --- a/src/libcharon/sa/trap_manager.c +++ b/src/libcharon/sa/trap_manager.c @@ -20,8 +20,11 @@ #include #include #include +#include #include +#define INSTALL_DISABLED ((u_int)~0) + typedef struct private_trap_manager_t private_trap_manager_t; typedef struct trap_listener_t trap_listener_t; @@ -76,6 +79,16 @@ struct private_trap_manager_t { */ mutex_t *mutex; + /** + * number of threads currently installing trap policies, or INSTALL_DISABLED + */ + u_int installing; + + /** + * condvar to signal trap policy installation + */ + rwlock_condvar_t *condvar; + /** * Whether to ignore traffic selectors from acquires */ @@ -171,6 +184,11 @@ METHOD(trap_manager_t, install, u_int32_t, } this->lock->write_lock(this->lock); + if (this->installing == INSTALL_DISABLED) + { /* flush() has been called */ + this->lock->unlock(this->lock); + return 0; + } enumerator = this->traps->create_enumerator(this->traps); while (enumerator->enumerate(enumerator, &entry)) { @@ -204,6 +222,7 @@ METHOD(trap_manager_t, install, u_int32_t, .peer_cfg = peer->get_ref(peer), ); this->traps->insert_first(this->traps, entry); + this->installing++; /* don't hold lock while creating CHILD_SA and installing policies */ this->lock->unlock(this->lock); @@ -252,6 +271,11 @@ METHOD(trap_manager_t, install, u_int32_t, { destroy_entry(found); } + this->lock->write_lock(this->lock); + /* do this at the end, so entries created temporarily are also destroyed */ + this->installing--; + this->condvar->signal(this->condvar); + this->lock->unlock(this->lock); return reqid; } @@ -495,8 +519,13 @@ METHOD(trap_manager_t, flush, void, private_trap_manager_t *this) { this->lock->write_lock(this->lock); + while (this->installing) + { + this->condvar->wait(this->condvar, this->lock); + } this->traps->destroy_function(this->traps, (void*)destroy_entry); this->traps = linked_list_create(); + this->installing = INSTALL_DISABLED; this->lock->unlock(this->lock); } @@ -506,6 +535,7 @@ 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->condvar->destroy(this->condvar); this->mutex->destroy(this->mutex); this->lock->destroy(this->lock); free(this); @@ -539,6 +569,7 @@ trap_manager_t *trap_manager_create(void) .acquires = linked_list_create(), .mutex = mutex_create(MUTEX_TYPE_DEFAULT), .lock = rwlock_create(RWLOCK_TYPE_DEFAULT), + .condvar = rwlock_condvar_create(), .ignore_acquire_ts = lib->settings->get_bool(lib->settings, "%s.ignore_acquire_ts", FALSE, lib->ns), ); From f3d39666e0d62fb9a790b72ee7ae2b9255b21cdd Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 14 Jul 2015 16:35:21 +0200 Subject: [PATCH 4/7] shunt-manager: Add a lock to safely access the list of shunt policies --- src/libcharon/sa/shunt_manager.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/libcharon/sa/shunt_manager.c b/src/libcharon/sa/shunt_manager.c index 73e1abbf3..434bacee7 100644 --- a/src/libcharon/sa/shunt_manager.c +++ b/src/libcharon/sa/shunt_manager.c @@ -1,4 +1,5 @@ /* + * Copyright (C) 2015 Tobias Brunner * Copyright (C) 2011 Andreas Steffen * HSR Hochschule fuer Technik Rapperswil * @@ -20,7 +21,6 @@ #include #include - typedef struct private_shunt_manager_t private_shunt_manager_t; /** @@ -37,6 +37,11 @@ struct private_shunt_manager_t { * Installed shunts, as child_cfg_t */ linked_list_t *shunts; + + /** + * Lock to safely access the list of shunts + */ + rwlock_t *lock; }; /** @@ -120,6 +125,7 @@ METHOD(shunt_manager_t, install, bool, bool found = FALSE; /* check if not already installed */ + this->lock->write_lock(this->lock); enumerator = this->shunts->create_enumerator(this->shunts); while (enumerator->enumerate(enumerator, &child_cfg)) { @@ -130,14 +136,15 @@ METHOD(shunt_manager_t, install, bool, } } enumerator->destroy(enumerator); - if (found) { DBG1(DBG_CFG, "shunt %N policy '%s' already installed", ipsec_mode_names, child->get_mode(child), child->get_name(child)); + this->lock->unlock(this->lock); return TRUE; } this->shunts->insert_last(this->shunts, child->get_ref(child)); + this->lock->unlock(this->lock); return install_shunt_policy(child); } @@ -215,6 +222,7 @@ METHOD(shunt_manager_t, uninstall, bool, enumerator_t *enumerator; child_cfg_t *child, *found = NULL; + this->lock->write_lock(this->lock); enumerator = this->shunts->create_enumerator(this->shunts); while (enumerator->enumerate(enumerator, &child)) { @@ -226,6 +234,7 @@ METHOD(shunt_manager_t, uninstall, bool, } } enumerator->destroy(enumerator); + this->lock->unlock(this->lock); if (!found) { @@ -239,7 +248,10 @@ METHOD(shunt_manager_t, uninstall, bool, METHOD(shunt_manager_t, create_enumerator, enumerator_t*, private_shunt_manager_t *this) { - return this->shunts->create_enumerator(this->shunts); + this->lock->read_lock(this->lock); + return enumerator_create_cleaner( + this->shunts->create_enumerator(this->shunts), + (void*)this->lock->unlock, this->lock); } METHOD(shunt_manager_t, destroy, void, @@ -253,6 +265,7 @@ METHOD(shunt_manager_t, destroy, void, child->destroy(child); } this->shunts->destroy(this->shunts); + this->lock->destroy(this->lock); free(this); } @@ -271,6 +284,7 @@ shunt_manager_t *shunt_manager_create() .destroy = _destroy, }, .shunts = linked_list_create(), + .lock = rwlock_create(RWLOCK_TYPE_DEFAULT), ); return &this->public; From 616ff9a2369fd250a2b9e8d2a00f37e2e8d3a2f3 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 14 Jul 2015 16:50:32 +0200 Subject: [PATCH 5/7] shunt-manager: Remove stored entries if installation fails --- src/libcharon/sa/shunt_manager.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/libcharon/sa/shunt_manager.c b/src/libcharon/sa/shunt_manager.c index 434bacee7..2e42e7ebd 100644 --- a/src/libcharon/sa/shunt_manager.c +++ b/src/libcharon/sa/shunt_manager.c @@ -122,7 +122,7 @@ METHOD(shunt_manager_t, install, bool, { enumerator_t *enumerator; child_cfg_t *child_cfg; - bool found = FALSE; + bool found = FALSE, success; /* check if not already installed */ this->lock->write_lock(this->lock); @@ -146,7 +146,16 @@ METHOD(shunt_manager_t, install, bool, this->shunts->insert_last(this->shunts, child->get_ref(child)); this->lock->unlock(this->lock); - return install_shunt_policy(child); + success = install_shunt_policy(child); + + if (!success) + { + this->lock->write_lock(this->lock); + this->shunts->remove(this->shunts, child, NULL); + this->lock->unlock(this->lock); + child->destroy(child); + } + return success; } /** From bc36530670cbbe2362053f1604f67e481afd336c Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 14 Jul 2015 16:55:36 +0200 Subject: [PATCH 6/7] shunt-manager: Add flush() method to properly uninstall shunts This will allow us to uninstall shunts before unloading the kernel-interface plugins. --- src/libcharon/sa/shunt_manager.c | 44 +++++++++++++++++++++++++++++--- src/libcharon/sa/shunt_manager.h | 6 +++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/libcharon/sa/shunt_manager.c b/src/libcharon/sa/shunt_manager.c index 2e42e7ebd..1a984435c 100644 --- a/src/libcharon/sa/shunt_manager.c +++ b/src/libcharon/sa/shunt_manager.c @@ -19,8 +19,11 @@ #include #include #include +#include #include +#define INSTALL_DISABLED ((u_int)~0) + typedef struct private_shunt_manager_t private_shunt_manager_t; /** @@ -42,6 +45,16 @@ struct private_shunt_manager_t { * Lock to safely access the list of shunts */ rwlock_t *lock; + + /** + * Number of threads currently installing shunts, or INSTALL_DISABLED + */ + u_int installing; + + /** + * Condvar to signal shunt installation + */ + rwlock_condvar_t *condvar; }; /** @@ -126,6 +139,11 @@ METHOD(shunt_manager_t, install, bool, /* check if not already installed */ this->lock->write_lock(this->lock); + if (this->installing == INSTALL_DISABLED) + { /* flush() has been called */ + this->lock->unlock(this->lock); + return FALSE; + } enumerator = this->shunts->create_enumerator(this->shunts); while (enumerator->enumerate(enumerator, &child_cfg)) { @@ -144,17 +162,20 @@ METHOD(shunt_manager_t, install, bool, return TRUE; } this->shunts->insert_last(this->shunts, child->get_ref(child)); + this->installing++; this->lock->unlock(this->lock); success = install_shunt_policy(child); + this->lock->write_lock(this->lock); if (!success) { - this->lock->write_lock(this->lock); this->shunts->remove(this->shunts, child, NULL); - this->lock->unlock(this->lock); child->destroy(child); } + this->installing--; + this->condvar->signal(this->condvar); + this->lock->unlock(this->lock); return success; } @@ -263,18 +284,31 @@ METHOD(shunt_manager_t, create_enumerator, enumerator_t*, (void*)this->lock->unlock, this->lock); } -METHOD(shunt_manager_t, destroy, void, +METHOD(shunt_manager_t, flush, void, private_shunt_manager_t *this) { child_cfg_t *child; + this->lock->write_lock(this->lock); + while (this->installing) + { + this->condvar->wait(this->condvar, this->lock); + } while (this->shunts->remove_last(this->shunts, (void**)&child) == SUCCESS) { uninstall_shunt_policy(child); child->destroy(child); } - this->shunts->destroy(this->shunts); + this->installing = INSTALL_DISABLED; + this->lock->unlock(this->lock); +} + +METHOD(shunt_manager_t, destroy, void, + private_shunt_manager_t *this) +{ + this->shunts->destroy_offset(this->shunts, offsetof(child_cfg_t, destroy)); this->lock->destroy(this->lock); + this->condvar->destroy(this->condvar); free(this); } @@ -290,10 +324,12 @@ shunt_manager_t *shunt_manager_create() .install = _install, .uninstall = _uninstall, .create_enumerator = _create_enumerator, + .flush = _flush, .destroy = _destroy, }, .shunts = linked_list_create(), .lock = rwlock_create(RWLOCK_TYPE_DEFAULT), + .condvar = rwlock_condvar_create(), ); return &this->public; diff --git a/src/libcharon/sa/shunt_manager.h b/src/libcharon/sa/shunt_manager.h index 28a795dc9..c43f5db3d 100644 --- a/src/libcharon/sa/shunt_manager.h +++ b/src/libcharon/sa/shunt_manager.h @@ -1,4 +1,5 @@ /* + * Copyright (C) 2015 Tobias Brunner * Copyright (C) 2011 Andreas Steffen * HSR Hochschule fuer Technik Rapperswil * @@ -55,6 +56,11 @@ struct shunt_manager_t { */ enumerator_t* (*create_enumerator)(shunt_manager_t *this); + /** + * Clear any installed shunt. + */ + void (*flush)(shunt_manager_t *this); + /** * Destroy a shunt_manager_t. */ From c04345d5edbbc4c37027cdfc21dba85d03e312af Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 14 Jul 2015 16:56:33 +0200 Subject: [PATCH 7/7] daemon: Flush shunts before unloading plugins --- src/libcharon/daemon.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libcharon/daemon.c b/src/libcharon/daemon.c index b1b8f57f0..316be7611 100644 --- a/src/libcharon/daemon.c +++ b/src/libcharon/daemon.c @@ -462,6 +462,10 @@ static void destroy(private_daemon_t *this) { this->public.traps->flush(this->public.traps); } + if (this->public.shunts) + { + this->public.shunts->flush(this->public.shunts); + } if (this->public.sender) { this->public.sender->flush(this->public.sender);