| Message ID | 1483710822-15237-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
This bug fix belongs to master (and monarch), especially if there are no conflicts with the changes in api-next. > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Bill > Fischofer > Sent: Friday, January 06, 2017 3:54 PM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [API-NEXT PATCHv2] linux-generic: init: add openssl > locking support for thread safety > > 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> > --- > Changes for v2: > - OPENSSL_INIT stage should precede CRYPTO_INIT stage since crypto uses > OpenSSL OpenSSL init should be part of crypto init. > > 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..272c7cd 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; These do not belong here, but as part of crypto global data struct in odp_crypto.c. struct odp_crypto_global_s { odp_spinlock_t lock; << MOVE HERE. Use struct for explicit grouping of variables. >> /* OpenSSL globals */ struct { odp_shm_t shm; odp_ticketlock_t **lock; }openssl; odp_crypto_generic_session_t *free; odp_crypto_generic_session_t sessions[0]; }; > }; > > enum init_stage { > @@ -65,6 +69,7 @@ enum init_stage { > SCHED_INIT, > PKTIO_INIT, > TIMER_INIT, > + OPENSSL_INIT, > CRYPTO_INIT, > CLASSIFICATION_INIT, > TRAFFIC_MNGR_INIT, > diff --git a/platform/linux-generic/odp_init.c b/platform/linux- > generic/odp_init.c > index 1b0d8f8..7474ff0 100644 > --- a/platform/linux-generic/odp_init.c > +++ b/platform/linux-generic/odp_init.c OpenSSL dependency does not belong to init.c but into crypto.c. Move all code below into odp_crypto.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(); > +} I'm OK with these callbacks (since we use int as thread ID), but OpenSSL documentation says that those are deprecated by CRYPTO_THREADID_xxx functions. Should we use the simple thing that is already deprecated, or the new, more "advanced" interface ? "CRYPTO_THREADID and associated functions were introduced in OpenSSL 1.0.0 to replace (actually, deprecate) the previous CRYPTO_set_id_callback(), CRYPTO_get_id_callback(), and CRYPTO_thread_id() functions which assumed thread IDs to always be represented by 'unsigned long'." > + > +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]); > +} Modern OpenSSL lib may pass also CRYPTO_READ/CRYPTO_WRITE flags. We could improve performance with RW locks later on (if lib uses RW lock/unlock most of the time). CRYPTO_LOCK 0x01 CRYPTO_UNLOCK 0x02 CRYPTO_READ 0x04 CRYPTO_WRITE 0x08 -Petri
On Mon, Jan 9, 2017 at 2:37 AM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: > > This bug fix belongs to master (and monarch), especially if there are no conflicts with the changes in api-next. > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Bill >> Fischofer >> Sent: Friday, January 06, 2017 3:54 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [API-NEXT PATCHv2] linux-generic: init: add openssl >> locking support for thread safety >> >> 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> >> --- >> Changes for v2: >> - OPENSSL_INIT stage should precede CRYPTO_INIT stage since crypto uses >> OpenSSL > > > OpenSSL init should be part of crypto init. > > >> >> 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..272c7cd 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; > > > These do not belong here, but as part of crypto global data struct in odp_crypto.c. > > > struct odp_crypto_global_s { > odp_spinlock_t lock; > > << MOVE HERE. Use struct for explicit grouping of variables. >> > /* OpenSSL globals */ > struct { > odp_shm_t shm; > odp_ticketlock_t **lock; > }openssl; > > odp_crypto_generic_session_t *free; > odp_crypto_generic_session_t sessions[0]; > > }; > > > > > >> }; >> >> enum init_stage { >> @@ -65,6 +69,7 @@ enum init_stage { >> SCHED_INIT, >> PKTIO_INIT, >> TIMER_INIT, >> + OPENSSL_INIT, >> CRYPTO_INIT, >> CLASSIFICATION_INIT, >> TRAFFIC_MNGR_INIT, >> diff --git a/platform/linux-generic/odp_init.c b/platform/linux- >> generic/odp_init.c >> index 1b0d8f8..7474ff0 100644 >> --- a/platform/linux-generic/odp_init.c >> +++ b/platform/linux-generic/odp_init.c > > > OpenSSL dependency does not belong to init.c but into crypto.c. Move all code below into odp_crypto.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(); >> +} > > > I'm OK with these callbacks (since we use int as thread ID), but OpenSSL documentation says that those are deprecated by CRYPTO_THREADID_xxx functions. Should we use the simple thing that is already deprecated, or the new, more "advanced" interface ? > > "CRYPTO_THREADID and associated functions were introduced in OpenSSL 1.0.0 to replace (actually, deprecate) the previous CRYPTO_set_id_callback(), CRYPTO_get_id_callback(), and CRYPTO_thread_id() functions which assumed thread IDs to always be represented by 'unsigned long'." These are current as of OpenSSL 1.0.2g, which is what is distributed in Ubuntu 16.10. Starting with OpenSSL 1.1.0, all of this is deprecated in a backwards-compatible manner (these calls become no-ops) as OpenSSL itself is thread-safe. So these are the proper changes to support all current and future versions of OpenSSL. > > >> + >> +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]); >> +} > > Modern OpenSSL lib may pass also CRYPTO_READ/CRYPTO_WRITE flags. We could improve performance with RW locks later on (if lib uses RW lock/unlock most of the time). > > CRYPTO_LOCK 0x01 > CRYPTO_UNLOCK 0x02 > CRYPTO_READ 0x04 > CRYPTO_WRITE 0x08 > > -Petri > >
diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h index b313b1f..272c7cd 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 { @@ -65,6 +69,7 @@ enum init_stage { SCHED_INIT, PKTIO_INIT, TIMER_INIT, + OPENSSL_INIT, CRYPTO_INIT, CLASSIFICATION_INIT, TRAFFIC_MNGR_INIT, diff --git a/platform/linux-generic/odp_init.c b/platform/linux-generic/odp_init.c index 1b0d8f8..7474ff0 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(); @@ -156,6 +182,34 @@ int odp_init_global(odp_instance_t *instance, } stage = TIMER_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_crypto_init_global()) { ODP_ERR("ODP crypto init failed.\n"); goto init_failed; @@ -231,6 +285,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 TIMER_INIT: if (odp_timer_term_global()) { ODP_ERR("ODP timer 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> --- Changes for v2: - OPENSSL_INIT stage should precede CRYPTO_INIT stage since crypto uses OpenSSL platform/linux-generic/include/odp_internal.h | 5 +++ platform/linux-generic/odp_init.c | 63 +++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) -- 2.7.4