trap-manager: Changed how acquires we acted on are tracked
authorTobias Brunner <tobias@strongswan.org>
Thu, 9 Jul 2015 10:00:56 +0000 (12:00 +0200)
committerTobias Brunner <tobias@strongswan.org>
Mon, 27 Jul 2015 11:50:09 +0000 (13:50 +0200)
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

index 3a70bd1..83b6d6a 100644 (file)
@@ -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
  *
 
 #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),