Merge branch 'trap-shunt-updates'

Changes how acquires are tracked in the trap manager, which fixes
several race conditions.  Also fixes races between threads installing
trap policies and the main thread trying to flush the trap policies.
Similar changes were added to the shunt manager which previously
used no locking at all.

Fixes #1014.

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);

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
  *
@@ -18,8 +19,10 @@
 #include <hydra.h>
 #include <daemon.h>
 #include <threading/rwlock.h>
+#include <threading/rwlock_condvar.h>
 #include <collections/linked_list.h>
 
+#define INSTALL_DISABLED ((u_int)~0)
 
 typedef struct private_shunt_manager_t private_shunt_manager_t;
 
@@ -37,6 +40,21 @@ 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;
+
+	/**
+	 * Number of threads currently installing shunts, or INSTALL_DISABLED
+	 */
+	u_int installing;
+
+	/**
+	 * Condvar to signal shunt installation
+	 */
+	rwlock_condvar_t *condvar;
 };
 
 /**
@@ -117,9 +135,15 @@ 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);
+	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))
 	{
@@ -130,16 +154,29 @@ 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->installing++;
+	this->lock->unlock(this->lock);
 
-	return install_shunt_policy(child);
+	success = install_shunt_policy(child);
+
+	this->lock->write_lock(this->lock);
+	if (!success)
+	{
+		this->shunts->remove(this->shunts, child, NULL);
+		child->destroy(child);
+	}
+	this->installing--;
+	this->condvar->signal(this->condvar);
+	this->lock->unlock(this->lock);
+	return success;
 }
 
 /**
@@ -215,6 +252,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 +264,7 @@ METHOD(shunt_manager_t, uninstall, bool,
 		}
 	}
 	enumerator->destroy(enumerator);
+	this->lock->unlock(this->lock);
 
 	if (!found)
 	{
@@ -239,20 +278,37 @@ 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,
+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);
 }
 
@@ -268,9 +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;

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.
 	 */

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,9 +18,12 @@
 
 #include <hydra.h>
 #include <daemon.h>
+#include <threading/mutex.h>
 #include <threading/rwlock.h>
+#include <threading/rwlock_condvar.h>
 #include <collections/linked_list.h>
 
+#define INSTALL_DISABLED ((u_int)~0)
 
 typedef struct private_trap_manager_t private_trap_manager_t;
 typedef struct trap_listener_t trap_listener_t;
@@ -66,6 +69,26 @@ 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;
+
+	/**
+	 * 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
 	 */
@@ -80,23 +103,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,
@@ -139,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))
 	{
@@ -172,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);
 
@@ -220,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;
 }
 
@@ -314,6 +370,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 +394,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,24 +433,29 @@ 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
 		{
-			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);
 		}
 	}
+	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 +466,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 +518,15 @@ 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;
+	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);
-	traps->destroy_function(traps, (void*)destroy_entry);
 }
 
 METHOD(trap_manager_t, destroy, void,
@@ -459,6 +534,9 @@ 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);
 }
@@ -488,7 +566,10 @@ 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),
+		.condvar = rwlock_condvar_create(),
 		.ignore_acquire_ts = lib->settings->get_bool(lib->settings,
 										"%s.ignore_acquire_ts", FALSE, lib->ns),
 	);