| Message ID | 1483660643-14984-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 6 January 2017 at 00:57, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2798 by adding > OpenSSL callbacks for locking that use ticketlocks to provide > thread-safety for OpenSSL calls made by ODP components such as random > number generation. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/odp_internal.h | 5 +++ > platform/linux-generic/odp_init.c | 63 +++++++++++++++++++++++++++ > 2 files changed, 68 insertions(+) > > diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h > index b313b1f..2280e3f 100644 > --- a/platform/linux-generic/include/odp_internal.h > +++ b/platform/linux-generic/include/odp_internal.h > @@ -20,6 +20,8 @@ extern "C" { > #include <odp/api/init.h> > #include <odp/api/cpumask.h> > #include <odp/api/thread.h> > +#include <odp/api/shared_memory.h> > +#include <odp/api/ticketlock.h> > #include <stdio.h> > #include <sys/types.h> > > @@ -50,6 +52,8 @@ struct odp_global_data_s { > odp_cpumask_t control_cpus; > odp_cpumask_t worker_cpus; > int num_cpus_installed; > + odp_shm_t openssl_shm; > + odp_ticketlock_t **openssl_lock; > }; > > enum init_stage { > @@ -66,6 +70,7 @@ enum init_stage { > PKTIO_INIT, > TIMER_INIT, > CRYPTO_INIT, > + OPENSSL_INIT, > CLASSIFICATION_INIT, > TRAFFIC_MNGR_INIT, > NAME_TABLE_INIT, > diff --git a/platform/linux-generic/odp_init.c b/platform/linux-generic/odp_init.c > index 1b0d8f8..6873bb5 100644 > --- a/platform/linux-generic/odp_init.c > +++ b/platform/linux-generic/odp_init.c > @@ -6,6 +6,8 @@ > #include <odp/api/init.h> > #include <odp_debug_internal.h> > #include <odp/api/debug.h> > +#include <openssl/opensslconf.h> > +#include <openssl/crypto.h> > #include <unistd.h> > #include <odp_internal.h> > #include <odp_schedule_if.h> > @@ -21,8 +23,30 @@ > #define _ODP_FILES_FMT "odp-%d-" > #define _ODP_TMPDIR "/tmp" > > +static inline int openssl_numlocks(void) > +{ > + return CRYPTO_num_locks(); > +} > + > struct odp_global_data_s odp_global_data; > > +static unsigned long openssl_thread_id(void) > +{ > + return (unsigned long)odp_thread_id(); > +} > + > +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 *) > + &odp_global_data.openssl_lock[n]); > + else > + odp_ticketlock_unlock((odp_ticketlock_t *) > + &odp_global_data.openssl_lock[n]); > +} > + > /* remove all files staring with "odp-<pid>" from a directory "dir" */ > static int cleanup_files(const char *dirpath, int odp_pid) > { > @@ -70,6 +94,8 @@ int odp_init_global(odp_instance_t *instance, > const odp_platform_init_t *platform_params ODP_UNUSED) > { > char *hpdir; > + int nlocks = openssl_numlocks(); > + int i; > > memset(&odp_global_data, 0, sizeof(struct odp_global_data_s)); > odp_global_data.main_pid = getpid(); > @@ -162,6 +188,34 @@ int odp_init_global(odp_instance_t *instance, > } > stage = CRYPTO_INIT; > > + if (nlocks > 0) { > + odp_global_data.openssl_shm = > + odp_shm_reserve("ODP OpenSSL Mutexes", > + nlocks * sizeof(odp_ticketlock_t), > + sizeof(odp_ticketlock_t), > + ODP_SHM_SW_ONLY); > + if (odp_global_data.openssl_shm == ODP_SHM_INVALID) { > + ODP_ERR("ODP OpenSSL init reserve failed.\n"); > + goto init_failed; > + } > + > + odp_global_data.openssl_lock = > + odp_shm_addr(odp_global_data.openssl_shm); > + if (odp_global_data.openssl_lock == NULL) { > + odp_shm_free(odp_global_data.openssl_shm); > + ODP_ERR("ODP OpenSSL init addr failed.\n"); > + goto init_failed; > + } > + > + for (i = 0; i < nlocks; i++) > + odp_ticketlock_init((odp_ticketlock_t *) > + &odp_global_data.openssl_lock[i]); > + > + CRYPTO_set_id_callback(openssl_thread_id); > + CRYPTO_set_locking_callback(openssl_lock); > + } > + stage = OPENSSL_INIT; > + > if (odp_classification_init_global()) { > ODP_ERR("ODP classification init failed.\n"); > goto init_failed; > @@ -224,6 +278,15 @@ int _odp_term_global(enum init_stage stage) > } > /* Fall through */ > > + case OPENSSL_INIT: > + if (odp_global_data.openssl_lock) { > + CRYPTO_set_locking_callback(NULL); > + CRYPTO_set_id_callback(NULL); > + > + odp_shm_free(odp_global_data.openssl_shm); > + } > + /* Fall through */ > + This patch makes sense but should be part of odp_crypto_init_global() and odp_crypto_term_global(), right? I don't understand why it should be part of init.c, as it is only crypto function refering to openssl, right? (please explain if I am missing something) I have read you request for testing. I will do that over the coming week-end. As it took nearly 2 days on my system to trigger the fault, any tests shorter than that would be a bit meaningless. Christophe > case CRYPTO_INIT: > if (odp_crypto_term_global()) { > ODP_ERR("ODP crypto term failed.\n"); > -- > 2.7.4 >
On Mon, Jan 9, 2017 at 2:23 AM, Christophe Milard <christophe.milard@linaro.org> wrote: > On 6 January 2017 at 00:57, Bill Fischofer <bill.fischofer@linaro.org> wrote: >> Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2798 by adding >> OpenSSL callbacks for locking that use ticketlocks to provide >> thread-safety for OpenSSL calls made by ODP components such as random >> number generation. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> platform/linux-generic/include/odp_internal.h | 5 +++ >> platform/linux-generic/odp_init.c | 63 +++++++++++++++++++++++++++ >> 2 files changed, 68 insertions(+) >> >> diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h >> index b313b1f..2280e3f 100644 >> --- a/platform/linux-generic/include/odp_internal.h >> +++ b/platform/linux-generic/include/odp_internal.h >> @@ -20,6 +20,8 @@ extern "C" { >> #include <odp/api/init.h> >> #include <odp/api/cpumask.h> >> #include <odp/api/thread.h> >> +#include <odp/api/shared_memory.h> >> +#include <odp/api/ticketlock.h> >> #include <stdio.h> >> #include <sys/types.h> >> >> @@ -50,6 +52,8 @@ struct odp_global_data_s { >> odp_cpumask_t control_cpus; >> odp_cpumask_t worker_cpus; >> int num_cpus_installed; >> + odp_shm_t openssl_shm; >> + odp_ticketlock_t **openssl_lock; >> }; >> >> enum init_stage { >> @@ -66,6 +70,7 @@ enum init_stage { >> PKTIO_INIT, >> TIMER_INIT, >> CRYPTO_INIT, >> + OPENSSL_INIT, >> CLASSIFICATION_INIT, >> TRAFFIC_MNGR_INIT, >> NAME_TABLE_INIT, >> diff --git a/platform/linux-generic/odp_init.c b/platform/linux-generic/odp_init.c >> index 1b0d8f8..6873bb5 100644 >> --- a/platform/linux-generic/odp_init.c >> +++ b/platform/linux-generic/odp_init.c >> @@ -6,6 +6,8 @@ >> #include <odp/api/init.h> >> #include <odp_debug_internal.h> >> #include <odp/api/debug.h> >> +#include <openssl/opensslconf.h> >> +#include <openssl/crypto.h> >> #include <unistd.h> >> #include <odp_internal.h> >> #include <odp_schedule_if.h> >> @@ -21,8 +23,30 @@ >> #define _ODP_FILES_FMT "odp-%d-" >> #define _ODP_TMPDIR "/tmp" >> >> +static inline int openssl_numlocks(void) >> +{ >> + return CRYPTO_num_locks(); >> +} >> + >> struct odp_global_data_s odp_global_data; >> >> +static unsigned long openssl_thread_id(void) >> +{ >> + return (unsigned long)odp_thread_id(); >> +} >> + >> +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 *) >> + &odp_global_data.openssl_lock[n]); >> + else >> + odp_ticketlock_unlock((odp_ticketlock_t *) >> + &odp_global_data.openssl_lock[n]); >> +} >> + >> /* remove all files staring with "odp-<pid>" from a directory "dir" */ >> static int cleanup_files(const char *dirpath, int odp_pid) >> { >> @@ -70,6 +94,8 @@ int odp_init_global(odp_instance_t *instance, >> const odp_platform_init_t *platform_params ODP_UNUSED) >> { >> char *hpdir; >> + int nlocks = openssl_numlocks(); >> + int i; >> >> memset(&odp_global_data, 0, sizeof(struct odp_global_data_s)); >> odp_global_data.main_pid = getpid(); >> @@ -162,6 +188,34 @@ int odp_init_global(odp_instance_t *instance, >> } >> stage = CRYPTO_INIT; >> >> + if (nlocks > 0) { >> + odp_global_data.openssl_shm = >> + odp_shm_reserve("ODP OpenSSL Mutexes", >> + nlocks * sizeof(odp_ticketlock_t), >> + sizeof(odp_ticketlock_t), >> + ODP_SHM_SW_ONLY); >> + if (odp_global_data.openssl_shm == ODP_SHM_INVALID) { >> + ODP_ERR("ODP OpenSSL init reserve failed.\n"); >> + goto init_failed; >> + } >> + >> + odp_global_data.openssl_lock = >> + odp_shm_addr(odp_global_data.openssl_shm); >> + if (odp_global_data.openssl_lock == NULL) { >> + odp_shm_free(odp_global_data.openssl_shm); >> + ODP_ERR("ODP OpenSSL init addr failed.\n"); >> + goto init_failed; >> + } >> + >> + for (i = 0; i < nlocks; i++) >> + odp_ticketlock_init((odp_ticketlock_t *) >> + &odp_global_data.openssl_lock[i]); >> + >> + CRYPTO_set_id_callback(openssl_thread_id); >> + CRYPTO_set_locking_callback(openssl_lock); >> + } >> + stage = OPENSSL_INIT; >> + >> if (odp_classification_init_global()) { >> ODP_ERR("ODP classification init failed.\n"); >> goto init_failed; >> @@ -224,6 +278,15 @@ int _odp_term_global(enum init_stage stage) >> } >> /* Fall through */ >> >> + case OPENSSL_INIT: >> + if (odp_global_data.openssl_lock) { >> + CRYPTO_set_locking_callback(NULL); >> + CRYPTO_set_id_callback(NULL); >> + >> + odp_shm_free(odp_global_data.openssl_shm); >> + } >> + /* Fall through */ >> + > > This patch makes sense but should be part of odp_crypto_init_global() > and odp_crypto_term_global(), right? > I don't understand why it should be part of init.c, as it is only > crypto function refering to openssl, right? (please explain if I am > missing something) Since both you and Petri have made this point I'll do that in v3 > > I have read you request for testing. I will do that over the coming > week-end. As it took nearly 2 days on my system to trigger the fault, > any tests shorter than that would be a bit meaningless. I didn't realize it was that hard to trigger, but thanks. > > Christophe > > >> case CRYPTO_INIT: >> if (odp_crypto_term_global()) { >> ODP_ERR("ODP crypto term failed.\n"); >> -- >> 2.7.4 >>
diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h index b313b1f..2280e3f 100644 --- a/platform/linux-generic/include/odp_internal.h +++ b/platform/linux-generic/include/odp_internal.h @@ -20,6 +20,8 @@ extern "C" { #include <odp/api/init.h> #include <odp/api/cpumask.h> #include <odp/api/thread.h> +#include <odp/api/shared_memory.h> +#include <odp/api/ticketlock.h> #include <stdio.h> #include <sys/types.h> @@ -50,6 +52,8 @@ struct odp_global_data_s { odp_cpumask_t control_cpus; odp_cpumask_t worker_cpus; int num_cpus_installed; + odp_shm_t openssl_shm; + odp_ticketlock_t **openssl_lock; }; enum init_stage { @@ -66,6 +70,7 @@ enum init_stage { PKTIO_INIT, TIMER_INIT, CRYPTO_INIT, + OPENSSL_INIT, CLASSIFICATION_INIT, TRAFFIC_MNGR_INIT, NAME_TABLE_INIT, diff --git a/platform/linux-generic/odp_init.c b/platform/linux-generic/odp_init.c index 1b0d8f8..6873bb5 100644 --- a/platform/linux-generic/odp_init.c +++ b/platform/linux-generic/odp_init.c @@ -6,6 +6,8 @@ #include <odp/api/init.h> #include <odp_debug_internal.h> #include <odp/api/debug.h> +#include <openssl/opensslconf.h> +#include <openssl/crypto.h> #include <unistd.h> #include <odp_internal.h> #include <odp_schedule_if.h> @@ -21,8 +23,30 @@ #define _ODP_FILES_FMT "odp-%d-" #define _ODP_TMPDIR "/tmp" +static inline int openssl_numlocks(void) +{ + return CRYPTO_num_locks(); +} + struct odp_global_data_s odp_global_data; +static unsigned long openssl_thread_id(void) +{ + return (unsigned long)odp_thread_id(); +} + +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 *) + &odp_global_data.openssl_lock[n]); + else + odp_ticketlock_unlock((odp_ticketlock_t *) + &odp_global_data.openssl_lock[n]); +} + /* remove all files staring with "odp-<pid>" from a directory "dir" */ static int cleanup_files(const char *dirpath, int odp_pid) { @@ -70,6 +94,8 @@ int odp_init_global(odp_instance_t *instance, const odp_platform_init_t *platform_params ODP_UNUSED) { char *hpdir; + int nlocks = openssl_numlocks(); + int i; memset(&odp_global_data, 0, sizeof(struct odp_global_data_s)); odp_global_data.main_pid = getpid(); @@ -162,6 +188,34 @@ int odp_init_global(odp_instance_t *instance, } stage = CRYPTO_INIT; + if (nlocks > 0) { + odp_global_data.openssl_shm = + odp_shm_reserve("ODP OpenSSL Mutexes", + nlocks * sizeof(odp_ticketlock_t), + sizeof(odp_ticketlock_t), + ODP_SHM_SW_ONLY); + if (odp_global_data.openssl_shm == ODP_SHM_INVALID) { + ODP_ERR("ODP OpenSSL init reserve failed.\n"); + goto init_failed; + } + + odp_global_data.openssl_lock = + odp_shm_addr(odp_global_data.openssl_shm); + if (odp_global_data.openssl_lock == NULL) { + odp_shm_free(odp_global_data.openssl_shm); + ODP_ERR("ODP OpenSSL init addr failed.\n"); + goto init_failed; + } + + for (i = 0; i < nlocks; i++) + odp_ticketlock_init((odp_ticketlock_t *) + &odp_global_data.openssl_lock[i]); + + CRYPTO_set_id_callback(openssl_thread_id); + CRYPTO_set_locking_callback(openssl_lock); + } + stage = OPENSSL_INIT; + if (odp_classification_init_global()) { ODP_ERR("ODP classification init failed.\n"); goto init_failed; @@ -224,6 +278,15 @@ int _odp_term_global(enum init_stage stage) } /* Fall through */ + case OPENSSL_INIT: + if (odp_global_data.openssl_lock) { + CRYPTO_set_locking_callback(NULL); + CRYPTO_set_id_callback(NULL); + + odp_shm_free(odp_global_data.openssl_shm); + } + /* Fall through */ + case CRYPTO_INIT: if (odp_crypto_term_global()) { ODP_ERR("ODP crypto term failed.\n");
Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2798 by adding OpenSSL callbacks for locking that use ticketlocks to provide thread-safety for OpenSSL calls made by ODP components such as random number generation. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/include/odp_internal.h | 5 +++ platform/linux-generic/odp_init.c | 63 +++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) -- 2.7.4