| Message ID | 20170206214526.6926-1-dmitry.ereminsolenikov@linaro.org |
|---|---|
| State | New |
| Headers | show |
The problem is not what level of OpenSSL ODP might be compiled against, but what level is installed on the system the ODP application is running on, since we don't distribute OpenSSL with ODP. OpenSSL v1.1.0 is backward-compatible with the older callback structure (they become no-ops) so there's no real penalty for using them as the init/term calls are no-ops and the actual lock functions will never be called by OpenSSL. So I don't see a need for this patch. On Mon, Feb 6, 2017 at 3:45 PM, Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> wrote: > OpenSSL 1.1.0 uses new threading API. It is no longer necessary to set > locking callbacks to use OpenSSL in a multi-threaded environment. The > old threading API should no longer be used. Separate old locking code > into separate functions that are guarded by OPENSSL_VERSION_NUMBER > check. > > Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> > --- > .../linux-generic/include/odp_crypto_internal.h | 1 + > platform/linux-generic/odp_crypto.c | 78 ++++++++++++++++------ > 2 files changed, 59 insertions(+), 20 deletions(-) > > diff --git a/platform/linux-generic/include/odp_crypto_internal.h b/platform/linux-generic/include/odp_crypto_internal.h > index c7b893aa..f85b76ea 100644 > --- a/platform/linux-generic/include/odp_crypto_internal.h > +++ b/platform/linux-generic/include/odp_crypto_internal.h > @@ -13,6 +13,7 @@ extern "C" { > > #include <openssl/des.h> > #include <openssl/aes.h> > +#include <openssl/evp.h> > > #define MAX_IV_LEN 64 > #define OP_RESULT_MAGIC 0x91919191 > diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c > index b53b0fc1..2145c36e 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -26,6 +26,7 @@ > #include <openssl/rand.h> > #include <openssl/hmac.h> > #include <openssl/evp.h> > +#include <openssl/opensslv.h> > > #define MAX_SESSIONS 32 > > @@ -64,7 +65,6 @@ typedef struct odp_crypto_global_s odp_crypto_global_t; > > struct odp_crypto_global_s { > odp_spinlock_t lock; > - odp_ticketlock_t **openssl_lock; > odp_crypto_generic_session_t *free; > odp_crypto_generic_session_t sessions[0]; > }; > @@ -949,35 +949,80 @@ odp_crypto_operation(odp_crypto_op_param_t *param, > return 0; > } > > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > static unsigned long openssl_thread_id(void) > { > return (unsigned long)odp_thread_id(); > } > > +odp_ticketlock_t *openssl_locks; > + > static void openssl_lock(int mode, int n, > const char *file ODP_UNUSED, > int line ODP_UNUSED) > { > if (mode & CRYPTO_LOCK) > - odp_ticketlock_lock((odp_ticketlock_t *) > - &global->openssl_lock[n]); > + odp_ticketlock_lock(&openssl_locks[n]); > else > - odp_ticketlock_unlock((odp_ticketlock_t *) > - &global->openssl_lock[n]); > + odp_ticketlock_unlock(&openssl_locks[n]); > +} > + > +static void odp_crypto_init_openssl_locks(void) > +{ > + int nlocks = CRYPTO_num_locks(); > + size_t mem_size; > + odp_shm_t shm; > + int idx; > + > + if (nlocks <= 0) > + return; > + > + mem_size = nlocks * sizeof(odp_ticketlock_t); > + > + /* Allocate our globally shared memory */ > + shm = odp_shm_reserve("crypto_openssl_locks", mem_size, > + ODP_CACHE_LINE_SIZE, 0); > + > + openssl_locks = odp_shm_addr(shm); > + > + /* Clear it out */ > + memset(openssl_locks, 0, mem_size); > + > + for (idx = 0; idx < nlocks; idx++) > + odp_ticketlock_init(&openssl_locks[idx]); > + > + CRYPTO_set_id_callback(openssl_thread_id); > + CRYPTO_set_locking_callback(openssl_lock); > } > > +static int odp_crypto_term_openssl_locks(void) > +{ > + CRYPTO_set_locking_callback(NULL); > + CRYPTO_set_id_callback(NULL); > + > + return odp_shm_free(odp_shm_lookup("crypto_openssl_locks")); > +} > +#else > +static void odp_crypto_init_openssl_locks(void) > +{ > +} > + > +static int odp_crypto_term_openssl_locks(void) > +{ > + return 0; > +} > +#endif > + > int > odp_crypto_init_global(void) > { > size_t mem_size; > odp_shm_t shm; > int idx; > - int nlocks = CRYPTO_num_locks(); > > /* Calculate the memory size we need */ > mem_size = sizeof(*global); > mem_size += (MAX_SESSIONS * sizeof(odp_crypto_generic_session_t)); > - mem_size += nlocks * sizeof(odp_ticketlock_t); > > /* Allocate our globally shared memory */ > shm = odp_shm_reserve("crypto_pool", mem_size, > @@ -995,17 +1040,7 @@ odp_crypto_init_global(void) > } > odp_spinlock_init(&global->lock); > > - if (nlocks > 0) { > - global->openssl_lock = > - (odp_ticketlock_t **)&global->sessions[MAX_SESSIONS]; > - > - for (idx = 0; idx < nlocks; idx++) > - odp_ticketlock_init((odp_ticketlock_t *) > - &global->openssl_lock[idx]); > - > - CRYPTO_set_id_callback(openssl_thread_id); > - CRYPTO_set_locking_callback(openssl_lock); > - } > + odp_crypto_init_openssl_locks(); > > return 0; > } > @@ -1024,8 +1059,11 @@ int odp_crypto_term_global(void) > rc = -1; > } > > - CRYPTO_set_locking_callback(NULL); > - CRYPTO_set_id_callback(NULL); > + ret = odp_crypto_term_openssl_locks(); > + if (ret < 0) { > + ODP_ERR("shm free failed for crypto_openssl_locks\n"); > + rc = -1; > + } > > ret = odp_shm_free(odp_shm_lookup("crypto_pool")); > if (ret < 0) { > -- > 2.11.0 >
On 07.02.2017 00:57, Bill Fischofer wrote: > The problem is not what level of OpenSSL ODP might be compiled > against, but what level is installed on the system the ODP application > is running on, since we don't distribute OpenSSL with ODP. OpenSSL > v1.1.0 is backward-compatible with the older callback structure (they > become no-ops) so there's no real penalty for using them as the > init/term calls are no-ops > and the actual lock functions will never be called by OpenSSL. Compiler will bug out with 'function is not used' error with OpenSSL 1.1.0 headers. There is no way to use app compiled against 1.1.0 with earlier library versions or vice versa, because libssl will have different file/so names. -- With best wishes Dmitry
On Mon, Feb 6, 2017 at 5:15 PM, Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> wrote: > On 07.02.2017 00:57, Bill Fischofer wrote: >> >> The problem is not what level of OpenSSL ODP might be compiled >> against, but what level is installed on the system the ODP application >> is running on, since we don't distribute OpenSSL with ODP. OpenSSL >> v1.1.0 is backward-compatible with the older callback structure (they >> become no-ops) so there's no real penalty for using them as the >> init/term calls are no-ops >> and the actual lock functions will never be called by OpenSSL. > > > Compiler will bug out with 'function is not used' error with OpenSSL 1.1.0 > headers. There is no way to use app compiled against 1.1.0 with earlier > library versions or vice versa, because libssl will have different file/so > names. If that's true then OpenSSL v1.1.0 is failing the backward compatibility it claims to provide and I'd think that would be a bug against OpenSSL. Until OpenSSL v1.1.0 is part of all currently supported standard distros (Ubuntu 16.10 still uses v1.0.2g) we cannot compile ODP against those higher levels for distribution. If you compile against the older version then it should work against an installed copy of v1.1.0 as noted above since v1.1.0 is backwards-compatible. > > -- > With best wishes > Dmitry
I googled this link form compatibility for versions: https://abi-laboratory.pro/tracker/timeline/openssl/ What is function is not used error? Is it gcc or #error in includes? I see that functions should be empty macros: /* * The old locking functions have been removed completely without compatibility * macros. This is because the old functions either could not properly report * errors, or the returned error values were not clearly documented. * Replacing the locking functions with with no-ops would cause race condition * issues in the affected applications. It is far better for them to fail at * compile time. * On the other hand, the locking callbacks are no longer used. Consequently, * the callback management functions can be safely replaced with no-op macros. */ # define CRYPTO_num_locks() (1) # define CRYPTO_set_locking_callback(func) # define CRYPTO_get_locking_callback() (NULL) # define CRYPTO_set_add_lock_callback(func) # define CRYPTO_get_add_lock_callback() (NULL) On 7 February 2017 at 03:15, Bill Fischofer <bill.fischofer@linaro.org> wrote: > On Mon, Feb 6, 2017 at 5:15 PM, Dmitry Eremin-Solenikov > <dmitry.ereminsolenikov@linaro.org> wrote: > > On 07.02.2017 00:57, Bill Fischofer wrote: > >> > >> The problem is not what level of OpenSSL ODP might be compiled > >> against, but what level is installed on the system the ODP application > >> is running on, since we don't distribute OpenSSL with ODP. OpenSSL > >> v1.1.0 is backward-compatible with the older callback structure (they > >> become no-ops) so there's no real penalty for using them as the > >> init/term calls are no-ops > >> and the actual lock functions will never be called by OpenSSL. > > > > > > Compiler will bug out with 'function is not used' error with OpenSSL > 1.1.0 > > headers. There is no way to use app compiled against 1.1.0 with earlier > > library versions or vice versa, because libssl will have different > file/so > > names. > > If that's true then OpenSSL v1.1.0 is failing the backward > compatibility it claims to provide and I'd think that would be a bug > against OpenSSL. > > Until OpenSSL v1.1.0 is part of all currently supported standard > distros (Ubuntu 16.10 still uses v1.0.2g) we cannot compile ODP > against those higher levels for distribution. If you compile against > the older version then it should work against an installed copy of > v1.1.0 as noted above since v1.1.0 is backwards-compatible. > > > > > -- > > With best wishes > > Dmitry >
Hello,
On 7 February 2017 at 10:14, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> What is function is not used error? Is it gcc or #error in includes?
First error:
In file included from ./include/odp_packet_internal.h:28:0,
from odp_classification.c:13:
./include/odp_crypto_internal.h:55:5: error: unknown type name ‘EVP_CIPHER_CTX’
EVP_CIPHER_CTX *ctx;
^~~~~~~~~~~~~~
This is fixed by including <openssl/evp.h>
Second error:
odp_crypto.c:957:13: error: ‘openssl_lock’ defined but not used
[-Werror=unused-function]
static void openssl_lock(int mode, int n,
^~~~~~~~~~~~
odp_crypto.c:952:22: error: ‘openssl_thread_id’ defined but not used
[-Werror=unused-function]
static unsigned long openssl_thread_id(void)
^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
--
With best wishes
Dmitry
On 02/07/17 12:21, Dmitry Eremin-Solenikov wrote: > Hello, > > On 7 February 2017 at 10:14, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> What is function is not used error? Is it gcc or #error in includes? > > First error: > > In file included from ./include/odp_packet_internal.h:28:0, > from odp_classification.c:13: > ./include/odp_crypto_internal.h:55:5: error: unknown type name ‘EVP_CIPHER_CTX’ > EVP_CIPHER_CTX *ctx; > ^~~~~~~~~~~~~~ > > This is fixed by including <openssl/evp.h> > > Second error: > > odp_crypto.c:957:13: error: ‘openssl_lock’ defined but not used > [-Werror=unused-function] > static void openssl_lock(int mode, int n, > ^~~~~~~~~~~~ > odp_crypto.c:952:22: error: ‘openssl_thread_id’ defined but not used > [-Werror=unused-function] > static unsigned long openssl_thread_id(void) > ^~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > ok. It's better to split this patch on 2 patches. One for dynamic init array and other for check for odp version. As Bill said we need some portable solution which will work with any version of openssl. We don't know which exactly version will be so we cover common debian, ubuntu, openembedded and etc versions. I see that you took define from crypto.h. So portable code might be: instead of: +#if OPENSSL_VERSION_NUMBER < 0x10100000L write: if (OPENSSL_VERSION_NUMBER < 0x10100000L) odp_crypto_init_openssl_locks(); also it's better to avoid ifdefs from not hot code as match as possible, changing it to local ifs. In that case compiler will check that not used code is valid. btw, do not name any internal functions or macros with odp_ prefix. It stand for api functions. Maxim.
On 07.02.2017 18:59, Maxim Uvarov wrote: > On 02/07/17 12:21, Dmitry Eremin-Solenikov wrote: >> Hello, >> >> On 7 February 2017 at 10:14, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>> What is function is not used error? Is it gcc or #error in includes? >> >> First error: >> >> In file included from ./include/odp_packet_internal.h:28:0, >> from odp_classification.c:13: >> ./include/odp_crypto_internal.h:55:5: error: unknown type name ‘EVP_CIPHER_CTX’ >> EVP_CIPHER_CTX *ctx; >> ^~~~~~~~~~~~~~ >> >> This is fixed by including <openssl/evp.h> >> >> Second error: >> >> odp_crypto.c:957:13: error: ‘openssl_lock’ defined but not used >> [-Werror=unused-function] >> static void openssl_lock(int mode, int n, >> ^~~~~~~~~~~~ >> odp_crypto.c:952:22: error: ‘openssl_thread_id’ defined but not used >> [-Werror=unused-function] >> static unsigned long openssl_thread_id(void) >> ^~~~~~~~~~~~~~~~~ >> cc1: all warnings being treated as errors >> > > ok. > > It's better to split this patch on 2 patches. One for dynamic init array > and other for check for odp version. > > As Bill said we need some portable solution which will work with any > version of openssl. We don't know which exactly version will be so we > cover common debian, ubuntu, openembedded and etc versions. As I wrote previously, there is no way to have an app to be compiled against OpenSSL 1.0.x and running agains 1.1.y or vice versa. SONAMEs are different. > I see that you took define from crypto.h. So portable code might be: > > instead of: > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > > write: > > if (OPENSSL_VERSION_NUMBER < 0x10100000L) > odp_crypto_init_openssl_locks(); > > also it's better to avoid ifdefs from not hot code as match as possible, > changing it to local ifs. In that case compiler will check > that not used code is valid. I can mark those functions with __attribute__((unused)), if you'd prefer that over ifdefs. Is that better from your POV? > > btw, do not name any internal functions or macros with odp_ prefix. It > stand for api functions. ack. -- With best wishes Dmitry
On 02/07/17 20:52, Dmitry Eremin-Solenikov wrote: > On 07.02.2017 18:59, Maxim Uvarov wrote: >> On 02/07/17 12:21, Dmitry Eremin-Solenikov wrote: >>> Hello, >>> >>> On 7 February 2017 at 10:14, Maxim Uvarov <maxim.uvarov@linaro.org> >>> wrote: >>>> What is function is not used error? Is it gcc or #error in includes? >>> >>> First error: >>> >>> In file included from ./include/odp_packet_internal.h:28:0, >>> from odp_classification.c:13: >>> ./include/odp_crypto_internal.h:55:5: error: unknown type name >>> ‘EVP_CIPHER_CTX’ >>> EVP_CIPHER_CTX *ctx; >>> ^~~~~~~~~~~~~~ >>> >>> This is fixed by including <openssl/evp.h> >>> >>> Second error: >>> >>> odp_crypto.c:957:13: error: ‘openssl_lock’ defined but not used >>> [-Werror=unused-function] >>> static void openssl_lock(int mode, int n, >>> ^~~~~~~~~~~~ >>> odp_crypto.c:952:22: error: ‘openssl_thread_id’ defined but not used >>> [-Werror=unused-function] >>> static unsigned long openssl_thread_id(void) >>> ^~~~~~~~~~~~~~~~~ >>> cc1: all warnings being treated as errors >>> >> >> ok. >> >> It's better to split this patch on 2 patches. One for dynamic init array >> and other for check for odp version. >> >> As Bill said we need some portable solution which will work with any >> version of openssl. We don't know which exactly version will be so we >> cover common debian, ubuntu, openembedded and etc versions. > > As I wrote previously, there is no way to have an app to be compiled > against OpenSSL 1.0.x and running agains 1.1.y or vice versa. SONAMEs > are different. > Ok. That is not a problem and odp package should solve correct dependencies with their versions. Only thing we need to care is that we compile and work for most common versions. >> I see that you took define from crypto.h. So portable code might be: >> >> instead of: >> +#if OPENSSL_VERSION_NUMBER < 0x10100000L >> >> write: >> >> if (OPENSSL_VERSION_NUMBER < 0x10100000L) >> odp_crypto_init_openssl_locks(); >> > >> also it's better to avoid ifdefs from not hot code as match as possible, >> changing it to local ifs. In that case compiler will check >> that not used code is valid. > > I can mark those functions with __attribute__((unused)), if you'd prefer > that over ifdefs. Is that better from your POV? > That some times used and some times not, looks like good candidate for macro similar to kernel like: # define __maybe_unused __attribute__((unused)) ODP_MAYBE_UNUSED ? What do you think? Some functions defiantly should be under ifdef. Maxim. >> >> btw, do not name any internal functions or macros with odp_ prefix. It >> stand for api functions. > > ack. >
diff --git a/platform/linux-generic/include/odp_crypto_internal.h b/platform/linux-generic/include/odp_crypto_internal.h index c7b893aa..f85b76ea 100644 --- a/platform/linux-generic/include/odp_crypto_internal.h +++ b/platform/linux-generic/include/odp_crypto_internal.h @@ -13,6 +13,7 @@ extern "C" { #include <openssl/des.h> #include <openssl/aes.h> +#include <openssl/evp.h> #define MAX_IV_LEN 64 #define OP_RESULT_MAGIC 0x91919191 diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c index b53b0fc1..2145c36e 100644 --- a/platform/linux-generic/odp_crypto.c +++ b/platform/linux-generic/odp_crypto.c @@ -26,6 +26,7 @@ #include <openssl/rand.h> #include <openssl/hmac.h> #include <openssl/evp.h> +#include <openssl/opensslv.h> #define MAX_SESSIONS 32 @@ -64,7 +65,6 @@ typedef struct odp_crypto_global_s odp_crypto_global_t; struct odp_crypto_global_s { odp_spinlock_t lock; - odp_ticketlock_t **openssl_lock; odp_crypto_generic_session_t *free; odp_crypto_generic_session_t sessions[0]; }; @@ -949,35 +949,80 @@ odp_crypto_operation(odp_crypto_op_param_t *param, return 0; } +#if OPENSSL_VERSION_NUMBER < 0x10100000L static unsigned long openssl_thread_id(void) { return (unsigned long)odp_thread_id(); } +odp_ticketlock_t *openssl_locks; + static void openssl_lock(int mode, int n, const char *file ODP_UNUSED, int line ODP_UNUSED) { if (mode & CRYPTO_LOCK) - odp_ticketlock_lock((odp_ticketlock_t *) - &global->openssl_lock[n]); + odp_ticketlock_lock(&openssl_locks[n]); else - odp_ticketlock_unlock((odp_ticketlock_t *) - &global->openssl_lock[n]); + odp_ticketlock_unlock(&openssl_locks[n]); +} + +static void odp_crypto_init_openssl_locks(void) +{ + int nlocks = CRYPTO_num_locks(); + size_t mem_size; + odp_shm_t shm; + int idx; + + if (nlocks <= 0) + return; + + mem_size = nlocks * sizeof(odp_ticketlock_t); + + /* Allocate our globally shared memory */ + shm = odp_shm_reserve("crypto_openssl_locks", mem_size, + ODP_CACHE_LINE_SIZE, 0); + + openssl_locks = odp_shm_addr(shm); + + /* Clear it out */ + memset(openssl_locks, 0, mem_size); + + for (idx = 0; idx < nlocks; idx++) + odp_ticketlock_init(&openssl_locks[idx]); + + CRYPTO_set_id_callback(openssl_thread_id); + CRYPTO_set_locking_callback(openssl_lock); } +static int odp_crypto_term_openssl_locks(void) +{ + CRYPTO_set_locking_callback(NULL); + CRYPTO_set_id_callback(NULL); + + return odp_shm_free(odp_shm_lookup("crypto_openssl_locks")); +} +#else +static void odp_crypto_init_openssl_locks(void) +{ +} + +static int odp_crypto_term_openssl_locks(void) +{ + return 0; +} +#endif + int odp_crypto_init_global(void) { size_t mem_size; odp_shm_t shm; int idx; - int nlocks = CRYPTO_num_locks(); /* Calculate the memory size we need */ mem_size = sizeof(*global); mem_size += (MAX_SESSIONS * sizeof(odp_crypto_generic_session_t)); - mem_size += nlocks * sizeof(odp_ticketlock_t); /* Allocate our globally shared memory */ shm = odp_shm_reserve("crypto_pool", mem_size, @@ -995,17 +1040,7 @@ odp_crypto_init_global(void) } odp_spinlock_init(&global->lock); - if (nlocks > 0) { - global->openssl_lock = - (odp_ticketlock_t **)&global->sessions[MAX_SESSIONS]; - - for (idx = 0; idx < nlocks; idx++) - odp_ticketlock_init((odp_ticketlock_t *) - &global->openssl_lock[idx]); - - CRYPTO_set_id_callback(openssl_thread_id); - CRYPTO_set_locking_callback(openssl_lock); - } + odp_crypto_init_openssl_locks(); return 0; } @@ -1024,8 +1059,11 @@ int odp_crypto_term_global(void) rc = -1; } - CRYPTO_set_locking_callback(NULL); - CRYPTO_set_id_callback(NULL); + ret = odp_crypto_term_openssl_locks(); + if (ret < 0) { + ODP_ERR("shm free failed for crypto_openssl_locks\n"); + rc = -1; + } ret = odp_shm_free(odp_shm_lookup("crypto_pool")); if (ret < 0) {
OpenSSL 1.1.0 uses new threading API. It is no longer necessary to set locking callbacks to use OpenSSL in a multi-threaded environment. The old threading API should no longer be used. Separate old locking code into separate functions that are guarded by OPENSSL_VERSION_NUMBER check. Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> --- .../linux-generic/include/odp_crypto_internal.h | 1 + platform/linux-generic/odp_crypto.c | 78 ++++++++++++++++------ 2 files changed, 59 insertions(+), 20 deletions(-) -- 2.11.0