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

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

On 2020-04-22 22:35, Dan Gora wrote:
> On Wed, Apr 22, 2020 at 5:14 PM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>> On 2020-04-22 19:44, Dan Gora wrote:
>>> On Wed, Apr 22, 2020 at 5:28 AM Mattias Rönnblom
>>> <mattias.ronnblom@ericsson.com> wrote:
>>>> On 2020-04-21 21:54, 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() which will try to
>>>>> resolve the getentropy() function dynamically using dlopen()/dlsym(),
>>>>> returning failure if the getentropy() function cannot be resolved or
>>>>> if it fails.
>>>> Two other options: providing a DPDK-native syscall wrapper for
>>>> getrandom(), or falling back to reading /dev/urandom. Have you
>>>> considered any of those two options? If so, why do you prefer
>>>> dlopen()/dlsym()?
>>> I didn't give any thought at all to using /dev/urandom.  The goal was
>>> not really to change how the thing worked, just to remove the
>>> dependency on glibc 2.25.
>>
>> /dev/urandom is basically only a different interface to the same
>> underlying mechanism.
>>
>> Such an alternative would look something like:
>>
>> static int
>> getentropy(void *buffer, size_t length)
>> {
>>           int rc = -1;
>>           int old_errno = errno;
>>           int fd;
>>
>>           fd = open("/dev/urandom", O_RDONLY);
>>
>>           if (fd < 0)
>>                   goto out;
>>
>>           if (read(fd, buffer, length) != length)
>>                   goto out_close;
>>
>>           rc = 0;
>>
>> out_close:
>>           close(fd);
>> out:
>>           errno = old_errno;
>>
>>           return rc;
>> }
> That's fine with me, but like I said I wasn't trying to change how any
> of this worked, just work around glibc dependencies.  There seems to
> be some subtle difference between /dev/urandom and /dev/random, but...
>
> https://protect2.fireeye.com/v1/url?k=1705be57-4b8f6b41-1705fecc-862f14a9365e-bb983def357fdfad&q=1&e=10fec9c1-51b3-4bc3-b77d-7eb39787d007&u=https%3A%2F%2Fpatches-gcc.linaro.org%2Fcomment%2F14484%2F
>
>>>> Failure to run on old libc seems like a non-issue to me.
>>> Well, again, it's a new dependency that didn't exist before.. We sell
>>> to telco customers, so we have to support 10s of different target
>>> platforms of various ages.  If they update their system, we'd have to
>>> recompile our code to be able to use getentropy().  Similarly, if we
>>> compiled on a system which has getentropy(), but the target system
>>> doesn't, then they cannot run our binary because of the glibc 2.25
>>> dependency.  That means that we have to have separate versions with
>>> and without getentropy().  It's a maintenance headache for no real
>>> benefit.
>>
>> I'm not sure I follow. Why would you need to recompile DPDK in case they
>> upgrade their system? It sounds like you care about initial seeding,
>> since you want getentropy() if it exists, but then in the next paragraph
>> you want to throw it out, so I'm a little confused.
> Well  _I_ wouldn't but maybe someone wants getentropy() for the
> initial seed.. I assume that's why it was added in the first place..
> For my application we don't care at all.  I just want to get rid of
> this dependency on glibc 2.25 and have the behavior be the same on
> meson and Makefile builds on the same complication system.


The reason for trying to avoid a wall time-based seed as the default is 
that application instances started at the roughly the same time might 
end up having a the same seed, which in turn might impact their behavior 
in an adverse way. For example, random back-off timers may be the same. 
On x86_64, TSC has a high resolution, but on other platforms its 
equivalent the clock rate is much lower.


>> Why doesn't the standard practice of compiling against the oldest
>> supported libc work for you?
> I guess I didn't realize that was "standard practice" but even so it
> still adds an unnecessary restriction on the complication platform.


If DPDK has the policy of attempting to allow DPDK applications compiled 
against one glibc version to run against another, older, version, we can 
go ahead and discuss the details further. That would be up to the tech 
board to decide. I would vote against it.


If the fix was simple, that's one thing. dlopen()/dlsym() doesn't 
qualify as such, nor does a syscall wrapper, as you pointed out.


>>> To my mind, since getentropy() can block it seems like it would
>>> probably be better to just remove it entirely, but I suppose that's up
>>> to the person(s) who put it in in the first place.
>>
>> Maybe I'm wrong, but I found it unlikely that a DPDK application would
>> start before the entropy pool was initialized. After this point,
>> getentropy() will not block. Do you consider this a real problem?
> Not really.. I don't know the original motivations for adding
> getentropy() in the first place.  I assumed that there was some good
> reason.  If there's not we could just back out all of these
> complications and revert commit faf8fd252785ee8b1 (et al...)
>
> Again, I don't have any dog in the fight about how to get the seed, I
> just wanted to remove the glibc dependency and the different behavior
> for Makefile and meson builds on the same complication system.
>
> For me, using dlsym() or reading from /dev/[u]random is 6 for half a
> dozen.  Both have precedents in other places in the DPDK code base.  I
> can fix it either way.
>
> thanks
> dan