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

diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c
index 83b6d6a9a6285758a0cd812b3b59600ee9f55008..424d9e7c7443cec837bb89324df3e315a9c26e99 100644 (file)
--- a/src/libcharon/sa/trap_manager.c
+++ b/src/libcharon/sa/trap_manager.c
@@ -20,8 +20,11 @@
 #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;
 
@@ -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),
        );