Don't take a write lock when freeing an EVP_PKEY

When freeing the last reference to an EVP_PKEY there is no point in
taking the lock for the key. It is the last reference and is being freed
so must only be being used by a single thread.

This should not have been the source of any contention so its unclear to
what extent this will improve performance. But we should not be locking
when we don't need to.

Partially fixes #20286

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20932)

diff --git a/crypto/evp/keymgmt_lib.c b/crypto/evp/keymgmt_lib.c
index 2d0238ee27124e76f151b7b58fdb7e8f46315453..47c802bfb4af809706180ad3eaf48014f36041e6 100644 (file)
--- a/crypto/evp/keymgmt_lib.c
+++ b/crypto/evp/keymgmt_lib.c
@@ -194,7 +194,7 @@ void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt,
      * operation cache.  In that case, we know that |i| is zero.
      */
     if (pk->dirty_cnt != pk->dirty_cnt_copy)
-        evp_keymgmt_util_clear_operation_cache(pk, 0);
+        evp_keymgmt_util_clear_operation_cache(pk);
 
     /* Add the new export to the operation cache */
     if (!evp_keymgmt_util_cache_keydata(pk, keymgmt, import_data.keydata,
@@ -219,15 +219,11 @@ static void op_cache_free(OP_CACHE_ELEM *e)
     OPENSSL_free(e);
 }
 
-int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking)
+int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk)
 {
     if (pk != NULL) {
-        if (locking && pk->lock != NULL && !CRYPTO_THREAD_write_lock(pk->lock))
-            return 0;
         sk_OP_CACHE_ELEM_pop_free(pk->operation_cache, op_cache_free);
         pk->operation_cache = NULL;
-        if (locking && pk->lock != NULL)
-            CRYPTO_THREAD_unlock(pk->lock);
     }
 
     return 1;

diff --git a/crypto/evp/p_lib.c b/crypto/evp/p_lib.c
index fa51304c97bea0c3aaaa163285ee22f8eb50d862..d0a7fde1e6c6351c809eacc6cce80c1a517daf50 100644 (file)
--- a/crypto/evp/p_lib.c
+++ b/crypto/evp/p_lib.c
@@ -1757,7 +1757,7 @@ void evp_pkey_free_legacy(EVP_PKEY *x)
 static void evp_pkey_free_it(EVP_PKEY *x)
 {
     /* internal function; x is never NULL */
-    evp_keymgmt_util_clear_operation_cache(x, 1);
+    evp_keymgmt_util_clear_operation_cache(x);
 #ifndef FIPS_MODULE
     evp_pkey_free_legacy(x);
 #endif
@@ -1936,7 +1936,7 @@ void *evp_pkey_export_to_provider(EVP_PKEY *pk, OSSL_LIB_CTX *libctx,
         if (!CRYPTO_THREAD_write_lock(pk->lock))
             goto end;
         if (pk->ameth->dirty_cnt(pk) != pk->dirty_cnt_copy
-                && !evp_keymgmt_util_clear_operation_cache(pk, 0)) {
+                && !evp_keymgmt_util_clear_operation_cache(pk)) {
             CRYPTO_THREAD_unlock(pk->lock);
             evp_keymgmt_freedata(tmp_keymgmt, keydata);
             keydata = NULL;

diff --git a/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod b/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod
index 7099e449647312786325cc78ad2cec6857da3905..0a32da25a98991a94bd6b84074b37329330bf297 100644 (file)
--- a/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod
+++ b/doc/internal/man3/evp_keymgmt_util_export_to_provider.pod
@@ -25,7 +25,7 @@ OP_CACHE_ELEM
  OP_CACHE_ELEM *evp_keymgmt_util_find_operation_cache(EVP_PKEY *pk,
                                                       EVP_KEYMGMT *keymgmt,
                                                       int selection);
- int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking);
+ int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk);
  int evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt,
                                     void *keydata, int selection);
  void evp_keymgmt_util_cache_keyinfo(EVP_PKEY *pk);
@@ -52,9 +52,8 @@ I<keymgmt> in I<pk>'s cache of provided keys for operations.
 It should only be called while holding I<pk>'s lock (read or write).
 
 evp_keymgmt_util_clear_operation_cache() can be used to explicitly
-clear the cache of operation key references. If I<locking> is set to 1 then
-then I<pk>'s lock will be obtained while doing the clear. Otherwise it will be
-assumed that the lock has already been obtained or is not required.
+clear the cache of operation key references. If required the lock must already
+have been obtained.
 
 evp_keymgmt_util_cache_keydata() can be used to add a provider key
 object to a B<PKEY>.

diff --git a/include/crypto/evp.h b/include/crypto/evp.h
index 55b44cd8353c92cfaddd725b327472be903cc554..4f7f0c6eeaa8fdc114cfce8241976f74960d7c74 100644 (file)
--- a/include/crypto/evp.h
+++ b/include/crypto/evp.h
@@ -785,7 +785,7 @@ void *evp_keymgmt_util_export_to_provider(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt,
 OP_CACHE_ELEM *evp_keymgmt_util_find_operation_cache(EVP_PKEY *pk,
                                                      EVP_KEYMGMT *keymgmt,
                                                      int selection);
-int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk, int locking);
+int evp_keymgmt_util_clear_operation_cache(EVP_PKEY *pk);
 int evp_keymgmt_util_cache_keydata(EVP_PKEY *pk, EVP_KEYMGMT *keymgmt,
                                    void *keydata, int selection);
 void evp_keymgmt_util_cache_keyinfo(EVP_PKEY *pk);