Re: [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial random seed
On 2020-06-29 19:57, Dan Gora wrote:
> On Mon, Jun 29, 2020 at 6:30 AM Mattias Rönnblom
> wrote:
>> On 2020-04-23 01:42, Dan Gora wrote:
>>> The getentropy() function was introduced into glibc v2.25 and so is
>>> not available on all supported platforms. Previously, if DPDK was
>>> compiled (using meson) on a system which has getentropy(), it would
>>> introduce a dependency on glibc v2.25 which would prevent that binary
>>> from running on a system with an older glibc. Similarly if DPDK was
>>> compiled on a system which did not have getentropy(), getentropy()
>>> could not be used even if the execution system supported it.
>>>
>>> Introduce a new static function, __rte_getentropy() to emulate the
>>> glibc getentropy() function by reading from /dev/urandom to remove
>>> this dependency on the glibc version.
>>>
>>> Since __rte_genentropy() should never fail, the rdseed method is
>>> tried first.
>>>
>>> Signed-off-by: Dan Gora
>>> ---
>>>lib/librte_eal/common/rte_random.c | 62 ++
>>>lib/librte_eal/meson.build | 3 --
>>>2 files changed, 54 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/lib/librte_eal/common/rte_random.c
>>> b/lib/librte_eal/common/rte_random.c
>>> index 2c84c8527..f043adf03 100644
>>> --- a/lib/librte_eal/common/rte_random.c
>>> +++ b/lib/librte_eal/common/rte_random.c
>>> @@ -7,6 +7,7 @@
>>>#endif
>>>#include
>>>#include
>>> +#include
>>>
>>>#include
>>>#include
>>> @@ -176,20 +177,61 @@ rte_rand_max(uint64_t upper_bound)
>>>return res;
>>>}
>>>
>>> +/* Emulate glibc getentropy() using /dev/urandom */
>>> +static int
>>> +__rte_getentropy(void *buffer, size_t length)
>>> +{
>>> + uint8_t *start = buffer;
>>> + uint8_t *end;
>>> + ssize_t bytes;
>>> + int fd;
>>> + int rc = -1;
>>> +
>>> + if (length > 256) {
>>> + errno = EIO;
>>
>> First of all; only the return code is needed, so why bother with errno?
>> If you would, I suspect it should be rte_errno and not errno (which is
>> already set).
> Because, as I thought that I clearly explained in the previous email
> in this thread:
>
> https://protect2.fireeye.com/v1/url?k=64eebf70-3a4e5fe4-64eeffeb-86d2114eab2f-e9077eca0a4dd2ab&q=1&e=2360d5cd-0b70-4aa9-86f1-f72782986b27&u=https%3A%2F%2Fwww.mail-archive.com%2Fdev%40dpdk.org%2Fmsg164646.html
>
> this function is emulating the getentropy() system call. Since we
> want it to have to the same semantics as getentropy() and since
> getentropy() is a system call, it clears and sets errno, just like
> getentropy():
Since you've replaced getentropy() altogether for all builds, there's no
need to be API-compatible. Just do an as-simple-as-possible function
that reads 8 bytes from /dev/urandom.
> https://protect2.fireeye.com/v1/url?k=7d08ee94-23a80e00-7d08ae0f-86d2114eab2f-0d7c5c2b9ffa9874&q=1&e=2360d5cd-0b70-4aa9-86f1-f72782986b27&u=https%3A%2F%2Fsourceware.org%2Fgit%2F%3Fp%3Dglibc.git%3Ba%3Dblob%3Bf%3Dsysdeps%2Funix%2Fsysv%2Flinux%2Fgetentropy.c%3Bh%3D1778632ff1f1fd77019401c3fbaa164c167248b0%3Bhb%3D92dcaa3e2f7bf0f7f1c04cd2fb6a317df1a4e225
>
>>
>>> + return -1;
>>> + }
>>> +
>>> + fd = open("/dev/urandom", O_RDONLY);
>>> + if (fd < 0) {
>>> + errno = ENODEV;
>>
>> See above.
>>
>>
>>> + return -1;
>>> + }
>>> +
>>> + end = start + length;
>>> + while (start < end) {
>>> + bytes = read(fd, start, end - start);
>>> + if (bytes < 0) {
>>> + if (errno == EINTR)
>>> + /* Supposedly cannot be interrupted by
>>> + * a signal, but just in case...
>>> + */
>>> + continue;
>>> + else
>>> + goto out;
>>> + }
>>> + if (bytes == 0) {
>>> + /* no more bytes available, should not happen under
>>> + * normal circumstances.
>>> + */
>>> + errno = EIO;
>>> + goto out;
>>> + }
>>> + start += bytes;
>>> + }
>>
>> There's no need for this loop. A /dev/urandom read() is guaranteed to
>> return as many bytes as requested, up to 256 bytes. See random(4) for
>> details.
> It can't be interrupted by a signal? Are you _sure_ that it cannot
> return less than the requested number of bytes and has been that was
> forever and always? Why does getentropy() check this then? In the
> case where it does not fail this error checking makes no difference
> other than a couple extra instructions. In the case that it does, it
> saves your bacon.
The random(4) manual page says it can't be interrupted for small
requests, which seems to hold true for Linux 3.17 and later. I don't
know the hows and whys of glibc getentropy(). Studying LGPL code before
implementing BSD licensed code pe
Re: [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial random seed
On Mon, Jun 29, 2020 at 6:30 AM Mattias Rönnblom
wrote:
>
> On 2020-04-23 01:42, Dan Gora wrote:
> > The getentropy() function was introduced into glibc v2.25 and so is
> > not available on all supported platforms. Previously, if DPDK was
> > compiled (using meson) on a system which has getentropy(), it would
> > introduce a dependency on glibc v2.25 which would prevent that binary
> > from running on a system with an older glibc. Similarly if DPDK was
> > compiled on a system which did not have getentropy(), getentropy()
> > could not be used even if the execution system supported it.
> >
> > Introduce a new static function, __rte_getentropy() to emulate the
> > glibc getentropy() function by reading from /dev/urandom to remove
> > this dependency on the glibc version.
> >
> > Since __rte_genentropy() should never fail, the rdseed method is
> > tried first.
> >
> > Signed-off-by: Dan Gora
> > ---
> > lib/librte_eal/common/rte_random.c | 62 ++
> > lib/librte_eal/meson.build | 3 --
> > 2 files changed, 54 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/rte_random.c
> > b/lib/librte_eal/common/rte_random.c
> > index 2c84c8527..f043adf03 100644
> > --- a/lib/librte_eal/common/rte_random.c
> > +++ b/lib/librte_eal/common/rte_random.c
> > @@ -7,6 +7,7 @@
> > #endif
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -176,20 +177,61 @@ rte_rand_max(uint64_t upper_bound)
> > return res;
> > }
> >
> > +/* Emulate glibc getentropy() using /dev/urandom */
> > +static int
> > +__rte_getentropy(void *buffer, size_t length)
> > +{
> > + uint8_t *start = buffer;
> > + uint8_t *end;
> > + ssize_t bytes;
> > + int fd;
> > + int rc = -1;
> > +
> > + if (length > 256) {
> > + errno = EIO;
>
>
> First of all; only the return code is needed, so why bother with errno?
> If you would, I suspect it should be rte_errno and not errno (which is
> already set).
Because, as I thought that I clearly explained in the previous email
in this thread:
https://www.mail-archive.com/dev@dpdk.org/msg164646.html
this function is emulating the getentropy() system call. Since we
want it to have to the same semantics as getentropy() and since
getentropy() is a system call, it clears and sets errno, just like
getentropy():
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getentropy.c;h=1778632ff1f1fd77019401c3fbaa164c167248b0;hb=92dcaa3e2f7bf0f7f1c04cd2fb6a317df1a4e225
>
>
> > + return -1;
> > + }
> > +
> > + fd = open("/dev/urandom", O_RDONLY);
> > + if (fd < 0) {
> > + errno = ENODEV;
>
>
> See above.
>
>
> > + return -1;
> > + }
> > +
> > + end = start + length;
> > + while (start < end) {
> > + bytes = read(fd, start, end - start);
> > + if (bytes < 0) {
> > + if (errno == EINTR)
> > + /* Supposedly cannot be interrupted by
> > + * a signal, but just in case...
> > + */
> > + continue;
> > + else
> > + goto out;
> > + }
> > + if (bytes == 0) {
> > + /* no more bytes available, should not happen under
> > + * normal circumstances.
> > + */
> > + errno = EIO;
> > + goto out;
> > + }
> > + start += bytes;
> > + }
>
>
> There's no need for this loop. A /dev/urandom read() is guaranteed to
> return as many bytes as requested, up to 256 bytes. See random(4) for
> details.
It can't be interrupted by a signal? Are you _sure_ that it cannot
return less than the requested number of bytes and has been that was
forever and always? Why does getentropy() check this then? In the
case where it does not fail this error checking makes no difference
other than a couple extra instructions. In the case that it does, it
saves your bacon.
>
>
> > + rc = 0;
> > + errno = 0;
>
>
> Why are you changing errno? You should never touch errno on success.
Because getentropy() does and we are emulating getentropy() and want
to have the same semantics:
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getentropy.c;h=1778632ff1f1fd77019401c3fbaa164c167248b0;hb=92dcaa3e2f7bf0f7f1c04cd2fb6a317df1a4e225
>
>
> > +out:
> > + close(fd);
> > + return rc;
> > +}
> > +
> > static uint64_t
> > __rte_random_initial_seed(void)
> > {
> > -#ifdef RTE_LIBEAL_USE_GETENTROPY
> > - int ge_rc;
> > uint64_t ge_seed;
> >
> > - ge_rc = getentropy(&ge_seed, sizeof(ge_seed));
> > -
> > - if (ge_rc == 0)
> > - return ge_seed;
> > -#endif
> > #if defined(RTE_ARCH_X86)
> > - /* first fallback: rdseed instruction, if ava
Re: [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial random seed
On 2020-04-23 01:42, Dan Gora wrote:
> The getentropy() function was introduced into glibc v2.25 and so is
> not available on all supported platforms. Previously, if DPDK was
> compiled (using meson) on a system which has getentropy(), it would
> introduce a dependency on glibc v2.25 which would prevent that binary
> from running on a system with an older glibc. Similarly if DPDK was
> compiled on a system which did not have getentropy(), getentropy()
> could not be used even if the execution system supported it.
>
> Introduce a new static function, __rte_getentropy() to emulate the
> glibc getentropy() function by reading from /dev/urandom to remove
> this dependency on the glibc version.
>
> Since __rte_genentropy() should never fail, the rdseed method is
> tried first.
>
> Signed-off-by: Dan Gora
> ---
> lib/librte_eal/common/rte_random.c | 62 ++
> lib/librte_eal/meson.build | 3 --
> 2 files changed, 54 insertions(+), 11 deletions(-)
>
> diff --git a/lib/librte_eal/common/rte_random.c
> b/lib/librte_eal/common/rte_random.c
> index 2c84c8527..f043adf03 100644
> --- a/lib/librte_eal/common/rte_random.c
> +++ b/lib/librte_eal/common/rte_random.c
> @@ -7,6 +7,7 @@
> #endif
> #include
> #include
> +#include
>
> #include
> #include
> @@ -176,20 +177,61 @@ rte_rand_max(uint64_t upper_bound)
> return res;
> }
>
> +/* Emulate glibc getentropy() using /dev/urandom */
> +static int
> +__rte_getentropy(void *buffer, size_t length)
> +{
> + uint8_t *start = buffer;
> + uint8_t *end;
> + ssize_t bytes;
> + int fd;
> + int rc = -1;
> +
> + if (length > 256) {
> + errno = EIO;
First of all; only the return code is needed, so why bother with errno?
If you would, I suspect it should be rte_errno and not errno (which is
already set).
> + return -1;
> + }
> +
> + fd = open("/dev/urandom", O_RDONLY);
> + if (fd < 0) {
> + errno = ENODEV;
See above.
> + return -1;
> + }
> +
> + end = start + length;
> + while (start < end) {
> + bytes = read(fd, start, end - start);
> + if (bytes < 0) {
> + if (errno == EINTR)
> + /* Supposedly cannot be interrupted by
> + * a signal, but just in case...
> + */
> + continue;
> + else
> + goto out;
> + }
> + if (bytes == 0) {
> + /* no more bytes available, should not happen under
> + * normal circumstances.
> + */
> + errno = EIO;
> + goto out;
> + }
> + start += bytes;
> + }
There's no need for this loop. A /dev/urandom read() is guaranteed to
return as many bytes as requested, up to 256 bytes. See random(4) for
details.
> + rc = 0;
> + errno = 0;
Why are you changing errno? You should never touch errno on success.
> +out:
> + close(fd);
> + return rc;
> +}
> +
> static uint64_t
> __rte_random_initial_seed(void)
> {
> -#ifdef RTE_LIBEAL_USE_GETENTROPY
> - int ge_rc;
> uint64_t ge_seed;
>
> - ge_rc = getentropy(&ge_seed, sizeof(ge_seed));
> -
> - if (ge_rc == 0)
> - return ge_seed;
> -#endif
> #if defined(RTE_ARCH_X86)
> - /* first fallback: rdseed instruction, if available */
> if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_RDSEED)) {
> unsigned int rdseed_low;
> unsigned int rdseed_high;
> @@ -200,6 +242,10 @@ __rte_random_initial_seed(void)
> ((uint64_t)rdseed_high << 32);
> }
> #endif
> + /* first fallback: read from /dev/urandom.. */
Remove "..".
> + if (__rte_getentropy(&ge_seed, sizeof(ge_seed)) == 0)
> + return ge_seed;
> +
> /* second fallback: seed using rdtsc */
> return rte_get_tsc_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 ')
> - cflags += '-DRTE_LIBEAL_USE_GETENTROPY'
> -endif
> if cc.has_header('getopt.h')
> cflags += ['-DHAVE_GETOPT_H', '-DHAVE_GETOPT', '-DHAVE_GETOPT_LONG']
> endif
Re: [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial random seed
On Wed, Apr 22, 2020 at 11:39 PM Stephen Hemminger
wrote:
>
> On Wed, 22 Apr 2020 20:42:54 -0300
> Dan Gora wrote:
>
> > + fd = open("/dev/urandom", O_RDONLY);
> > + if (fd < 0) {
> > + errno = ENODEV;
> > + return -1;
> > + }
> > +
> > + end = start + length;
> > + while (start < end) {
> > + bytes = read(fd, start, end - start);
> > + if (bytes < 0) {
>
> You are overdoing the complexity here. More error handling is not better.
I've definitely never heard that expression before!
> 1. This should only be called once at startup EINTR is not an issue then
> 2. The amount requested is always returned when using urandom (see man page
> for random(4))
>
>The O_NONBLOCK flag has no effect when opening /dev/urandom. When
> calling
>read(2) for the device /dev/urandom, reads of up to 256 bytes will
> return as
>many bytes as are requested and will not be interrupted by a signal
> handler.
>Reads with a buffer over this limit may return less than the requested
> number
>of bytes or fail with the error EINTR, if interrupted by a signal
> handler.
I didn't just make this up out of whole cloth... This code was lifted,
almost verbatim, from the glibc implementation of getentropy(), which
is the function that we are trying to emulate:
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getentropy.c;h=1778632ff1f1fd77019401c3fbaa164c167248b0;hb=92dcaa3e2f7bf0f7f1c04cd2fb6a317df1a4e225
I assumed that they added this error handling for a reason.
Yes, since this function is only called once at startup EINTR should
not be an issue, but if we need to add __rte_getentropy() as a
generic, exported interface later, that error case would already be
taken care of.
thanks
dan
Re: [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial random seed
On Wed, 22 Apr 2020 20:42:54 -0300
Dan Gora wrote:
> + fd = open("/dev/urandom", O_RDONLY);
> + if (fd < 0) {
> + errno = ENODEV;
> + return -1;
> + }
> +
> + end = start + length;
> + while (start < end) {
> + bytes = read(fd, start, end - start);
> + if (bytes < 0) {
You are overdoing the complexity here. More error handling is not better.
1. This should only be called once at startup EINTR is not an issue then
2. The amount requested is always returned when using urandom (see man page for
random(4))
The O_NONBLOCK flag has no effect when opening /dev/urandom. When
calling
read(2) for the device /dev/urandom, reads of up to 256 bytes will
return as
many bytes as are requested and will not be interrupted by a signal
handler.
Reads with a buffer over this limit may return less than the requested
number
of bytes or fail with the error EINTR, if interrupted by a signal
handler.
