using shared read locks in credential set enumerators to avoid deadlocks
authorMartin Willi <martin@strongswan.org>
Wed, 30 Jul 2008 11:38:44 +0000 (11:38 -0000)
committerMartin Willi <martin@strongswan.org>
Wed, 30 Jul 2008 11:38:44 +0000 (11:38 -0000)
src/charon/credentials/credential_manager.c
src/charon/credentials/credential_set.h
src/charon/credentials/sets/cert_cache.c
src/charon/plugins/stroke/stroke_ca.c
src/charon/plugins/stroke/stroke_cred.c

index 251194b..c850a15 100644 (file)
@@ -61,6 +61,11 @@ struct private_credential_manager_t {
        cert_cache_t *cache;
        
        /**
+        * certificates queued for persistent caching
+        */
+       linked_list_t *cache_queue;
+       
+       /**
         * read-write lock to sets list
         */
        pthread_rwlock_t lock;
@@ -407,17 +412,51 @@ static void cache_cert(private_credential_manager_t *this, certificate_t *cert)
        credential_set_t *set;
        enumerator_t *enumerator;
        
-       pthread_rwlock_rdlock(&this->lock);
-       enumerator = this->sets->create_enumerator(this->sets);
-       while (enumerator->enumerate(enumerator, &set))
+       if (pthread_rwlock_trywrlock(&this->lock) == 0)
        {
-               set->cache_cert(set, cert);
+               enumerator = this->sets->create_enumerator(this->sets);
+               while (enumerator->enumerate(enumerator, &set))
+               {
+                       set->cache_cert(set, cert);
+               }
+               enumerator->destroy(enumerator);
+       }
+       else
+       {       /* we can't cache now as other threads are active, queue for later */
+               pthread_rwlock_rdlock(&this->lock);
+               this->cache_queue->insert_last(this->cache_queue, cert->get_ref(cert));
        }
-       enumerator->destroy(enumerator);
        pthread_rwlock_unlock(&this->lock);
 }
 
 /**
+ * Try to cache certificates queued for caching
+ */
+static void cache_queue(private_credential_manager_t *this)
+{
+       credential_set_t *set;
+       certificate_t *cert;
+       enumerator_t *enumerator;
+       
+       if (this->cache_queue->get_count(this->cache_queue) > 0 &&
+               pthread_rwlock_trywrlock(&this->lock) == 0)
+       {
+               while (this->cache_queue->remove_last(this->cache_queue,
+                                                                                         (void**)&cert) == SUCCESS)
+               {
+                       enumerator = this->sets->create_enumerator(this->sets);
+                       while (enumerator->enumerate(enumerator, &set))
+                       {
+                               set->cache_cert(set, cert);
+                       }
+                       enumerator->destroy(enumerator);
+                       cert->destroy(cert);
+               }
+               pthread_rwlock_unlock(&this->lock);
+       }
+}
+
+/**
  * forward declaration 
  */
 static enumerator_t *create_trusted_enumerator(private_credential_manager_t *this,
@@ -1262,6 +1301,9 @@ static void public_destroy(public_enumerator_t *this)
                this->wrapper->destroy(this->wrapper);
        }
        pthread_rwlock_unlock(&this->this->lock);
+       
+       /* check for delayed certificate cache queue */
+       cache_queue(this->this);
        free(this);
 }
 
