path: root/main/strongswan/0007-trap-manager-Changed-how-acquires-we-acted-on-are-tr.patch

From a229bdce625338117966a53efd0475b2c7c84566 Mon Sep 17 00:00:00 2001
From: Tobias Brunner <tobias@strongswan.org>
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 <hydra.h>
 #include <daemon.h>
+#include <threading/mutex.h>
 #include <threading/rwlock.h>
 #include <collections/linked_list.h>
 
-
 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