[v2,2/3] linux-generic: crypto: move OpenSSL locks out of global crypto data

Message ID 20170207180617.5169-2-dmitry.ereminsolenikov@linaro.org
State New
Headers show
Series
  • [v2,1/3] linux-generic: crypto: add missing include
Related show

Commit Message

Dmitry Eremin-Solenikov Feb. 7, 2017, 6:06 p.m.
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

Comments

Bill Fischofer Feb. 7, 2017, 8:58 p.m. | #1
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

>
Forrest Shi Feb. 13, 2017, 9:21 a.m. | #2
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

>

>

Patch

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) {