DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dan Gora <dg@adax.com>
To: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	David Marchand <david.marchand@redhat.com>,
	 Jerin Jacob <jerinjacobk@gmail.com>
Subject: Re: [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial random seed
Date: Mon, 29 Jun 2020 14:57:35 -0300
Message-ID: <CAGyogRYQ_MJ6ncs=OLekHJ3Wyb7zFSd1uP8iq8w3Qo+Kh7pdcw@mail.gmail.com> (raw)
In-Reply-To: <aae985dc-ad3a-9a63-1c87-120afe56e12b@ericsson.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

  reply	other threads:[~2020-06-29 17:58 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-21 19:54 [dpdk-dev] [PATCH 0/2] eal: choose initial PRNG seed source at runtime Dan Gora
2020-04-21 19:54 ` [dpdk-dev] [PATCH 1/2] eal: check for rdseed at run time for random seed Dan Gora
2020-04-22  8:22   ` Mattias Rönnblom
2020-04-21 19:54 ` [dpdk-dev] [PATCH 2/2] eal: resolve getentropy " Dan Gora
2020-04-21 21:03   ` Stephen Hemminger
2020-04-21 21:08     ` Dan Gora
2020-04-22  8:28   ` Mattias Rönnblom
2020-04-22 17:44     ` Dan Gora
2020-04-22 20:14       ` Mattias Rönnblom
2020-04-22 20:35         ` Dan Gora
2020-04-23 10:04           ` Luca Boccassi
2020-04-23 17:38             ` Dan Gora
2020-04-27 12:44               ` Luca Boccassi
2020-04-27 16:57                 ` Dan Gora
2020-04-30  8:41                   ` Luca Boccassi
2020-04-30 20:43                     ` Dan Gora
2020-05-01 10:33                       ` Luca Boccassi
2020-05-01 21:05                         ` Dan Gora
2020-05-04  8:04                           ` Mattias Rönnblom
2020-05-04 14:13                             ` Dan Gora
2020-05-04 14:19                               ` Dan Gora
2020-06-02  5:10                                 ` Dan Gora
2020-06-09 15:37                                   ` Dan Gora
2020-06-10  8:15                                     ` Thomas Monjalon
2020-06-10  8:33                                       ` Luca Boccassi
2020-06-10  8:07                               ` Thomas Monjalon
2020-04-23 12:36           ` Mattias Rönnblom
2020-04-23 17:27             ` Dan Gora
2020-04-21 20:41 ` [dpdk-dev] [PATCH v2 0/2] eal: choose initial PRNG seed source at runtime Dan Gora
2020-04-21 20:41   ` [dpdk-dev] [PATCH v2 1/2] eal: check for rdseed at run time for random seed Dan Gora
2020-04-21 20:41   ` [dpdk-dev] [PATCH v2 2/2] eal: resolve getentropy " Dan Gora
2020-04-22 18:15 ` [dpdk-dev] [PATCH v3 0/2] eal: choose initial PRNG seed source at runtime Dan Gora
2020-04-22 18:15   ` [dpdk-dev] [PATCH v3 1/2] eal: check for rdseed at run time for random seed Dan Gora
2020-04-22 18:15   ` [dpdk-dev] [PATCH v3 2/2] eal: resolve getentropy " Dan Gora
2020-04-22 23:42 ` [dpdk-dev] [PATCH v4 0/2] eal: choose initial PRNG seed source at runtime Dan Gora
2020-04-22 23:42   ` [dpdk-dev] [PATCH v4 1/2] eal: check for rdseed at run time for random seed Dan Gora
2020-04-22 23:42   ` [dpdk-dev] [PATCH v4 2/2] eal: emulate glibc getentropy for initial " Dan Gora
2020-04-23  2:39     ` Stephen Hemminger
2020-04-23 17:42       ` Dan Gora
2020-06-29  9:30     ` Mattias Rönnblom
2020-06-29 17:57       ` Dan Gora [this message]
2020-06-29 20:57         ` Mattias Rönnblom
2020-06-29  9:32   ` [dpdk-dev] [PATCH v4 0/2] eal: choose initial PRNG seed source at runtime Mattias Rönnblom
2020-06-29 18:01     ` Dan Gora
2020-06-29 18:04       ` Dan Gora
2020-06-29 21:05       ` Mattias Rönnblom
2020-06-29 21:14         ` Dan Gora

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGyogRYQ_MJ6ncs=OLekHJ3Wyb7zFSd1uP8iq8w3Qo+Kh7pdcw@mail.gmail.com' \
    --to=dg@adax.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=jerinjacobk@gmail.com \
    --cc=mattias.ronnblom@ericsson.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git