From 2ba9fbdfe97604632bf844ac77a97287582222c8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 28 May 2019 16:11:43 +0100 Subject: [PATCH] Allow multiple concurrent readers for X.509 CRT frame and PK context Previously, only one thread could access the parsing cache of an X.509 CRT at a time. Firstly, this leads to significant performance penalties on systems running many concurrent threads which share CRT structures -- for example, server threads sharing an SSL configuration containing the server CRT. Secondly, the locking should be logically unnecessary, because the threads are supposed to access the CRT frame and PK in a read-only, or at least thread-safe manner. This commit modifies the X.509 CRT cache implementation by allowing an arbitrary number of concurrent readers, locking only the path of setting up and clearing the cache. --- include/mbedtls/x509_crt.h | 93 +++++++++++++++++++++------------ include/mbedtls/x509_internal.h | 4 ++ library/x509_crt.c | 28 ++++++---- 3 files changed, 81 insertions(+), 44 deletions(-) diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index fb5269e7e..3ef2b48dc 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -817,26 +817,31 @@ int mbedtls_x509_crt_flush_cache( mbedtls_x509_crt const *crt ); * \return A negative error code on failure. */ static inline int mbedtls_x509_crt_frame_acquire( mbedtls_x509_crt const *crt, - mbedtls_x509_crt_frame const **frame_ptr ) + mbedtls_x509_crt_frame const **dst ) { - int ret; + int ret = 0; #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_lock( &crt->cache->frame_mutex ) != 0 ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); -#endif - ret = mbedtls_x509_crt_cache_provide_frame( crt ); - if( ret != 0 ) + if( crt->cache->frame_readers == 0 ) +#endif /* MBEDTLS_THREADING_C */ { -#if defined(MBEDTLS_THREADING_C) - if( mbedtls_mutex_unlock( &crt->cache->frame_mutex ) != 0 ) - return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); -#endif - return( ret ); + ret = mbedtls_x509_crt_cache_provide_frame( crt ); } - *frame_ptr = crt->cache->frame; - return( 0 ); +#if defined(MBEDTLS_THREADING_C) + if( crt->cache->frame_readers == MBEDTLS_X509_CACHE_FRAME_READERS_MAX ) + return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); + + crt->cache->frame_readers++; + + if( mbedtls_mutex_unlock( &crt->cache->frame_mutex ) != 0 ) + return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); +#endif /* MBEDTLS_THREADING_C */ + + *dst = crt->cache->frame; + return( ret ); } /** @@ -846,17 +851,24 @@ static inline int mbedtls_x509_crt_frame_acquire( mbedtls_x509_crt const *crt, * \param crt The certificate for which a certificate frame has * previously been acquired. */ -static inline void mbedtls_x509_crt_frame_release( mbedtls_x509_crt const *crt ) +static inline int mbedtls_x509_crt_frame_release( mbedtls_x509_crt const *crt ) { - ((void) crt); - #if defined(MBEDTLS_THREADING_C) + if( mbedtls_mutex_lock( &crt->cache->frame_mutex ) != 0 ) + return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); + + if( crt->cache->frame_readers == 0 ) + return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); + + crt->cache->frame_readers--; + mbedtls_mutex_unlock( &crt->cache->frame_mutex ); -#endif +#endif /* MBEDTLS_THREADING_C */ #if defined(MBEDTLS_X509_ALWAYS_FLUSH) - (void) mbedtls_x509_crt_flush_cache_frame( crt ); + (void) mbedtls_x509_crt_flush_cache_frame( crt ); #endif /* MBEDTLS_X509_ALWAYS_FLUSH */ + return( 0 ); } /** @@ -887,26 +899,31 @@ static inline void mbedtls_x509_crt_frame_release( mbedtls_x509_crt const *crt ) * \return A negative error code on failure. */ static inline int mbedtls_x509_crt_pk_acquire( mbedtls_x509_crt const *crt, - mbedtls_pk_context **pk_ptr ) + mbedtls_pk_context **dst ) { - int ret; + int ret = 0; #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_lock( &crt->cache->pk_mutex ) != 0 ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); -#endif - ret = mbedtls_x509_crt_cache_provide_pk( crt ); - if( ret != 0 ) + if( crt->cache->pk_readers == 0 ) +#endif /* MBEDTLS_THREADING_C */ { -#if defined(MBEDTLS_THREADING_C) - if( mbedtls_mutex_unlock( &crt->cache->pk_mutex ) != 0 ) - return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); -#endif - return( ret ); + ret = mbedtls_x509_crt_cache_provide_pk( crt ); } - *pk_ptr = crt->cache->pk; - return( 0 ); +#if defined(MBEDTLS_THREADING_C) + if( crt->cache->pk_readers == MBEDTLS_X509_CACHE_PK_READERS_MAX ) + return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); + + crt->cache->pk_readers++; + + if( mbedtls_mutex_unlock( &crt->cache->pk_mutex ) != 0 ) + return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); +#endif /* MBEDTLS_THREADING_C */ + + *dst = crt->cache->pk; + return( ret ); } /** @@ -916,17 +933,25 @@ static inline int mbedtls_x509_crt_pk_acquire( mbedtls_x509_crt const *crt, * \param crt The certificate for which a certificate frame has * previously been acquired. */ -static inline void mbedtls_x509_crt_pk_release( mbedtls_x509_crt const *crt ) +static inline int mbedtls_x509_crt_pk_release( mbedtls_x509_crt const *crt ) { - ((void) crt); - #if defined(MBEDTLS_THREADING_C) + if( mbedtls_mutex_lock( &crt->cache->pk_mutex ) != 0 ) + return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); + + if( crt->cache->pk_readers == 0 ) + return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); + + crt->cache->pk_readers--; + mbedtls_mutex_unlock( &crt->cache->pk_mutex ); -#endif +#endif /* MBEDTLS_THREADING_C */ #if defined(MBEDTLS_X509_ALWAYS_FLUSH) - (void) mbedtls_x509_crt_flush_cache_pk( crt ); + (void) mbedtls_x509_crt_flush_cache_pk( crt ); #endif /* MBEDTLS_X509_ALWAYS_FLUSH */ + + return( 0 ); } #endif /* MBEDTLS_X509_CRT_PARSE_C */ diff --git a/include/mbedtls/x509_internal.h b/include/mbedtls/x509_internal.h index 19d63ec00..3816c3611 100644 --- a/include/mbedtls/x509_internal.h +++ b/include/mbedtls/x509_internal.h @@ -33,9 +33,13 @@ struct mbedtls_x509_crt; struct mbedtls_pk_context; struct mbedtls_x509_crt_frame; +#define MBEDTLS_X509_CACHE_PK_READERS_MAX ((uint32_t) -1) +#define MBEDTLS_X509_CACHE_FRAME_READERS_MAX ((uint32_t) -1) typedef struct mbedtls_x509_crt_cache { #if defined(MBEDTLS_THREADING_C) + uint32_t frame_readers; + uint32_t pk_readers; mbedtls_threading_mutex_t frame_mutex; mbedtls_threading_mutex_t pk_mutex; #endif diff --git a/library/x509_crt.c b/library/x509_crt.c index 04e812556..03cda69c5 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -112,17 +112,21 @@ int mbedtls_x509_crt_flush_cache_pk( mbedtls_x509_crt const *crt ) #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_lock( &crt->cache->pk_mutex ) != 0 ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); -#endif + /* Can only free the PK context if nobody is using it. */ + if( crt->cache->pk_readers == 0 ) +#endif + { #if !defined(MBEDTLS_X509_ON_DEMAND_PARSING) - /* The cache holds a shallow copy of the PK context - * in the legacy struct, so don't free PK context. */ - mbedtls_free( crt->cache->pk ); + /* The cache holds a shallow copy of the PK context + * in the legacy struct, so don't free PK context. */ + mbedtls_free( crt->cache->pk ); #else - mbedtls_pk_free( crt->cache->pk ); - mbedtls_free( crt->cache->pk ); + mbedtls_pk_free( crt->cache->pk ); + mbedtls_free( crt->cache->pk ); #endif /* MBEDTLS_X509_ON_DEMAND_PARSING */ - crt->cache->pk = NULL; + crt->cache->pk = NULL; + } #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_unlock( &crt->cache->pk_mutex ) != 0 ) @@ -136,10 +140,14 @@ int mbedtls_x509_crt_flush_cache_frame( mbedtls_x509_crt const *crt ) #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_lock( &crt->cache->frame_mutex ) != 0 ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); -#endif - mbedtls_free( crt->cache->frame ); - crt->cache->frame = NULL; + /* Can only free the frame if nobody is using it. */ + if( crt->cache->frame_readers == 0 ) +#endif + { + mbedtls_free( crt->cache->frame ); + crt->cache->frame = NULL; + } #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_unlock( &crt->cache->frame_mutex ) != 0 )