Re: [lng-odp] [PATCH v2 2/3] linux-generic: crypto: move OpenSSL locks out of global crypto data

[Forrest Shi]

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.ereminsoleni...@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 
> ---
>  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_tlock;
> -   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 *)
> -   >openssl_lock[n]);
> +   odp_ticketlock_lock(_locks[n]);
> else
> -   odp_ticketlock_unlock((odp_ticketlock_t *)
> - >openssl_lock[n]);
> +   odp_ticketlock_unlock(_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(_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(>lock);
>
> -   if (nlocks > 0) {
> -   global->openssl_lock =
> -   (odp_ticketlock_t **)>sessions[MAX_
> SESSIONS];
> -
> -   for (idx = 0; idx < nlocks; idx++)
> -   odp_ticketlock_init((odp_ticketlock_t *)
> -   >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
>
>

Re: [lng-odp] [PATCH v2 2/3] linux-generic: crypto: move OpenSSL locks out of global crypto data

[Bill Fischofer]

On Tue, Feb 7, 2017 at 12:06 PM, Dmitry Eremin-Solenikov
 wrote:
> In preparation to update crypto code for OpenSSL 1.1.0 move locks out of
> global data.
>
> Signed-off-by: Dmitry Eremin-Solenikov 
> ---
>  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_tlock;
> -   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 *)
> -   >openssl_lock[n]);
> +   odp_ticketlock_lock(_locks[n]);
> else
> -   odp_ticketlock_unlock((odp_ticketlock_t *)
> - >openssl_lock[n]);
> +   odp_ticketlock_unlock(_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(_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(>lock);
>
> -   if (nlocks > 0) {
> -   global->openssl_lock =
> -   (odp_ticketlock_t **)>sessions[MAX_SESSIONS];
> -
> -   for (idx = 0; idx < nlocks; idx++)
> -   odp_ticketlock_init((odp_ticketlock_t *)
> -   >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
>