On 2020-06-29 19:57, Dan Gora wrote: > On Mon, Jun 29, 2020 at 6:30 AM Mattias Rönnblom > <mattias.ronnb...@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 <d...@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://protect2.fireeye.com/v1/url?k=64eebf70-3a4e5fe4-64eeffeb-86d2114eab2f-e9077eca0a4dd2ab&q=1&e=2360d5cd-0b70-4aa9-86f1-f72782986b27&u=https%3A%2F%2Fwww.mail-archive.com%2Fdev%40dpdk.org%2Fmsg164646.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():
Since you've replaced getentropy() altogether for all builds, there's no need to be API-compatible. Just do an as-simple-as-possible function that reads 8 bytes from /dev/urandom. > https://protect2.fireeye.com/v1/url?k=7d08ee94-23a80e00-7d08ae0f-86d2114eab2f-0d7c5c2b9ffa9874&q=1&e=2360d5cd-0b70-4aa9-86f1-f72782986b27&u=https%3A%2F%2Fsourceware.org%2Fgit%2F%3Fp%3Dglibc.git%3Ba%3Dblob%3Bf%3Dsysdeps%2Funix%2Fsysv%2Flinux%2Fgetentropy.c%3Bh%3D1778632ff1f1fd77019401c3fbaa164c167248b0%3Bhb%3D92dcaa3e2f7bf0f7f1c04cd2fb6a317df1a4e225 > >> >>> + 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. The random(4) manual page says it can't be interrupted for small requests, which seems to hold true for Linux 3.17 and later. I don't know the hows and whys of glibc getentropy(). Studying LGPL code before implementing BSD licensed code performing the same function might not be the best of ideas. >> >>> + 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://protect2.fireeye.com/v1/url?k=44546baa-1af48b3e-44542b31-86d2114eab2f-bc2d2a695ed31cdc&q=1&e=2360d5cd-0b70-4aa9-86f1-f72782986b27&u=https%3A%2F%2Fsourceware.org%2Fgit%2F%3Fp%3Dglibc.git%3Ba%3Dblob%3Bf%3Dsysdeps%2Funix%2Fsysv%2Flinux%2Fgetentropy.c%3Bh%3D1778632ff1f1fd77019401c3fbaa164c167248b0%3Bhb%3D92dcaa3e2f7bf0f7f1c04cd2fb6a317df1a4e225 > >> >>> +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