Fix context locking #9590
Fix context locking #9590
Conversation
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
Simplify the initialisation of the core by pre-initialising properties.
Don't modify the cipher/md we just fetched - it could be shared by multiple threads.
|
This is an alternative to #9513. |
|
|
||
| if (!crypto_new_ex_data_ex(ctx, CRYPTO_EX_INDEX_OPENSSL_CTX, NULL, | ||
| &ctx->data)) { | ||
| crypto_cleanup_all_ex_data_int(ctx); | ||
| goto err; | ||
| } | ||
|
|
||
| /* Everything depends on properties, so we also pre-initialise that */ | ||
| if (!ossl_property_parse_init(ctx)) |
There was a problem hiding this comment.
I think that do_method_store_init might be a better initialisation call under a openssl_ctx_run_once cover.
There was a problem hiding this comment.
I removed the openssl_ctx_run_once because it was one of the causes of the lock order inversion issues. By moving it here (during initialisation of the OPENSSL_CTX) we know that it won't be shared by any other threads yet so can avoid the extra lock. I'm slightly confused by your reference to do_method_store_init - since such a function doesn't exist. Or are you suggesting I create it? If so, the only thing it would do would call ossl_property_parse_init() I think.
There was a problem hiding this comment.
do_method_store_init() is in crypto/property/property.c. It's the RUN_ONCE that calls ossl_property_parse_init() (and that's the only thing it does).
The ossl_property_parse_init() function can safely be called multiple times, it is preloading our property names and the Boolean values. If any already exist, they aren't added repeatedly.
So it would be safe to leave this as it is (and perhaps remove do_method_store_init() completely).
|
Fixup commits pushed following review feedback. |
|
Pushed. Thanks. |
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 #9454 Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #9590)
Simplify the initialisation of the core by pre-initialising properties. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #9590)
Don't modify the cipher/md we just fetched - it could be shared by multiple threads. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #9590)
|
Hi guys, one question. I understand you add the patch to the 3.0 version but what about 1.1.x? So many commits and PR that I'm lost. Thanks. |
This PR fixes issues in the OPENSSL_CTX type. An OPENSSL_CTX can be considered like a library wide "scope". See also: https://www.openssl.org/docs/manmaster/man3/OPENSSL_CTX.html OPENSSL_CTX is a newly introduced concept in 3.0, so this issue does not affect 1.1.x and earlier. |
|
You might also like to read the 3.0 design document which explains how all these things fit together. |
|
Thanks @mattcaswell. I just realized 1.1.1 is in its specific branch and master has the new 3.0 code. Kind regards. |
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 #9454
I also simplified properties initialisation because there were some thread santizer issues cropping up in this area too. Finally I fixed some data races in EVP_CIPHER_fetch and EVP_MD_fetch.
There are still some thread sanitizer issues coming from evp_test that I haven't looked at yet, but I believe they are unrelated issues to those addressed here.