@@ -1501,6 +1543,8 @@ static void remove_set(private_credential_manager_t *this, credential_set_t *set
  */
 static void destroy(private_credential_manager_t *this)
 {
+       cache_queue(this);
+       this->cache_queue->destroy(this->cache_queue);
        this->sets->remove(this->sets, this->cache, NULL);
        this->sets->destroy(this->sets);
        pthread_key_delete(this->local_sets);
@@ -1532,6 +1576,7 @@ credential_manager_t *credential_manager_create()
        this->sets = linked_list_create();
        pthread_key_create(&this->local_sets, (void*)this->sets->destroy);
        this->cache = cert_cache_create();
+       this->cache_queue = linked_list_create();
        this->sets->insert_first(this->sets, this->cache);
        pthread_rwlock_init(&this->lock, NULL);
        
index 41c5b16..6a973e9 100644 (file)
@@ -36,6 +36,13 @@ typedef struct credential_set_t credential_set_t;
  * Enumerators are used because queries might return multiple matches.
  * Filter parameters restrict enumeration over specific items only.
  * See credential_manager_t for an overview of the credential framework.
+ *
+ * A credential set enumerator may not block the credential set, i.e. multiple
+ * threads must be able to hold multiple enumerators, as the credential manager
+ * is higly parallelized. The best way to achieve this is by using shared
+ * read locks for the enumerators only. Otherwiese deadlocks will occur.
+ * The writing cache_cert() routine is called by the manager only if no
+ * enumerator is alive, so it is save to use a write lock there.
  */
 struct credential_set_t {
        
index 8af8bb6..4a9a971 100644 (file)
  * $Id$
  */
 
+#define _GNU_SOURCE
+#include <pthread.h>
+
 #include "cert_cache.h"
 
 #include <daemon.h>
 #include <utils/linked_list.h>
-#include <utils/mutex.h>
 
 #define CACHE_SIZE 30
 
@@ -44,7 +46,7 @@ struct private_cert_cache_t {
        /**
         * do we have an active enumerator
         */
-       bool enumerating;
+       refcount_t enumerating;
        
        /**
         * have we increased the cache without a check_cache?
@@ -52,9 +54,9 @@ struct private_cert_cache_t {
        bool check_required;
        
        /**
-        * mutex to lock relations list
+        * read-write lock to sets list
         */
-       mutex_t *mutex;
+       pthread_rwlock_t lock;
 };
 
 /**
@@ -88,8 +90,8 @@ static void check_cache(private_cert_cache_t *this)
        {
                this->check_required = TRUE;
        }
-       else
-       {
+       else if (pthread_rwlock_trywrlock(&this->lock) == 0)
+       {       /* never blocks, only done if lock is available */
                while (this->relations->get_count(this->relations) > CACHE_SIZE)
                {
                        relation_t *oldest = NULL, *current;
@@ -108,6 +110,7 @@ static void check_cache(private_cert_cache_t *this)
                        relation_destroy(oldest);
                }
                this->check_required = FALSE;
+               pthread_rwlock_unlock(&this->lock);
        }
 }
 
@@ -121,7 +124,7 @@ static bool issued_by(private_cert_cache_t *this,
        enumerator_t *enumerator;
        
        /* lookup cache */
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_rdlock(&this->lock);
        enumerator = this->relations->create_enumerator(this->relations);
        while (enumerator->enumerate(enumerator, &current))
        {
@@ -146,7 +149,7 @@ static bool issued_by(private_cert_cache_t *this,
                }
        }
        enumerator->destroy(enumerator);
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
        if (found)
        {
                return TRUE;
@@ -161,10 +164,9 @@ static bool issued_by(private_cert_cache_t *this,
        found->subject = subject->get_ref(subject);
        found->issuer = issuer->get_ref(issuer);
        found->last_use = time(NULL);
-       this->mutex->lock(this->mutex);
+       /* insert should be ok without lock */
        this->relations->insert_last(this->relations, found);
        check_cache(this);
-       this->mutex->unlock(this->mutex);
        return TRUE;
 }
 
@@ -230,12 +232,12 @@ static bool certs_filter(cert_data_t *data, relation_t **in, certificate_t **out
  */
 static void certs_destroy(cert_data_t *data)
 {
-       data->this->enumerating--;
+       ref_put(&data->this->enumerating);
+       pthread_rwlock_unlock(&data->this->lock);
        if (data->this->check_required)
        {
                check_cache(data->this);
        }
-       data->this->mutex->unlock(data->this->mutex);
        free(data);
 }
 
@@ -258,22 +260,14 @@ static enumerator_t *create_enumerator(private_cert_cache_t *this,
        data->id = id;
        data->this = this;
        
-       this->mutex->lock(this->mutex);
-       this->enumerating++;
+       pthread_rwlock_rdlock(&this->lock);
+       ref_get(&this->enumerating);
        return enumerator_create_filter(
                                                        this->relations->create_enumerator(this->relations),
                                                        (void*)certs_filter, data, (void*)certs_destroy);
 }
 
 /**
- * Implementation of credential_set_t.cache_cert.
- */
-static void cache_cert(private_cert_cache_t *this, certificate_t *cert)
-{
-       /* TODO: implement caching */
-}
-
-/**
  * Implementation of cert_cache_t.flush.
  */
 static void flush(private_cert_cache_t *this, certificate_type_t type)
@@ -281,7 +275,7 @@ static void flush(private_cert_cache_t *this, certificate_type_t type)
        enumerator_t *enumerator;
        relation_t *relation;
        
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_wrlock(&this->lock);
        enumerator = this->relations->create_enumerator(this->relations);
        while (enumerator->enumerate(enumerator, &relation))
        {
@@ -293,7 +287,7 @@ static void flush(private_cert_cache_t *this, certificate_type_t type)
                }
        }
        enumerator->destroy(enumerator);
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
 }
 
 /**
@@ -302,7 +296,7 @@ static void flush(private_cert_cache_t *this, certificate_type_t type)
 static void destroy(private_cert_cache_t *this)
 {
        this->relations->destroy_function(this->relations, (void*)relation_destroy);
-       this->mutex->destroy(this->mutex);
+       pthread_rwlock_destroy(&this->lock);
        free(this);
 }
 
@@ -317,15 +311,15 @@ cert_cache_t *cert_cache_create()
        this->public.set.create_cert_enumerator = (void*)create_enumerator;
        this->public.set.create_shared_enumerator = (void*)return_null;
        this->public.set.create_cdp_enumerator = (void*)return_null;
-       this->public.set.cache_cert = (void*)cache_cert;
+       this->public.set.cache_cert = (void*)nop;
        this->public.issued_by = (bool(*)(cert_cache_t*, certificate_t *subject, certificate_t *issuer))issued_by;
        this->public.flush = (void(*)(cert_cache_t*, certificate_type_t type))flush;
        this->public.destroy = (void(*)(cert_cache_t*))destroy;
        
        this->relations = linked_list_create();
-       this->enumerating = FALSE;
+       this->enumerating = 0;
        this->check_required = FALSE;
-       this->mutex = mutex_create(MUTEX_RECURSIVE);
+       pthread_rwlock_init(&this->lock, NULL);
        
        return &this->public;
 }
index 897365e..8569f49 100644 (file)
  * $Id$
  */
 
+#define _GNU_SOURCE
+#include <pthread.h>
+
 #include "stroke_ca.h"
 #include "stroke_cred.h"
 
-#include <utils/mutex.h>
 #include <utils/linked_list.h>
 #include <crypto/hashers/hasher.h>
 
@@ -38,9 +40,9 @@ struct private_stroke_ca_t {
        stroke_ca_t public;
        
        /**
-        * mutex to lock access to list
+        * read-write lock to lists
         */
-       mutex_t *mutex;
+       pthread_rwlock_t lock;
        
        /**
         * list of starters CA sections and its certificates (ca_section_t)
@@ -134,7 +136,7 @@ typedef struct {
  */
 static void cdp_data_destroy(cdp_data_t *data)
 {
-       data->this->mutex->unlock(data->this->mutex);
+       pthread_rwlock_unlock(&data->this->lock);
        free(data);
 }
 
@@ -234,7 +236,7 @@ static enumerator_t *create_cdp_enumerator(private_stroke_ca_t *this,
        data->type = type;
        data->id = id;
        
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_rdlock(&this->lock);
        return enumerator_create_nested(this->sections->create_enumerator(this->sections),
                        (type == CERT_X509) ? (void*)create_inner_cdp_hashandurl : (void*)create_inner_cdp,
                        data, (void*)cdp_data_destroy);
@@ -276,9 +278,9 @@ static void add(private_stroke_ca_t *this, stroke_msg_t *msg)
                {
                        ca->certuribase = strdup(msg->add_ca.certuribase);
                }
-               this->mutex->lock(this->mutex);
+               pthread_rwlock_wrlock(&this->lock);
                this->sections->insert_last(this->sections, ca);
-               this->mutex->unlock(this->mutex);
+               pthread_rwlock_unlock(&this->lock);
                DBG1(DBG_CFG, "added ca '%s'", msg->add_ca.name);
        }
 }
@@ -291,7 +293,7 @@ static void del(private_stroke_ca_t *this, stroke_msg_t *msg)
        enumerator_t *enumerator;
        ca_section_t *ca = NULL;
        
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_wrlock(&this->lock);
        enumerator = this->sections->create_enumerator(this->sections);
        while (enumerator->enumerate(enumerator, &ca))
        {
@@ -303,7 +305,7 @@ static void del(private_stroke_ca_t *this, stroke_msg_t *msg)
                ca = NULL;
        }
        enumerator->destroy(enumerator);
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
        if (ca == NULL)
        {
                DBG1(DBG_CFG, "no ca named '%s' found\n", msg->del_ca.name);
@@ -354,7 +356,7 @@ static void check_for_hash_and_url(private_stroke_ca_t *this, certificate_t* cer
                return;
        }
        
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_wrlock(&this->lock);
        enumerator = this->sections->create_enumerator(this->sections);
        while (enumerator->enumerate(enumerator, (void**)&section))
        {
@@ -370,7 +372,7 @@ static void check_for_hash_and_url(private_stroke_ca_t *this, certificate_t* cer
                }
        }
        enumerator->destroy(enumerator);
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
        
        hasher->destroy(hasher);
 }
@@ -384,7 +386,7 @@ static void list(private_stroke_ca_t *this, stroke_msg_t *msg, FILE *out)
        ca_section_t *section;
        enumerator_t *enumerator;
        
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_rdlock(&this->lock);
        enumerator = this->sections->create_enumerator(this->sections);
        while (enumerator->enumerate(enumerator, (void**)&section))
        {
@@ -417,7 +419,7 @@ static void list(private_stroke_ca_t *this, stroke_msg_t *msg, FILE *out)
                }
        }
        enumerator->destroy(enumerator);
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
 }
 
 /**
@@ -426,7 +428,7 @@ static void list(private_stroke_ca_t *this, stroke_msg_t *msg, FILE *out)
 static void destroy(private_stroke_ca_t *this)
 {
        this->sections->destroy_function(this->sections, (void*)ca_section_destroy);
-       this->mutex->destroy(this->mutex);
+       pthread_rwlock_destroy(&this->lock);
        free(this);
 }
 
@@ -449,7 +451,7 @@ stroke_ca_t *stroke_ca_create(stroke_cred_t *cred)
        this->public.destroy = (void(*)(stroke_ca_t*))destroy;
        
        this->sections = linked_list_create();
-       this->mutex = mutex_create(MUTEX_RECURSIVE);
+       pthread_rwlock_init(&this->lock, NULL);
        this->cred = cred;
        
        return &this->public;
index 2235004..b1cd293 100644 (file)
  * $Id$
  */
 
