[v2] eal: choose initial PRNG seed source at runtime

  Instead of choosing to use getentropy() or the rdseed instruction for
the random number generator entropy source using compilation flags,
determine the best source at run time.

This is accomplished by defining a weak symbol for getentropy(),
checking that the compiler can generate the rdseed instruction even if
the compilation platform does not natively support it, and checking for
the availability of the rdseed instruction on the execution platform
using rte_cpu_get_flag_enabled().

If neither getentropy() or rdseed is available, rte_get_timer_cycles()
will be continue to be used as the entropy source.

This also allows non-meson builds to use getentropy().

Signed-off-by: Dan Gora <dg@adax.com>
---
v2:
* Rebase to latest master.
* Fix spelling of "meson".

 config/x86/meson.build             |  7 +++++++
 lib/librte_eal/common/rte_random.c | 29 ++++++++++++++++++++++++-----
 lib/librte_eal/meson.build         |  3 ---
 mk/rte.cpuflags.mk                 |  8 ++++++++
 4 files changed, 39 insertions(+), 8 deletions(-)
  

On 2020-04-16 01:59, Dan Gora wrote:
> Instead of choosing to use getentropy() or the rdseed instruction for
> the random number generator entropy source using compilation flags,
> determine the best source at run time.


I like this idea.


> This is accomplished by defining a weak symbol for getentropy(),
> checking that the compiler can generate the rdseed instruction even if
> the compilation platform does not natively support it, and checking for
> the availability of the rdseed instruction on the execution platform
> using rte_cpu_get_flag_enabled().
>
> If neither getentropy() or rdseed is available, rte_get_timer_cycles()
> will be continue to be used as the entropy source.
>
> This also allows non-meson builds to use getentropy().
>
> Signed-off-by: Dan Gora <dg@adax.com>
> ---
> v2:
> * Rebase to latest master.
> * Fix spelling of "meson".
>
>   config/x86/meson.build             |  7 +++++++
>   lib/librte_eal/common/rte_random.c | 29 ++++++++++++++++++++++++-----
>   lib/librte_eal/meson.build         |  3 ---
>   mk/rte.cpuflags.mk                 |  8 ++++++++
>   4 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/config/x86/meson.build b/config/x86/meson.build
> index adc857ba2..214b16f2a 100644
> --- a/config/x86/meson.build
> +++ b/config/x86/meson.build
> @@ -20,6 +20,13 @@ if cc.get_define('__SSE4_2__', args: machine_args) == ''
>   	machine_args += '-msse4'
>   endif
>   
> +# set -mrdseed if necessary so _rdseed32_step compiles if the
> +# compilation host does not support the RDSEED instruction.
> +if cc.get_define('__RDSEED__', args: machine_args) == '' and cc.has_argument('-mrdseed')
> +	machine_args += '-mrdseed'
> +	message('RDSEED not enabled by default, explicitly setting -mrdseed')
> +endif
> +
>   base_flags = ['SSE', 'SSE2', 'SSE3','SSSE3', 'SSE4_1', 'SSE4_2']
>   foreach f:base_flags
>   	dpdk_conf.set('RTE_MACHINE_CPUFLAG_' + f, 1)
> diff --git a/lib/librte_eal/common/rte_random.c b/lib/librte_eal/common/rte_random.c
> index 57ec8fb2b..40f8b5aab 100644
> --- a/lib/librte_eal/common/rte_random.c
> +++ b/lib/librte_eal/common/rte_random.c
> @@ -25,6 +25,8 @@ struct rte_rand_state {
>   
>   static struct rte_rand_state rand_states[RTE_MAX_LCORE];
>   
> +__rte_weak int getentropy(void *__buffer, size_t __length);
> +
>   static uint32_t
>   __rte_rand_lcg32(uint32_t *seed)
>   {
> @@ -176,10 +178,24 @@ rte_rand_max(uint64_t upper_bound)
>   	return res;
>   }
>   
> +/* Use rte_get_timer_cycles() if the system does not have
> + * genentropy() or the rdseed instruction.
> + */
> +__rte_weak int
> +getentropy(void *__buffer, size_t __length __rte_unused)
> +{


Just make the weak getentropy() to return -1, always.


> +	uint64_t *ge_seed = __buffer;
> +#ifdef RTE_MACHINE_CPUFLAG_RDSEED
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_RDSEED))
> +		return -1;
> +#endif
> +	*ge_seed = rte_get_timer_cycles();
> +	return 0;
> +}
> +
>   static uint64_t
>   __rte_random_initial_seed(void)
>   {
> -#ifdef RTE_LIBEAL_USE_GETENTROPY
>   	int ge_rc;
>   	uint64_t ge_seed;
>   
> @@ -187,15 +203,18 @@ __rte_random_initial_seed(void)
>   
>   	if (ge_rc == 0)
>   		return ge_seed;
> -#endif
> +
>   #ifdef RTE_MACHINE_CPUFLAG_RDSEED
>   	unsigned int rdseed_low;
>   	unsigned int rdseed_high;
>   
>   	/* first fallback: rdseed instruction, if available */
> -	if (_rdseed32_step(&rdseed_low) == 1 &&
> -	    _rdseed32_step(&rdseed_high) == 1)
> -		return (uint64_t)rdseed_low | ((uint64_t)rdseed_high << 32);
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_RDSEED)) {


Move the variable declarations into this scope.


> +		if (_rdseed32_step(&rdseed_low) == 1 &&
> +		    _rdseed32_step(&rdseed_high) == 1)
> +			return (uint64_t)rdseed_low |
> +				((uint64_t)rdseed_high << 32);
> +	}
>   #endif
>   	/* second fallback: seed using rdtsc */
>   	return rte_get_timer_cycles();
> diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
> index 0267c3b9d..748359b8c 100644
> --- a/lib/librte_eal/meson.build
> +++ b/lib/librte_eal/meson.build
> @@ -15,9 +15,6 @@ deps += 'kvargs'
>   if dpdk_conf.has('RTE_USE_LIBBSD')
>   	ext_deps += libbsd
>   endif
> -if cc.has_function('getentropy', prefix : '#include <unistd.h>')
> -	cflags += '-DRTE_LIBEAL_USE_GETENTROPY'
> -endif
>   if cc.has_header('getopt.h')
>   	cflags += ['-DHAVE_GETOPT_H', '-DHAVE_GETOPT', '-DHAVE_GETOPT_LONG']
>   endif
> diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk
> index fa8753531..fb7bf8a53 100644
> --- a/mk/rte.cpuflags.mk
> +++ b/mk/rte.cpuflags.mk
> @@ -53,6 +53,14 @@ endif
>   
>   ifneq ($(filter $(AUTO_CPUFLAGS),__RDSEED__),)
>   CPUFLAGS += RDSEED
> +else
> +# If the native environment doesn't define __RDSEED__, see
> +# if the compiler supports -mrdseed.


