rgw: setup locks for libopenssl #20390

theanalyst · 2018-02-09T16:49:38Z

openssl <= 1.02 requires explicit callbacks for locking which libcurl doesn't
set. This causes random segmentation faults when openssl uses its global
structures across multiple threads. Providing a simple mutex lock/unlock
functions as a callback.

https://curl.haxx.se/libcurl/c/threadsafe.html
https://www.openssl.org/docs/man1.0.2/crypto/threads.html#DESCRIPTION

Fixes: http://tracker.ceph.com/issues/22951
Signed-off-by: Abhishek Lekshmanan abhishek@suse.com
Signed-off-by: Jesse Williamson jwilliamson@suse.com

edit- tracker ref

mattbenjamin · 2018-02-09T16:59:18Z

@mdw-at-linuxbox please have a look, sir :) [I'm interested in whether this could be a root cause and fix for memory leak issues, as well?]

theanalyst · 2018-02-12T09:36:20Z

jenkins test this please (arm test failure on bluefs: ceph.conf not found)

theanalyst · 2018-02-12T14:35:37Z

This was triggered by using https://github.com/rakyll/hey which is a load gen similar to apache bench and setting up openstack keystone on a vstart cluster and making aws requests which force keystone to be contacted at every request.

something as simple as

$ hey -c 50 -n 100000 -H "Authorization: AWS access1:bar" -H "DATE: $(LC_ALL=C date -u +'%a, %d %b %Y %X %z')" http://localhost:8000/foo/bar

could trigger the crash in my env. (s3 auth use keystone and keystone url were set)

mdw-at-linuxbox · 2018-02-12T22:35:25Z

I don't think this will fix any memory leak problems with curl/nss. I can believe it will fix crashing problems with curl/openssl.

This is a really ugly property of curl, and one that does not make me happy. What it means is that somehow, at compile time, the configuration logic needs to determine if curl is going to be linked against openssl or gnutls, and make the appropriate lock setup calls directly. Failure to get it rlght = crash.

theanalyst · 2018-02-12T23:01:33Z

I don't think this will fix any memory leak problems with curl/nss. I can believe it will fix crashing problems with curl/openssl.

I don't think so either, but we do see crashes with openssl and I believe it is related to this. I was able to test this locally with the command I posted.

This is a really ugly property of curl, and one that does not make me happy. What it means is that somehow, at compile time, the configuration logic needs to determine if curl is going to be linked against openssl or gnutls, and make the appropriate lock setup calls directly. Failure to get it rlght = crash.

We probably also need checks for openssl versions <=1.10? I'm not so sure whether we need a similar callbacks for newer versions, but I could be wrong

mdw-at-linuxbox · 2018-02-13T09:49:27Z

In fact you don't need those locking calls in newer versions of openssl - 1.1 removes them. So ideally there should also be a cmake test to see if openssl has/needs this locking stuff.

The locking stuff also affects civetweb but that's a different problem.

cbodley · 2018-02-13T14:33:59Z

So ideally there should also be a cmake test to see if openssl has/needs this locking stuff.

+1. and some way to test whether libcurl is using openssl (it's nss on rhel) - whether that's at build- or runtime

theanalyst · 2018-02-13T14:43:32Z

+1. and some way to test whether libcurl is using openssl (it's nss on rhel) - whether that's at build- or runtime

I tried using curl-config and cmake to get around. I assume since we depend on curl-devel we should be able to do it this way? (GetPrerequisites may be an alternative if we can map from the shared library back to package.. assuming we know the libopenssl so names for all distros)

cbodley

the curl-config stuff looks good!

cbodley · 2018-02-13T14:50:20Z

CMakeLists.txt

+    if (NOT NO_CURL_SSL_LINK)
+      if (OPENSSL_VERSION VERSION_LESS "1.1.0")
+        message(STATUS "Found openssl ${OPENSSL_VERSION} < 1.1.0 need to explicitly set locking primitives")
+        set(WITH_RADOSGW_OPENSSL ON)


this define is a lot more specific than 'radosgw uses openssl' - maybe something like WITH_LIBCURL_OPENSSL_LOCK_COMPAT

ack much better

cbodley · 2018-02-13T14:54:28Z

src/rgw/CMakeLists.txt

@@ -189,6 +194,10 @@ add_dependencies(radosgw cls_rgw cls_lock cls_refcount
  cls_version cls_replica_log cls_user)
 install(TARGETS radosgw DESTINATION bin)

+if (WITH_RADOSGW_OPENSSL)
+  target_link_libraries(radosgw ${OPENSSL_LIBRARIES})


@mdw-at-linuxbox recently argued against linking directly to openssl libs (in discussions about ssl for the beast frontend). Marcus, do you think we should use dlopen(LIBCRYPTO_SONAME) for this stuff instead? is there any risk that civetweb and libcurl would be linking this differently?

cbodley · 2018-02-13T14:57:13Z

src/rgw/rgw_http_client_ssl.h

+unsigned long rgw_ssl_thread_id_callback();
+
+
+class RGWSSLSetup


this class, along with the dout defines/includes, can live in the source file instead

mdw-at-linuxbox · 2018-02-14T10:18:09Z

I've figured out there's another wrinkle to this. Our debian build of ceph builds against "curl-gnutls", which needs a similar but different locking fix.

curl-config should be a safe way to do things provided it can tell you if openssl/gnutls is there. For openssl, you don't want to try forcing the system version of openssl - that way just creates many minefields. You can instead use OPENSSL_API_COMPAT to dissect what should be there, but it's usually better to just see if the relevant functions exist (or in this case do not).

theanalyst · 2018-02-14T10:33:47Z

I've figured out there's another wrinkle to this. Our debian build of ceph builds against "curl-gnutls", which needs a similar but different locking fix.

https://curl.haxx.se/docs/install.htm at select-tls-backend mentions --without-ssl --with-gnutls for this. All other ssl engines seem to be added after explicitly disabling openssl and enabling the other tls backends.

curl-config should be a safe way to do things provided it can tell you if openssl/gnutls is there. For openssl, you don't want to try forcing the system version of openssl - that way just creates many minefields. You can instead use OPENSSL_API_COMPAT to dissect what should be there, but it's usually better to just see if the relevant functions exist (or in this case do not).

Thanks; I'll try to rework the PR based on this

theanalyst · 2018-02-14T17:15:21Z

I have moved the curl related global functions to a seperate file; I'm still to use openssl_api_compat or using cmake's check_symbol for identifying fi openssl needs those locks. GnuTLS locking primitives look much simpler but they need to be added as well.

theanalyst · 2018-02-15T15:39:55Z

I tried looking into gnutls though I'm not sure what needs to be done in order to initialize this lib. https://gnutls.org/manual/html_node/Thread-safety.html doesn't mention any special functions to be called to set up other than linking the application against pthreads; https://www.gnupg.org/documentation/manuals/gcrypt-devel/Multi_002dThreading.html mentions the need for gcry_control(GCRYCTL_SET_THREAD_CBS); however https://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library.html mentions this option is obsolete.

theanalyst · 2018-02-15T15:43:46Z

Changelog:

moved all curl related initialization routines from rgw_main to a seperate rgw_http_client_curl
used openssl_api_compat to determine whether we need to include the locking callbacks.

chardan · 2018-02-15T15:50:56Z

src/rgw/rgw_http_client_curl.cc

+#include <openssl/crypto.h>
+#endif
+
+namespace rgw {


Since we're now using C++17, you can also write:
namespace rgw::curl {

My hope is to backport some of this to Luminous and Jewel since the bug affects all versions of rgw atm

chardan · 2018-02-15T15:54:30Z

src/rgw/rgw_http_client_curl.cc

+namespace rgw {
+namespace curl {
+
+#ifdef HAVE_CURL_MULTI_WAIT


This is fine, but you might also consider moving the ifdef inside of the function; the functionality is the same but it's a bit more concise.

chardan · 2018-02-15T15:56:22Z

src/rgw/rgw_http_client_curl.cc

+  return (unsigned long)pthread_self();
+}
+
+void init_ssl(){


Same comment here as above-- it may be slightly more clear moving the ifdefs inside of the function body, but it's your call.

chardan · 2018-02-15T15:56:48Z

src/rgw/rgw_http_client_curl.cc

+}
+
+unsigned long rgw_ssl_thread_id_callback(){
+  return (unsigned long)pthread_self();


Insert my usual nit against C style casts here. ;-)

theanalyst · 2018-02-19T19:09:41Z

There is a major case I didn't factor when writing this. Civetweb will init openssl libraries and setup locks when civetweb is terminated with an ssl endpoint. We shouldn't override the libopenssl locks in that case. I haven't looked into libcurl's code yet, but in the case of civetweb with ssl termination, there is a possibility that both the libraries might initialize libssl, in which case; we may need to tell curl not to initialize libopenssl. cc @mdw-at-linuxbox

mdw-at-linuxbox · 2018-02-19T21:28:49Z

Looking more closely at the gnutls stuff I don't see where there are any necessary locks either.
I also put together a heavily threaded standalone program that does lots of connects. Interestingly enough, I did not find any locks to be necessary with either gnutls or openssl. No crashes, no complaints either. (Openssl was xenial openssl 1.0.2g-1ubuntu4.6)

theanalyst · 2018-02-19T21:41:56Z

@mdw-at-linuxbox with openssl (assuming civetweb isn't https terminated) I'm almost always able to reproduce the crash with keystone turned on & if I run the program as I mentioned in comment#3; with civetweb terminated on ssl however I haven't been able to reproduce the crash (but civetweb sets up openssl locks when it is terminated with ssl; so that explains curl not needing this)

theanalyst · 2018-02-28T00:26:08Z

changeset

made rgw frontend map a boost::optional type
rgw::curl::setup_curl is called by rgw admin as well (fixed cmake accordingly to optionally link on rgw_a)
print in status when we may be setting up locks due to curl being linked with ssl

chardan · 2018-02-28T09:09:45Z

src/rgw/rgw_http_client_curl.cc

+}
+
+bool fe_inits_ssl(boost::optional <const fe_map_t&> m, long& curl_global_flags){
+  if (m) {


Not necessary, but notice that if you turned this into a guard:
if (!m)
return false;

...then the whole function doesn't need to be in an if-block. Up to you!

we also have to return false when we later find out that ssl_certificate is not set.

yuriw · 2018-03-01T16:20:47Z

@theanalyst this needs rebase

--- pr 20390 --- pulling https://github.com/theanalyst/ceph.git branch rgw/openssl-init
remote: Counting objects: 24, done.
remote: Total 24 (delta 19), reused 19 (delta 19), pack-reused 5
Unpacking objects: 100% (24/24), done.
From https://github.com/theanalyst/ceph

branch rgw/openssl-init -> FETCH_HEAD
Auto-merging src/rgw/rgw_main.cc
CONFLICT (content): Merge conflict in src/rgw/rgw_main.cc
Auto-merging src/rgw/rgw_admin.cc
Auto-merging CMakeLists.txt
Automatic merge failed; fix conflicts and then commit the result.
Traceback (most recent call last):
File "/home/yuriw/wip1/src/script/build-integration-branch", line 62, in
assert not r
AssertionError

theanalyst · 2018-03-01T16:24:31Z

@yuriw rebased, but I think this may conflict with #20635

theanalyst · 2018-03-01T16:42:18Z

@cbodley can you take a look at the new changes when you have the time.

@yuriw this definitely would conflict with #20635

cbodley · 2018-03-01T18:43:34Z

@cbodley can you take a look at the new changes when you have the time.

i've been following the changes and my approval still stands 👍

smithfarm · 2018-03-02T03:17:32Z

@theanalyst Please rebase.

openssl <= 1.02 requires explicit callbacks for locking which libcurl doesn't set. This causes random segmentation faults when openssl uses its global structures across multiple threads. Providing a simple mutex lock/unlock functions as a callback. We determine whether openssl is used for libcurl via curl-config utility which should be installed as a part of our curl development headers package. We also additionally check that the openssl version is < 1.1.0 which alleviates the need for these callbacks. In this patchset we have done the following: - move all curl related global init functionality under rgw::curl namespace since libcurl may need to set up various ssl libraries etc during its init - introduce WITH_CURL_OPENSSL in cmake this checks the backend curl is deployed with using curl-config. Since curl devel is expected to be installed anyway, this binary should be available and can help identify the ssl backend curl was compiled with. - we only setup the locks if beast/civetweb aren't terminated with ssl, since these libraries setup the locks anyway and we want to prevent double initialization of openssl. Also we pass in ~CURL_GLOBAL_SSL making curl not initialize openssl if civetwb/beast is initializing them. Unfortunately this flag is a noop from curl >= 7.57 wherein both the libraries will end up initializing openssl anyway, which might override certain settings like error strings if using openssl < 1.1 https://curl.haxx.se/libcurl/c/threadsafe.html https://www.openssl.org/docs/man1.0.2/crypto/threads.html#DESCRIPTION Fixes: http://tracker.ceph.com/issues/22951 Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com> Signed-off-by: Jesse Williamson <jwilliamson@suse.com>

Since rgw admin also needs to init curl globally where we don't have a frontend map Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

since we use http manager which in turn uses curl and uses curl multi interfaces. While curl is initialized at the first call of curl_easy_init() this method isn't guaranteed to be safe when multiple threads may call the function since curl_global_init isn't reentrant. Calling curl_global_init via rgw::curl::setup_curl which additionally sets up ssl interfaces etc. when openssl is used as curl's ssl backend. Similarly moving rgw target link to accomodate this change. Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

Since rgw admin can also use it which will fail otherwise. Fixes: http://tracker.ceph.com/issues/23203 Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

cbodley · 2018-03-05T16:42:41Z

src/rgw/rgw_admin.cc

@@ -7246,5 +7247,6 @@ int main(int argc, const char **argv)
    }
  }

+  rgw::curl::cleanup_curl();


a lot of the admin commands will do an early return, so we can't rely on this to catch everything - that's why i had to go with raii in #20694

makes sense, I'll update rgw_admin

use raii for cleanup in rgw_admin.cc

Since a lot of rgw-admin commands can exit early Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

theanalyst · 2018-03-05T17:49:12Z

@cbodley updated

yuriw · 2018-03-06T20:57:31Z

wip-yuri3-testing-2018-03-06-2056

yuriw · 2018-03-07T18:01:08Z

wip-yuri3-testing-2018-03-07-1524

mattbenjamin added rgw bug-fix DNM labels Feb 9, 2018

mattbenjamin requested review from mdw-at-linuxbox and cbodley February 9, 2018 17:02

cbodley reviewed Feb 13, 2018

View reviewed changes

theanalyst force-pushed the rgw/openssl-init branch 3 times, most recently from ed53413 to 4f083be Compare February 13, 2018 18:19

theanalyst force-pushed the rgw/openssl-init branch from b9e8112 to 5d9b884 Compare February 15, 2018 12:35

chardan reviewed Feb 15, 2018

View reviewed changes

theanalyst force-pushed the rgw/openssl-init branch from 42627a8 to 35f0f46 Compare February 28, 2018 00:25

chardan reviewed Feb 28, 2018

View reviewed changes

chardan approved these changes Feb 28, 2018

View reviewed changes

theanalyst mentioned this pull request Mar 1, 2018

rgw: curl* reuse and for debian, use openssl not gnutls. #20635

Merged

yuriw added the wip-yuri-testing label Mar 1, 2018

theanalyst force-pushed the rgw/openssl-init branch from 35f0f46 to 4571e64 Compare March 1, 2018 16:24

yuriw removed the wip-yuri-testing label Mar 1, 2018

theanalyst force-pushed the rgw/openssl-init branch from 4571e64 to 3828715 Compare March 2, 2018 09:42

theanalyst added 3 commits March 2, 2018 17:41

rgw: make frontend map optional for ssl init

55dde3d

Since rgw admin also needs to init curl globally where we don't have a frontend map Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

theanalyst force-pushed the rgw/openssl-init branch from 3828715 to 112ba0b Compare March 2, 2018 16:43

rgw: call setup and release curl handles in setup_curl

08c4c05

Since rgw admin can also use it which will fail otherwise. Fixes: http://tracker.ceph.com/issues/23203 Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

theanalyst mentioned this pull request Mar 5, 2018

rgw: initialize curl handle cache in rgw_admin #20694

Closed

cbodley reviewed Mar 5, 2018

View reviewed changes

rgw: rgw_admin use raii for curl init/teardown

4667937

Since a lot of rgw-admin commands can exit early Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

theanalyst force-pushed the rgw/openssl-init branch from 6fc49c7 to 4667937 Compare March 5, 2018 17:01

cbodley approved these changes Mar 5, 2018

View reviewed changes

cbodley added the wip-yuri3-testing label Mar 6, 2018

yuriw merged commit 39f7377 into ceph:master Mar 8, 2018

		unsigned long rgw_ssl_thread_id_callback();


		class RGWSSLSetup

rgw: setup locks for libopenssl #20390

rgw: setup locks for libopenssl #20390

Conversation

theanalyst commented Feb 9, 2018 • edited

mattbenjamin commented Feb 9, 2018 • edited

theanalyst commented Feb 12, 2018

theanalyst commented Feb 12, 2018 • edited

mdw-at-linuxbox commented Feb 12, 2018

theanalyst commented Feb 12, 2018

mdw-at-linuxbox commented Feb 13, 2018

cbodley commented Feb 13, 2018

theanalyst commented Feb 13, 2018 • edited

cbodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdw-at-linuxbox commented Feb 14, 2018

theanalyst commented Feb 14, 2018

theanalyst commented Feb 14, 2018

theanalyst commented Feb 15, 2018

theanalyst commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theanalyst commented Feb 19, 2018

mdw-at-linuxbox commented Feb 19, 2018

theanalyst commented Feb 19, 2018

theanalyst commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuriw commented Mar 1, 2018

theanalyst commented Mar 1, 2018

theanalyst commented Mar 1, 2018

cbodley commented Mar 1, 2018

smithfarm commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theanalyst commented Mar 5, 2018

yuriw commented Mar 6, 2018

yuriw commented Mar 7, 2018

theanalyst commented Feb 9, 2018 •

edited

mattbenjamin commented Feb 9, 2018 •

edited

theanalyst commented Feb 12, 2018 •

edited

theanalyst commented Feb 13, 2018 •

edited