-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ThreadSanitizer reports lock-order-inversion in evp_extra_test #9454
Comments
Side note for the curious: the handy links were produced from the original tsan output using the following
|
The lock order inversions still prevail, but the too deep recursive read locks
|
My guess is that all providers need to document their dependencies before |
Thanks for the output. The test ran okay for me whereas previously it deadlocked hence my optimism. I'll try to have another look at this one tomorrow. I'm not sure what to do about the dependency graph. This test case jumps all over the place with respect to algorithm usage (& hence initialisation). Having the graph and initialising in a topologically sorted order won't necessarily address the problem with the lock inversion. E.g. initialising a public key algorithm then symmetric ciphers could start one way whereas reversing these could be another. Always initialising everything would work but it would be a big change from the current lazy model and one I'm not sure how palatable it would be. A single global initialisation lock might be the solution. |
I have of course not a ready solution. I just have the impression that it is not at all I mean from application code, it is probably not possible to know in advance what it will need. |
So long as the algorithm is fetched rather than being used, it should work. The CTR DRBG vs AES-CTR is a good example BTW. |
The drbg and aes-ctr provider is good today, but not so clear what will be added in the future. That may work today, but not when we do not know what new_func is actually doing. Why is there a recursion in the new_func ?
It would not need recursion, if the CTR is initialized before RAND |
@levitte @mattcaswell thoughts on the recursion question? |
Rather than initilisation, isn't it more that CTR DRBG needs an AES CTR cipher context? When a call is made to get data from the DRBG, the caller won't know what other cryptographic algorithm will be required. It's up to the specific DRBG to determine this. This is looking more problematic the more I dig in. |
I've emailed the project list for wider discussion. |
Some parts of OPENSSL_CTX intialisation can get quite complex (e.g. RAND). This can lead to complex interactions where different parts of the library try to initialise while other parts are still initialising. This can lead to deadlocks because both parts want to obtain the init lock. We separate out the init lock so that it is only used to manage the dynamic list of indexes. Each part of the library gets its own initialisation lock. Fixes openssl#9454
Today I hit the same deadlock in my app. Thread 1:
Thread 2:
Kind regards, |
@bernd-edlinger pointed me to his observation in #9437 (comment) that the ThreadSanitizer reports errors for the evp_extra_test. This here is only the first one reported.
It can be reproduced on current master (d0cf719) as follows:
The first issue reported by the ThreadSanitizer is a lock-order-inversion caused by
openssl_ctx_get_data(), which takes the context lock
openssl/crypto/context.c
Line 195 in d0cf719
and ossl_provider_forall_loaded() which takes the store lock
openssl/crypto/provider_core.c
Line 587 in d0cf719
Although the issue occurs during the test, I think it reveals a fundamental locking problem IMHO.
Here are the call stacks. (Forget about the #frame numbers, but the source code locations are real links!)
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32344)
Cycle in lock order graph: M44 (0x7b10000003c0) => M7 (0x7b1000000000) => M44
Mutex M7 acquired here while holding mutex M44 in main thread:
#0 pthread_rwlock_rdlock /var/tmp/portage/sys-devel/gcc-8.3.0-r1/work/gcc-8.3.0/libsanitizer/tsan/tsan_interceptors.cc:1242 (libtsan.so.0+0x2d7e9)
#1 CRYPTO_THREAD_read_lock crypto/threads_pthread.c:60 (libcrypto.so.3+0x206495)
#2 openssl_ctx_get_data crypto/context.c:195 (libcrypto.so.3+0x1f7059)
#3 ossl_namemap_stored crypto/core_namemap.c:92 (libcrypto.so.3+0x1f7c31)
#4 put_method_in_store crypto/evp/evp_fetch.c:126 (libcrypto.so.3+0x1d58af)
#5 ossl_method_construct_this crypto/core_fetch.c:55 (libcrypto.so.3+0x1f780a)
#6 algorithm_do_this crypto/core_algorithm.c:50 (libcrypto.so.3+0x1f748c)
#7 provider_forall_loaded crypto/provider_core.c:567 (libcrypto.so.3+0x2054ab)
#8 ossl_provider_forall_loaded crypto/provider_core.c:627 (libcrypto.so.3+0x2054ab)
#9 ossl_algorithm_do_all crypto/core_algorithm.c:72 (libcrypto.so.3+0x1f7679)
#10 ossl_method_construct crypto/core_fetch.c:92 (libcrypto.so.3+0x1f7a6b)
#11 evp_generic_fetch crypto/evp/evp_fetch.c:210 (libcrypto.so.3+0x1d5d39)
#12 EVP_MD_fetch crypto/evp/digest.c:686 (libcrypto.so.3+0x1bad13)
#13 EVP_DigestInit_ex crypto/evp/digest.c:177 (libcrypto.so.3+0x1bb400)
#14 EVP_DigestInit crypto/evp/digest.c:106 (libcrypto.so.3+0x1bb49a)
#15 sm2_compute_z_digest crypto/sm2/sm2_sign.c:64 (libcrypto.so.3+0x287cd0)
#16 pkey_sm2_digest_custom crypto/sm2/sm2_pmeth.c:287 (libcrypto.so.3+0x286e34)
#17 do_sigver_init crypto/evp/m_sigver.c:83 (libcrypto.so.3+0x1dc979)
#18 EVP_DigestVerifyInit crypto/evp/m_sigver.c:97 (libcrypto.so.3+0x1dc979)
#19 test_EVP_SM2_verify test/evp_extra_test.c:664 (evp_extra_test+0x599b)
#20 run_tests test/testutil/driver.c:353 (evp_extra_test+0x8f0d)
#21 main test/testutil/main.c:30 (evp_extra_test+0x4c4a)
Mutex M44 previously acquired by the same thread here:
#0 pthread_rwlock_rdlock /var/tmp/portage/sys-devel/gcc-8.3.0-r1/work/gcc-8.3.0/libsanitizer/tsan/tsan_interceptors.cc:1242 (libtsan.so.0+0x2d7e9)
#1 CRYPTO_THREAD_read_lock crypto/threads_pthread.c:60 (libcrypto.so.3+0x206495)
#2 ossl_provider_forall_loaded crypto/provider_core.c:587 (libcrypto.so.3+0x2052e2)
#3 ossl_algorithm_do_all crypto/core_algorithm.c:72 (libcrypto.so.3+0x1f7679)
#4 ossl_method_construct crypto/core_fetch.c:92 (libcrypto.so.3+0x1f7a6b)
#5 evp_generic_fetch crypto/evp/evp_fetch.c:210 (libcrypto.so.3+0x1d5d39)
#6 EVP_MD_fetch crypto/evp/digest.c:686 (libcrypto.so.3+0x1bad13)
#7 EVP_DigestInit_ex crypto/evp/digest.c:177 (libcrypto.so.3+0x1bb400)
#8 EVP_DigestInit crypto/evp/digest.c:106 (libcrypto.so.3+0x1bb49a)
#9 sm2_compute_z_digest crypto/sm2/sm2_sign.c:64 (libcrypto.so.3+0x287cd0)
#10 pkey_sm2_digest_custom crypto/sm2/sm2_pmeth.c:287 (libcrypto.so.3+0x286e34)
#11 do_sigver_init crypto/evp/m_sigver.c:83 (libcrypto.so.3+0x1dc979)
#12 EVP_DigestVerifyInit crypto/evp/m_sigver.c:97 (libcrypto.so.3+0x1dc979)
#13 test_EVP_SM2_verify test/evp_extra_test.c:664 (evp_extra_test+0x599b)
#14 run_tests test/testutil/driver.c:353 (evp_extra_test+0x8f0d)
#15 main test/testutil/main.c:30 (evp_extra_test+0x4c4a)
Mutex M44 acquired here while holding mutex M7 in main thread:
#0 pthread_rwlock_rdlock /var/tmp/portage/sys-devel/gcc-8.3.0-r1/work/gcc-8.3.0/libsanitizer/tsan/tsan_interceptors.cc:1242 (libtsan.so.0+0x2d7e9)
#1 CRYPTO_THREAD_read_lock crypto/threads_pthread.c:60 (libcrypto.so.3+0x206495)
#2 ossl_provider_forall_loaded crypto/provider_core.c:587 (libcrypto.so.3+0x2052e2)
#3 ossl_algorithm_do_all crypto/core_algorithm.c:72 (libcrypto.so.3+0x1f7679)
#4 ossl_method_construct crypto/core_fetch.c:92 (libcrypto.so.3+0x1f7a6b)
#5 evp_generic_fetch crypto/evp/evp_fetch.c:210 (libcrypto.so.3+0x1d5d39)
#6 EVP_CIPHER_fetch crypto/evp/evp_enc.c:1248 (libcrypto.so.3+0x1d4993)
#7 drbg_ctr_init crypto/rand/drbg_ctr.c:389 (libcrypto.so.3+0x242720)
#8 RAND_DRBG_set crypto/rand/drbg_lib.c:333 (libcrypto.so.3+0x2445b4)
#9 rand_drbg_new crypto/rand/drbg_lib.c:442 (libcrypto.so.3+0x246242)
#10 RAND_DRBG_secure_new_ex crypto/rand/drbg_lib.c:481 (libcrypto.so.3+0x246242)
#11 drbg_setup crypto/rand/drbg_lib.c:1120 (libcrypto.so.3+0x24672d)
#12 drbg_ossl_ctx_new crypto/rand/drbg_lib.c:175 (libcrypto.so.3+0x24672d)
#13 openssl_ctx_generic_new crypto/context.c:151 (libcrypto.so.3+0x1f6afe)
#14 CRYPTO_alloc_ex_data crypto/ex_data.c:424 (libcrypto.so.3+0x1f9aeb)
#15 openssl_ctx_get_data crypto/context.c:204 (libcrypto.so.3+0x1f7082)
#16 drbg_get_global crypto/rand/drbg_lib.c:245 (libcrypto.so.3+0x246d4a)
#17 OPENSSL_CTX_get0_private_drbg crypto/rand/drbg_lib.c:1362 (libcrypto.so.3+0x246d4a)
#18 rand_priv_bytes_ex crypto/rand/rand_lib.c:814 (libcrypto.so.3+0x248b41)
#19 rand_priv_bytes_ex crypto/rand/rand_lib.c:805 (libcrypto.so.3+0x248b41)
#20 bnrand crypto/bn/bn_rand.c:51 (libcrypto.so.3+0xf0ff4)
#21 bnrand_range crypto/bn/bn_rand.c:182 (libcrypto.so.3+0xf0ff4)
#22 bnrand_range crypto/bn/bn_rand.c:212 (libcrypto.so.3+0xf1ff2)
#23 BN_priv_rand_range_ex crypto/bn/bn_rand.c:211 (libcrypto.so.3+0xf1ff2)
#24 bn_miller_rabin_is_prime crypto/bn/bn_prime.c:324 (libcrypto.so.3+0xef871)
#25 bn_miller_rabin_is_prime crypto/bn/bn_prime.c:272 (libcrypto.so.3+0xef871)
#26 BN_is_prime_fasttest_ex crypto/bn/bn_prime.c:246 (libcrypto.so.3+0xefdf4)
#27 BN_is_prime_fasttest_ex crypto/bn/bn_prime.c:203 (libcrypto.so.3+0xefdf4)
#28 BN_is_prime_ex crypto/bn/bn_prime.c:199 (libcrypto.so.3+0xefe90)
#29 RSA_check_key_ex crypto/rsa/rsa_chk.c:82 (libcrypto.so.3+0x252365)
#30 RSA_check_key_ex crypto/rsa/rsa_chk.c:25 (libcrypto.so.3+0x252365)
#31 rsa_pkey_check crypto/rsa/rsa_ameth.c:1032 (libcrypto.so.3+0x24ece1)
#32 EVP_PKEY_check crypto/evp/pmeth_gn.c:192 (libcrypto.so.3+0x1e3fe2)
#33 test_EVP_PKEY_check test/evp_extra_test.c:965 (evp_extra_test+0x532a)
#34 run_tests test/testutil/driver.c:386 (evp_extra_test+0x8d63)
#35 main test/testutil/main.c:30 (evp_extra_test+0x4c4a)
Mutex M7 previously acquired by the same thread here:
#0 pthread_rwlock_rdlock /var/tmp/portage/sys-devel/gcc-8.3.0-r1/work/gcc-8.3.0/libsanitizer/tsan/tsan_interceptors.cc:1242 (libtsan.so.0+0x2d7e9)
#1 CRYPTO_THREAD_read_lock crypto/threads_pthread.c:60 (libcrypto.so.3+0x206495)
#2 openssl_ctx_get_data crypto/context.c:195 (libcrypto.so.3+0x1f7059)
#3 drbg_get_global crypto/rand/drbg_lib.c:245 (libcrypto.so.3+0x246d4a)
#4 OPENSSL_CTX_get0_private_drbg crypto/rand/drbg_lib.c:1362 (libcrypto.so.3+0x246d4a)
#5 rand_priv_bytes_ex crypto/rand/rand_lib.c:814 (libcrypto.so.3+0x248b41)
#6 rand_priv_bytes_ex crypto/rand/rand_lib.c:805 (libcrypto.so.3+0x248b41)
#7 bnrand crypto/bn/bn_rand.c:51 (libcrypto.so.3+0xf0ff4)
#8 bnrand_range crypto/bn/bn_rand.c:182 (libcrypto.so.3+0xf0ff4)
#9 bnrand_range crypto/bn/bn_rand.c:212 (libcrypto.so.3+0xf1ff2)
#10 BN_priv_rand_range_ex crypto/bn/bn_rand.c:211 (libcrypto.so.3+0xf1ff2)
#11 bn_miller_rabin_is_prime crypto/bn/bn_prime.c:324 (libcrypto.so.3+0xef871)
#12 bn_miller_rabin_is_prime crypto/bn/bn_prime.c:272 (libcrypto.so.3+0xef871)
#13 BN_is_prime_fasttest_ex crypto/bn/bn_prime.c:246 (libcrypto.so.3+0xefdf4)
#14 BN_is_prime_fasttest_ex crypto/bn/bn_prime.c:203 (libcrypto.so.3+0xefdf4)
#15 BN_is_prime_ex crypto/bn/bn_prime.c:199 (libcrypto.so.3+0xefe90)
#16 RSA_check_key_ex crypto/rsa/rsa_chk.c:82 (libcrypto.so.3+0x252365)
#17 RSA_check_key_ex crypto/rsa/rsa_chk.c:25 (libcrypto.so.3+0x252365)
#18 rsa_pkey_check crypto/rsa/rsa_ameth.c:1032 (libcrypto.so.3+0x24ece1)
#19 EVP_PKEY_check crypto/evp/pmeth_gn.c:192 (libcrypto.so.3+0x1e3fe2)
#20 test_EVP_PKEY_check test/evp_extra_test.c:965 (evp_extra_test+0x532a)
#21 run_tests test/testutil/driver.c:386 (evp_extra_test+0x8d63)
#22 main test/testutil/main.c:30 (evp_extra_test+0x4c4a)
The text was updated successfully, but these errors were encountered: