| Message ID | 20170207180617.5169-2-dmitry.ereminsolenikov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] linux-generic: crypto: add missing include | expand |
On Tue, Feb 7, 2017 at 12:06 PM, Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> wrote: > In preparation to update crypto code for OpenSSL 1.1.0 move locks out of > global data. > > Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> > --- > platform/linux-generic/odp_crypto.c | 67 ++++++++++++++++++++++++++----------- > 1 file changed, 47 insertions(+), 20 deletions(-) > > diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c > index b53b0fc1..d83b8e09 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -64,7 +64,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]; > }; > @@ -954,16 +953,53 @@ static unsigned long openssl_thread_id(void) > return (unsigned long)odp_thread_id(); > } > > +odp_ticketlock_t *openssl_locks; Not clear what advantage this offers since in OpenSSL v1.1.0 CRYPO_num_locks() returns 1 anyway. By having multiple reserve areas you have to add additional logic to handle cases where one reserve succeeds and the other fails, as noted below. > + > 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 openssl_init_locks(void) This routine can fail so you need to have an int return here, not void. > +{ > + int nlocks; > + size_t mem_size; > + odp_shm_t shm; > + int idx; > + > + nlocks = CRYPTO_num_locks(); > + if (nlocks <= 0) > + return; return -1 for failure. > + > + 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); Need to add a check to fail the init in case the reserve fails. > + > + openssl_locks = odp_shm_addr(shm); > + > + /* Clear it out */ > + memset(openssl_locks, 0, mem_size); This is unnecessary since odp_ticketlock_init() initializes the locks and requires no zeroing before calling it. > + > + 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 openssl_term_locks(void) > +{ > + CRYPTO_set_locking_callback(NULL); > + CRYPTO_set_id_callback(NULL); > + > + return odp_shm_free(odp_shm_lookup("crypto_openssl_locks")); > } > > int > @@ -972,12 +1008,10 @@ 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 +1029,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); > - } > + openssl_init_locks(); Need to check RC from this call and if -1 free the previous reserve of the global area to avoid leaking memory and return -1 from the crypto_init call. > > return 0; > } > @@ -1024,8 +1048,11 @@ int odp_crypto_term_global(void) > rc = -1; > } > > - CRYPTO_set_locking_callback(NULL); > - CRYPTO_set_id_callback(NULL); > + ret = openssl_term_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 >
Hi Dmitry, Have you got the CRYPTO_set_id_callback building issue? implicit declaration, because of failing define the macro OPENSSL_USE_DEPRECATED . Looks like the function is deprecated by #ifdef in openssl/crypto.h. # ifdef OPENSSL_USE_DEPRECATED DECLARE_DEPRECATED(void CRYPTO_set_id_callback(unsigned long (*func) (void))); /* * mkdef.pl cannot handle this next one so not inside DECLARE_DEPRECATED, * but still inside OPENSSL_USE_DEPRECATED */ unsigned long (*CRYPTO_get_id_callback(void)) (void); DECLARE_DEPRECATED(unsigned long CRYPTO_thread_id(void)); # endif Thanks, Forrest On 8 February 2017 at 02:06, Dmitry Eremin-Solenikov < dmitry.ereminsolenikov@linaro.org> wrote: > In preparation to update crypto code for OpenSSL 1.1.0 move locks out of > global data. > > Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> > --- > platform/linux-generic/odp_crypto.c | 67 ++++++++++++++++++++++++++---- > ------- > 1 file changed, 47 insertions(+), 20 deletions(-) > > diff --git a/platform/linux-generic/odp_crypto.c > b/platform/linux-generic/odp_crypto.c > index b53b0fc1..d83b8e09 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -64,7 +64,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]; > }; > @@ -954,16 +953,53 @@ 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 openssl_init_locks(void) > +{ > + int nlocks; > + size_t mem_size; > + odp_shm_t shm; > + int idx; > + > + nlocks = CRYPTO_num_locks(); > + 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 openssl_term_locks(void) > +{ > + CRYPTO_set_locking_callback(NULL); > + CRYPTO_set_id_callback(NULL); > + > + return odp_shm_free(odp_shm_lookup("crypto_openssl_locks")); > } > > int > @@ -972,12 +1008,10 @@ 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 +1029,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); > - } > + openssl_init_locks(); > > return 0; > } > @@ -1024,8 +1048,11 @@ int odp_crypto_term_global(void) > rc = -1; > } > > - CRYPTO_set_locking_callback(NULL); > - CRYPTO_set_id_callback(NULL); > + ret = openssl_term_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 > >
diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c index b53b0fc1..d83b8e09 100644 --- a/platform/linux-generic/odp_crypto.c +++ b/platform/linux-generic/odp_crypto.c @@ -64,7 +64,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]; }; @@ -954,16 +953,53 @@ 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 openssl_init_locks(void) +{ + int nlocks; + size_t mem_size; + odp_shm_t shm; + int idx; + + nlocks = CRYPTO_num_locks(); + 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 openssl_term_locks(void) +{ + CRYPTO_set_locking_callback(NULL); + CRYPTO_set_id_callback(NULL); + + return odp_shm_free(odp_shm_lookup("crypto_openssl_locks")); } int @@ -972,12 +1008,10 @@ 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 +1029,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); - } + openssl_init_locks(); return 0; } @@ -1024,8 +1048,11 @@ int odp_crypto_term_global(void) rc = -1; } - CRYPTO_set_locking_callback(NULL); - CRYPTO_set_id_callback(NULL); + ret = openssl_term_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) {
In preparation to update crypto code for OpenSSL 1.1.0 move locks out of global data. Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> --- platform/linux-generic/odp_crypto.c | 67 ++++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 20 deletions(-) -- 2.11.0