-#include "stroke_cred.h"
-#include "stroke_shared_key.h"
-
+#define _GNU_SOURCE
+#include <pthread.h>
 #include <sys/stat.h>
 #include <limits.h>
 
+#include "stroke_cred.h"
+#include "stroke_shared_key.h"
+
 #include <credentials/certificates/x509.h>
 #include <credentials/certificates/crl.h>
 #include <credentials/certificates/ac.h>
 #include <utils/linked_list.h>
-#include <utils/mutex.h>
 #include <utils/lexparser.h>
 #include <asn1/pem.h>
 #include <daemon.h>
@@ -70,9 +71,9 @@ struct private_stroke_cred_t {
        linked_list_t *private;
        
        /**
-        * mutex to lock lists above
+        * read-write lock to lists
         */
-       mutex_t *mutex;
+       pthread_rwlock_t lock;
        
        /**
         * cache CRLs to disk?
@@ -93,7 +94,7 @@ typedef struct {
  */
 static void id_data_destroy(id_data_t *data)
 {
-       data->this->mutex->unlock(data->this->mutex);
+       pthread_rwlock_unlock(&data->this->lock);
        free(data);
 }
 
@@ -139,7 +140,7 @@ static enumerator_t* create_private_enumerator(private_stroke_cred_t *this,
        data->this = this;
        data->id = id;
        
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_rdlock(&this->lock);
        return enumerator_create_filter(this->private->create_enumerator(this->private),
                                                                        (void*)private_filter, data,
                                                                        (void*)id_data_destroy);
@@ -240,7 +241,7 @@ static enumerator_t* create_cert_enumerator(private_stroke_cred_t *this,
                data->this = this;
                data->id = id;
                
-               this->mutex->lock(this->mutex);
+               pthread_rwlock_rdlock(&this->lock);
                return enumerator_create_filter(this->certs->create_enumerator(this->certs),
                                        (cert == CERT_X509_CRL)? (void*)crl_filter : (void*)ac_filter,
                                        data, (void*)id_data_destroy);
@@ -253,7 +254,7 @@ static enumerator_t* create_cert_enumerator(private_stroke_cred_t *this,
        data->this = this;
        data->id = id;
        
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_rdlock(&this->lock);
        return enumerator_create_filter(this->certs->create_enumerator(this->certs),
                                                                        (void*)certs_filter, data,
                                                                        (void*)id_data_destroy);
@@ -271,7 +272,7 @@ typedef struct {
  */
 static void shared_data_destroy(shared_data_t *data)
 {
-       data->this->mutex->unlock(data->this->mutex);
+       pthread_rwlock_unlock(&data->this->lock);
        free(data);
 }
 
@@ -323,7 +324,7 @@ static enumerator_t* create_shared_enumerator(private_stroke_cred_t *this,
        data->me = me;
        data->other = other;
        data->type = type;
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_rdlock(&this->lock);
        return enumerator_create_filter(this->shared->create_enumerator(this->shared),
                                                                        (void*)shared_filter, data,
                                                                        (void*)shared_data_destroy);
@@ -338,7 +339,7 @@ static certificate_t* add_cert(private_stroke_cred_t *this, certificate_t *cert)
        enumerator_t *enumerator;
        bool new = TRUE;        
 
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_rdlock(&this->lock);
        enumerator = this->certs->create_enumerator(this->certs);
        while (enumerator->enumerate(enumerator, (void**)&current))
        {
@@ -357,7 +358,7 @@ static certificate_t* add_cert(private_stroke_cred_t *this, certificate_t *cert)
        {
                this->certs->insert_last(this->certs, cert);
        }
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
        return cert;
 }
        
@@ -399,7 +400,7 @@ static bool add_crl(private_stroke_cred_t *this, crl_t* crl)
        enumerator_t *enumerator;
        bool new = TRUE, found = FALSE; 
 
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_wrlock(&this->lock);
        enumerator = this->certs->create_enumerator(this->certs);
        while (enumerator->enumerate(enumerator, (void**)&current))
        {
@@ -447,7 +448,7 @@ static bool add_crl(private_stroke_cred_t *this, crl_t* crl)
        {
                this->certs->insert_last(this->certs, cert);
        }
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
        return new;
 }
 
@@ -458,9 +459,9 @@ static bool add_ac(private_stroke_cred_t *this, ac_t* ac)
 {
        certificate_t *cert = &ac->certificate;
 
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_wrlock(&this->lock);
        this->certs->insert_last(this->certs, cert);
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
        return TRUE;
 }
 
@@ -697,7 +698,7 @@ static void load_secrets(private_stroke_cred_t *this)
        fclose(fd);
        src = chunk;
 
-       this->mutex->lock(this->mutex);
+       pthread_rwlock_wrlock(&this->lock);
        while (this->shared->remove_last(this->shared,
                                                                                   (void**)&shared) == SUCCESS)
        {
@@ -859,7 +860,7 @@ static void load_secrets(private_stroke_cred_t *this)
                }
        }
 error:
-       this->mutex->unlock(this->mutex);
+       pthread_rwlock_unlock(&this->lock);
        chunk_clear(&chunk);
 }
 
@@ -940,7 +941,7 @@ static void destroy(private_stroke_cred_t *this)
        this->certs->destroy_offset(this->certs, offsetof(certificate_t, destroy));
        this->shared->destroy_offset(this->shared, offsetof(shared_key_t, destroy));
        this->private->destroy_offset(this->private, offsetof(private_key_t, destroy));
-       this->mutex->destroy(this->mutex);
+       pthread_rwlock_destroy(&this->lock);
        free(this);
 }
 
@@ -965,7 +966,7 @@ stroke_cred_t *stroke_cred_create()
        this->certs = linked_list_create();
        this->shared = linked_list_create();
        this->private = linked_list_create();
-       this->mutex = mutex_create(MUTEX_RECURSIVE);
+       pthread_rwlock_init(&this->lock, NULL);
 
        load_certs(this);
        load_secrets(this);