Re: [dpdk-dev] [PATCH 2/2] eal: resolve getentropy at run time for random seed

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2020-05-01 23:05, Dan Gora wrote:
> 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.


rdseed will go away eventually, when all reasonable systems have 
getentropy(). That might well take a couple of years.


> 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.


You build against the oldest libc supported. The rest of the 
dependencies, you carry with your application. Again, I believe this to 
be standard practice, and has been so for ages. Containers are really an 
extension of this method, but it was common pattern when commercial 
applications were more common (especially true for things like Solaris- 
or HP-UX-based apps). You didn't even care about getentropy(), so I 
can't understand how this is a problem.


> 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.


There's no new dependency if you build against an older glibc version.


> 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.
>
The make build is going away completely.