From 38a8e397396985480d8e6a1e55e16902ad961bfa Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 30 Jul 2008 11:38:44 +0000 Subject: [PATCH] using shared read locks in credential set enumerators to avoid deadlocks --- src/charon/credentials/credential_manager.c | 55 +++++++++++++++++++-- src/charon/credentials/credential_set.h | 7 +++ src/charon/credentials/sets/cert_cache.c | 50 +++++++++---------- src/charon/plugins/stroke/stroke_ca.c | 32 ++++++------ src/charon/plugins/stroke/stroke_cred.c | 45 ++++++++--------- 5 files changed, 119 insertions(+), 70 deletions(-) diff --git a/src/charon/credentials/credential_manager.c b/src/charon/credentials/credential_manager.c index 251194ba8..c850a1515 100644 --- a/src/charon/credentials/credential_manager.c +++ b/src/charon/credentials/credential_manager.c @@ -60,6 +60,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 */ @@ -407,16 +412,50 @@ 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 */ @@ -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); diff --git a/src/charon/credentials/credential_set.h b/src/charon/credentials/credential_set.h index 41c5b1674..6a973e991 100644 --- a/src/charon/credentials/credential_set.h +++ b/src/charon/credentials/credential_set.h @@ -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 { diff --git a/src/charon/credentials/sets/cert_cache.c b/src/charon/credentials/sets/cert_cache.c index 8af8bb619..4a9a97149 100644 --- a/src/charon/credentials/sets/cert_cache.c +++ b/src/charon/credentials/sets/cert_cache.c @@ -15,11 +15,13 @@ * $Id$ */ +#define _GNU_SOURCE +#include + #include "cert_cache.h" #include #include -#include #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, ¤t)) { @@ -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,21 +260,13 @@ 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. */ @@ -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; } diff --git a/src/charon/plugins/stroke/stroke_ca.c b/src/charon/plugins/stroke/stroke_ca.c index 897365eb0..8569f49c3 100644 --- a/src/charon/plugins/stroke/stroke_ca.c +++ b/src/charon/plugins/stroke/stroke_ca.c @@ -16,10 +16,12 @@ * $Id$ */ +#define _GNU_SOURCE +#include + #include "stroke_ca.h" #include "stroke_cred.h" -#include #include #include @@ -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**)§ion)) { @@ -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**)§ion)) { @@ -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; diff --git a/src/charon/plugins/stroke/stroke_cred.c b/src/charon/plugins/stroke/stroke_cred.c index 223500488..b1cd29380 100644 --- a/src/charon/plugins/stroke/stroke_cred.c +++ b/src/charon/plugins/stroke/stroke_cred.c @@ -15,17 +15,18 @@ * $Id$ */ -#include "stroke_cred.h" -#include "stroke_shared_key.h" - +#define _GNU_SOURCE +#include #include #include +#include "stroke_cred.h" +#include "stroke_shared_key.h" + #include #include #include #include -#include #include #include #include @@ -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**)¤t)) { @@ -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**)¤t)) { @@ -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);