From d5aeab438988bcd6a931f9edf2553e4fd4e6d531 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Tue, 28 Nov 2023 13:55:49 -0500 Subject: [PATCH 1/2] Use context lock to protect only changed items OpenSSL is in charge of making sure the context is not used when it is freed. Repurpose the lock to only cover parts of the context that can be changed by independent threads after the context creation. Fixes: CID 468683 CID 468679 Signed-off-by: Simo Sorce --- src/provider.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/provider.c b/src/provider.c index 6f950da8..f4c33107 100644 --- a/src/provider.c +++ b/src/provider.c @@ -18,8 +18,6 @@ struct p11prov_ctx { P11PROV_IN_ERROR, } status; - pthread_rwlock_t rwlock; - /* Provider handles */ const OSSL_CORE_HANDLE *handle; OSSL_LIB_CTX *libctx; @@ -51,6 +49,7 @@ struct p11prov_ctx { OSSL_ALGORITHM *op_asym_cipher; OSSL_ALGORITHM *op_encoder; + pthread_rwlock_t quirk_lock; struct quirk *quirks; int nquirks; }; @@ -240,7 +239,7 @@ CK_RV p11prov_ctx_get_quirk(P11PROV_CTX *ctx, CK_SLOT_ID id, const char *name, int lock; CK_RV ret; - lock = pthread_rwlock_rdlock(&ctx->rwlock); + lock = pthread_rwlock_rdlock(&ctx->quirk_lock); if (lock != 0) { ret = CKR_CANT_LOCK; P11PROV_raise(ctx, ret, "Failure to rdlock! (%d)", errno); @@ -281,7 +280,7 @@ CK_RV p11prov_ctx_get_quirk(P11PROV_CTX *ctx, CK_SLOT_ID id, const char *name, ret = CKR_OK; done: - lock = pthread_rwlock_unlock(&ctx->rwlock); + lock = pthread_rwlock_unlock(&ctx->quirk_lock); if (lock != 0) { P11PROV_raise(ctx, CKR_CANT_LOCK, "Failure to unlock! (%d)", errno); /* we do not return an error in this case, as we got the info */ @@ -329,7 +328,7 @@ CK_RV p11prov_ctx_set_quirk(P11PROV_CTX *ctx, CK_SLOT_ID id, const char *name, memcpy(_data, data, _size); } - lock = pthread_rwlock_wrlock(&ctx->rwlock); + lock = pthread_rwlock_wrlock(&ctx->quirk_lock); if (lock != 0) { ret = CKR_CANT_LOCK; P11PROV_raise(ctx, ret, "Failure to wrlock! (%d)", errno); @@ -380,7 +379,7 @@ CK_RV p11prov_ctx_set_quirk(P11PROV_CTX *ctx, CK_SLOT_ID id, const char *name, done: P11PROV_debug("Set quirk '%s' of size %lu", name, size); - lock = pthread_rwlock_unlock(&ctx->rwlock); + lock = pthread_rwlock_unlock(&ctx->quirk_lock); if (lock != 0) { P11PROV_raise(ctx, CKR_CANT_LOCK, "Failure to unlock! (%d)", errno); /* we do not return an error in this case, as we got the info */ @@ -515,13 +514,6 @@ static void p11prov_ctx_free(P11PROV_CTX *ctx) { int ret; - ret = pthread_rwlock_wrlock(&ctx->rwlock); - if (ret != 0) { - P11PROV_raise(ctx, CKR_CANT_LOCK, - "Failure to wrlock! Data corruption may happen (%d)", - errno); - } - if (ctx->no_deinit) { ctx->status = P11PROV_NO_DEINIT; } @@ -549,6 +541,13 @@ static void p11prov_ctx_free(P11PROV_CTX *ctx) p11prov_module_free(ctx->module); ctx->module = NULL; + ret = pthread_rwlock_wrlock(&ctx->quirk_lock); + if (ret != 0) { + P11PROV_raise(ctx, CKR_CANT_LOCK, + "Failure to wrlock! Data corruption may happen (%d)", + errno); + } + if (ctx->quirks) { for (int i = 0; i < ctx->nquirks; i++) { OPENSSL_free(ctx->quirks[i].name); @@ -560,22 +559,23 @@ static void p11prov_ctx_free(P11PROV_CTX *ctx) OPENSSL_free(ctx->quirks); } - /* remove from pool */ - context_rm_pool(ctx); - - ret = pthread_rwlock_unlock(&ctx->rwlock); + ret = pthread_rwlock_unlock(&ctx->quirk_lock); if (ret != 0) { P11PROV_raise(ctx, CKR_CANT_LOCK, "Failure to unlock! Data corruption may happen (%d)", errno); } - ret = pthread_rwlock_destroy(&ctx->rwlock); + ret = pthread_rwlock_destroy(&ctx->quirk_lock); if (ret != 0) { P11PROV_raise(ctx, CKR_CANT_LOCK, "Failure to free lock! Data corruption may happen (%d)", errno); } + + /* remove from pool */ + context_rm_pool(ctx); + OPENSSL_clear_free(ctx, sizeof(P11PROV_CTX)); } @@ -1311,7 +1311,7 @@ int OSSL_provider_init(const OSSL_CORE_HANDLE *handle, const OSSL_DISPATCH *in, } ctx->handle = handle; - ret = pthread_rwlock_init(&ctx->rwlock, NULL); + ret = pthread_rwlock_init(&ctx->quirk_lock, NULL); if (ret != 0) { ret = errno; P11PROV_debug("rwlock init failed (%d)", ret); From 2e2c1873b6f4aba8ab18e8a3bc1acc4777ea611e Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Tue, 28 Nov 2023 14:00:48 -0500 Subject: [PATCH 2/2] Remove lockless peak from sessions managment We will always pay the price of a lock, but this is the more robust thing to do for now. Fixes: CID 468681 CID 468678 Signed-off-by: Simo Sorce --- src/session.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/session.c b/src/session.c index 9f16e278..cddd2ead 100644 --- a/src/session.c +++ b/src/session.c @@ -957,22 +957,16 @@ void p11prov_return_session(P11PROV_SESSION *session) pool = session->pool; if (pool) { - /* peek at the pool lockless worst case we waste some time */ - if (pool->open_sessions >= pool->max_cached_sessions) { - /* not much we can do if this fails, - * but only accounting will be a bit off */ - - /* LOCKED SECTION ------------- */ - if (MUTEX_LOCK(pool) == CKR_OK) { - if (pool->open_sessions >= pool->max_cached_sessions - && session != pool->login_session) { - token_session_close(session); - pool->open_sessions--; - } - (void)MUTEX_UNLOCK(pool); + /* LOCKED SECTION ------------- */ + if (MUTEX_LOCK(pool) == CKR_OK) { + if (pool->open_sessions >= pool->max_cached_sessions + && session != pool->login_session) { + token_session_close(session); + pool->open_sessions--; } - /* ------------- LOCKED SECTION */ + (void)MUTEX_UNLOCK(pool); } + /* ------------- LOCKED SECTION */ } ret = MUTEX_LOCK(session);