For which environments is this true?


> +RDSEED_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(WERROR_FLAGS) $(EXTRA_CFLAGS) -mrdseed -dM -E - < /dev/null)
> +ifneq ($(filter $(RDSEED_CPUFLAGS),__RDSEED__),)


There are no better ways to achieve this? It seems like a bit of a hack.


> +CPUFLAGS += RDSEED
> +MACHINE_CFLAGS += -mrdseed
> +endif
>   endif
>   
>   ifneq ($(filter $(AUTO_CPUFLAGS),__FSGSBASE__),)
  

On Thu, Apr 16, 2020 at 8:30 AM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
> > diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
> > index 0267c3b9d..748359b8c 100644
> > --- a/lib/librte_eal/meson.build
> > +++ b/lib/librte_eal/meson.build
> > @@ -15,9 +15,6 @@ deps += 'kvargs'
> >   if dpdk_conf.has('RTE_USE_LIBBSD')
> >       ext_deps += libbsd
> >   endif
> > -if cc.has_function('getentropy', prefix : '#include <unistd.h>')
> > -     cflags += '-DRTE_LIBEAL_USE_GETENTROPY'
> > -endif
> >   if cc.has_header('getopt.h')
> >       cflags += ['-DHAVE_GETOPT_H', '-DHAVE_GETOPT', '-DHAVE_GETOPT_LONG']
> >   endif
> > diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk
> > index fa8753531..fb7bf8a53 100644
> > --- a/mk/rte.cpuflags.mk
> > +++ b/mk/rte.cpuflags.mk
> > @@ -53,6 +53,14 @@ endif
> >
> >   ifneq ($(filter $(AUTO_CPUFLAGS),__RDSEED__),)
> >   CPUFLAGS += RDSEED
> > +else
> > +# If the native environment doesn't define __RDSEED__, see
> > +# if the compiler supports -mrdseed.
>
>
> For which environments is this true?

If you compile on a machine which does not have the RDSEED cpu flag,
gcc will not define __RDSEED__ nor compile the _rdseed32_step()
functions unless you explicitly set -mrdseed on the gcc/clang command
line..

We have a HP DL380 machine which do not have rdseed:
model name      : Intel(R) Xeon(R) CPU E5-2630 v3 @ 2.40GHz

And another which does:
model name      : Intel(R) Xeon(R) CPU E5-2640 v4 @ 2.40GHz

_rdseed32_step() will only compile on the v3 if we explicitly set -mrdseed.

> > +RDSEED_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(WERROR_FLAGS) $(EXTRA_CFLAGS) -mrdseed -dM -E - < /dev/null)
> > +ifneq ($(filter $(RDSEED_CPUFLAGS),__RDSEED__),)
>
>
> There are no better ways to achieve this? It seems like a bit of a hack.

