DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dan Gora <dg@adax.com>
To: Luca Boccassi <bluca@debian.org>
Cc: "Mattias Rönnblom" <mattias.ronnblom@ericsson.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"David Marchand" <david.marchand@redhat.com>,
	"Jerin Jacob" <jerinjacobk@gmail.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] eal: resolve getentropy at run time for random seed
Date: Fri, 1 May 2020 18:05:40 -0300	[thread overview]
Message-ID: <CAGyogRbpUcffHwKUY83KqMm9CuSxrMD9_JXqHDy=-ppuQTbQBg@mail.gmail.com> (raw)
In-Reply-To: <fa63d69c5fe16b7a4f43e7d0c8ca07fc6d9068c3.camel@debian.org>

On Fri, May 1, 2020 at 1:29 PM Luca Boccassi <bluca@debian.org> wrote:
> >
> > Well, no, because rdseed is used first if available and /dev/urandom
> > is used next..
> >
> > And this is not a corner case at all.. There are lots of linux
> > distributions which DPDK claims to support which do not support
> > getentropy().  This is hardly a non-standard build system.  You really
> > compile and support a separate binary for every possible system that
> > your customers will use?
>
> Yes, that's how Linux distributions work. At the very least, most
> deployments ensure dependencies are not downgraded, as that's rarely
> supported, for very good reasons.

I can assure you that most commercial companies do not want to have to
support building a separate binary of their product for every since
version of every single linux distribution.  And again no dependencies
are being "downgraded" here.  The binary will use the _best_ method
available on the target system, not the least common denominator of
the build system as it does now.

I don't really consider that using '/dev/urandom' is a "downgrade"
from getentropy().  getentropy() was only introduced in the last
couple of years, so it's not like it was some super important new
method of getting random numbers, it's just a more convenient method.
Under the hood, I would guess both methods are exactly the same and
will give you the same quality of random number.  It's just the API
interface which has been improved.  And I don't see /dev/urandom going
away any time soon.

Again, if you're still hung up on using /dev/urandom we can use v3 of
my patch to search for the getentropy()  symbol.

> > And what is this about third parties?  Last I checked DPDK was an API
> > framework, not a proprietary standalone application.  It is designed,
> > by definition, to be used by "third parties".  Or does it only have to
> > work in Intel's toolchains?
>
> Third party was the wrong expression. DPDK is a set of libraries with a
> build system (two actually) which links them against their
> dependencies. It is the reponsibility of who does deployment to make
> sure those dependencies, as established at build time, are satisfied at
> runtime. Up until now, downgrading at runtime vs build time was not
> supported as far as I know,

Of course it is.. That why the rte_cpu_get_flag_enabled() function
exists, so that the code can choose the best function at run-time, not
at build time.  There are numerous examples of this in DPDK already.

> so what you are effectively asking is to
> double the size of the support matrix, and for the project to ensure
> that every single dependency can always be built against a new version
> and used against an older one.

No, this is not correct.  The size of the support/test matrix is
exactly the same, what changes is the number of build systems that
have to be supported.

Before my patches you had to test targets with:

no rdseed and no getentropy
rdseed and no getentropy
no rdseed and getentropy
rdseed and getentropy.

For each of these test cases you had to have a separate build system
to generate a different binary (let's say app/test).

Oh and you also had to test with building with make and building with
meson because getentropy was only supported with meson.

Now, with my patches, you can build 'app/test' on *any* of these
systems, and use make _or_ meson, and it will run exactly the same
way as if you compiled it on the matching test system.

That is the difference.

Let's take another example:  Say I have an application which I have to
support on RHEL 7.7 and 7.5.  7.7 has getentropy, 7.5 does not (ignore
the rdseed case for now).

So I am faced with:
1) Build on the RHEL 7.7 system.  This allows the app to use
getentropy(), but it  cannot run on 7.5.
2) Build on the RHEL 7.5 system.  This allows the app to run on 7.5
and 7.7, but only uses the TSC counter.
3) Build on both systems.  Ok, now I have to build and support two
separate binaries.
4) Determine if  getentropy() exists at run time or use /dev/urandom.
Now I can build on either system and it will use getentropy() on 7.7
and the TSC counter on 7.5.

