Re: [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial random seed

[Dan Gora <dg@adax.com>]

On Mon, Jun 29, 2020 at 6:30 AM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> 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 <dg@adax.com>
> > ---
> >   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 <stdlib.h>
> >   #include <unistd.h>
> > +#include <fcntl.h>
> >
> >   #include <rte_branch_prediction.h>
> >   #include <rte_cycles.h>
> > @@ -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 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 "..".

*sigh*.....

thanks

dan