eh.. I don't know of any really.. It's the same command that is used
to set AUTO_CPUFLAGS a few lines before, just with -mrdseed set.  It
doesn't seem that bad to my mind, but if there is a better way, I'm
open to suggestions.

That said, this patch does not work.  We cannot set
RTE_MACHINE_CPUFLAG_RDSEED because rte_eal_init() will fail on a
machine which does not have rdseed because rte_cpu_is_supported()
checks all of these cpu flags set at compile time.  Since we're
detecting this flag at run time, we have to remove it from this list
of "required" CPU flags.

I'm working on a new patch.. I should have something tomorrow.

There is also still a problem with getting getentropy() to work when
the binary is compiled on a system with a glibc < 2.25 but run on a
system with glibc >= 2.25.  It doesn't resolve the weak symbol to the
glibc version.. I think that it's because getentropy() is versioned
within glibc.  I'm still working on how to fix this.. It might come
down to using dlopen(), dlsym()... :(

I'm not really sure if it's worth it.

thanks
dan
  

On 2020-04-17 01:50, Dan Gora wrote:
> On Thu, Apr 16, 2020 at 8:30 AM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>>> diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
>>> index 0267c3b9d..748359b8c 100644
>>> --- a/lib/librte_eal/meson.build
>>> +++ b/lib/librte_eal/meson.build
>>> @@ -15,9 +15,6 @@ deps += 'kvargs'
>>>    if dpdk_conf.has('RTE_USE_LIBBSD')
>>>        ext_deps += libbsd
>>>    endif
>>> -if cc.has_function('getentropy', prefix : '#include <unistd.h>')
>>> -     cflags += '-DRTE_LIBEAL_USE_GETENTROPY'
>>> -endif
>>>    if cc.has_header('getopt.h')
>>>        cflags += ['-DHAVE_GETOPT_H', '-DHAVE_GETOPT', '-DHAVE_GETOPT_LONG']
>>>    endif
>>> diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk
>>> index fa8753531..fb7bf8a53 100644
>>> --- a/mk/rte.cpuflags.mk
>>> +++ b/mk/rte.cpuflags.mk
>>> @@ -53,6 +53,14 @@ endif
>>>
>>>    ifneq ($(filter $(AUTO_CPUFLAGS),__RDSEED__),)
>>>    CPUFLAGS += RDSEED
>>> +else
>>> +# If the native environment doesn't define __RDSEED__, see
>>> +# if the compiler supports -mrdseed.
>>
>> For which environments is this true?
> If you compile on a machine which does not have the RDSEED cpu flag,
> gcc will not define __RDSEED__ nor compile the _rdseed32_step()
> functions unless you explicitly set -mrdseed on the gcc/clang command
> line..
>
> We have a HP DL380 machine which do not have rdseed:
> model name      : Intel(R) Xeon(R) CPU E5-2630 v3 @ 2.40GHz
>
> And another which does:
> model name      : Intel(R) Xeon(R) CPU E5-2640 v4 @ 2.40GHz
>
> _rdseed32_step() will only compile on the v3 if we explicitly set -mrdseed.
>
>>> +RDSEED_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(WERROR_FLAGS) $(EXTRA_CFLAGS) -mrdseed -dM -E - < /dev/null)
>>> +ifneq ($(filter $(RDSEED_CPUFLAGS),__RDSEED__),)
>>
>> There are no better ways to achieve this? It seems like a bit of a hack.
> eh.. I don't know of any really.. It's the same command that is used
> to set AUTO_CPUFLAGS a few lines before, just with -mrdseed set.  It
> doesn't seem that bad to my mind, but if there is a better way, I'm
> open to suggestions.
>
> That said, this patch does not work.  We cannot set
> RTE_MACHINE_CPUFLAG_RDSEED because rte_eal_init() will fail on a
> machine which does not have rdseed because rte_cpu_is_supported()
> checks all of these cpu flags set at compile time.  Since we're
> detecting this flag at run time, we have to remove it from this list
> of "required" CPU flags.
>
> I'm working on a new patch.. I should have something tomorrow.
>
> There is also still a problem with getting getentropy() to work when
> the binary is compiled on a system with a glibc < 2.25 but run on a
> system with glibc >= 2.25.  It doesn't resolve the weak symbol to the
> glibc version.. I think that it's because getentropy() is versioned
> within glibc.  I'm still working on how to fix this.. It might come
> down to using dlopen(), dlsym()... :(


One way would be to include a getrandom syscall wrapper, instead of 
going via libc, in case libc is too old.


> I'm not really sure if it's worth it.


I personally don't think having high-quality initial, automatic, seeding 
is important-enough to warrant much complexity.


> thanks
> dan