How is #3 "better" than #4?  You still have to test on 7.7 and 7.5.
The amount of work in testing has not changed at all, only the number
of build systems and binary versions that you have to maintain.

And I'm not saying that this has to be a hard and fast rule for
everything for now and forever.  I am fixing a small function which is
used exactly once.  The original patch which introduced getentropy()
in v19.08 created an extra dependency that the target system have
glibc 2.25, _even if it does not use this function at all_.

That said, it seems to me to be a good idea to remove as many build
time dependencies as possible and use things like
rte_cpu_get_flag_enabled() to determine the best code path at run time
(that is, initialization time, not in the fastpath obviously).  It
just makes life a lot easier... If the compiler can generate the code,
then it should be possible in many cases.

> > > > > This is especially true when dealing with RNG APIs, where the tiniest
> > > > > and most innocent-looking mistake could have disastrous consequences.
> > > >
> > > > This does not change the functionality of the RNG at all.  It just
> > > > makes it work in the way that it was intended.  These changes were
> > > > only introduced into 19.08, so they are not historical artifacts or
> > > > anything.
> > >
> > > It's reimplementing a syscall using a different interface which has
> > > different semantics. A small mistake there is going to cost us dearly.
> >
> > The code was copied almost verbatim from the getentropy() function in
> > glibc, it's hard to see it going that wrong there.  The code can be
> > tested using the same test cases that the original patch used.  I
> > don't see how there is a difference in test coverage here.
>
> And who's going to keep it updated as the implementation in glibc
> changes, if/when a bug is found there?

What happens when any bug is found in DPDK... It gets fixed.  Why is
this any different?

> Also, glibc is LGPL2.1 licensed, which means copying almost verbatim a
> non-trivial piece of code can possibly result in librte_eal as a whole
> to be covered under the terms of LGPL2.1 (IANAL disclaimer). This is
> unlikely to be well received, given it's BSD-3-clause now.

Well, verbatim, is an exaggeration and IANAL either, but I would
consider what was written to be general enough to not really run into
any problems.  If others disagree, the v3 patch is there waiting too..

> > If it's such a big deal to use /dev/urandom, then what about my v3
> > patch which used dlopen()/dlsym() to try to find getentropy()?  Is
> > that not then acceptable?   dlopen/dlsym() are used in several placed
> > in DPDK.
>
> I don't have a problem with that specifically, but it needs to be clear
> to the maintainers that what you are effectively asking is for all
> dependencies to support runtime-downgrading transparently and out of
> the box.

Again, I am not asking for all build time dependencies to be
eliminated, just this one, which was only introduced in v19.08, which
creates a dependency on glibc 2.25, even if the application does not
use the RNG at all.

This is already done in numerous places in the DPDK. See
aesni_gcm_create().  See rte_hash_crc_set_alg().  See
rte_hash_create().  They all check to see if certain CPU flags are
supported and use the appropriate function.  This is not "downgrading"
anything.  It is using the best available function that is supported
on the target system, not the build system.  That's exactly what this
(or the v3) patch does.

> This comes with a huge additional cost, and I do not believe
> it is supported at the moment.

It does not come at any additional cost really, all it does is reduce
the number of build systems which have to be supported.  It is exactly
the same idea as my first patch which checks for the RDSEED
instruction at run time and if the target system does not support it
"downgrades" to use the TSC counter.  Except now this is done at
run-time, not at build time.

thanks,
dan

  reply	other threads:[~2020-05-01 21:06 UTC|newest]

Thread overview: 48+ 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 [this message]
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
2023-06-12 15:55                                         ` Stephen Hemminger
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
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='CAGyogRbpUcffHwKUY83KqMm9CuSxrMD9_JXqHDy=-ppuQTbQBg@mail.gmail.com' \
    --to=dg@adax.com \
    --cc=bluca@debian.org \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).