From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5FC90A04B2; Fri, 1 May 2020 23:06:20 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id F06241D8EC; Fri, 1 May 2020 23:06:18 +0200 (CEST) Received: from mail-io1-f50.google.com (mail-io1-f50.google.com [209.85.166.50]) by dpdk.org (Postfix) with ESMTP id 77C4E4C89 for ; Fri, 1 May 2020 23:06:17 +0200 (CEST) Received: by mail-io1-f50.google.com with SMTP id e9so6095995iok.9 for ; Fri, 01 May 2020 14:06:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=YXtg27VXQCsz6EItfkmUV6wAaE4961KxgGsGHOSW6xA=; b=J33Jlpc4+mAneJ4bcM+oyptqLMt35eHXqCuhX3F4ourar5Q91Y0LvNca0Sxc1hDUiT ZtwbVeiohyfoDKwYytsHXi2Wpwb1a6c/1ThzKduFqcpm/6lXFg0tL4OrLdVnW/UAdu7o YkqzC3fyJYddmjjppjvpPmpdGELeDsWcr+zx8FMZaKvUlXTSxScgdm/2oQ8PFSAzsAr6 AAEsqRR1170joXolYLhBWo4vmR6cVkIktJ1DpvPa4qCBAqWcopGDS6Z8mrWY3n+DaQZw 1cmsGwx8U1bMMpddGZ4YhqHGJ2D/b4Lxhpowch9qf9g+Wxm6X3XEHY1XqxgedtdUIAbd N6nw== X-Gm-Message-State: AGi0PuZYF6oZguLsRmR6f3MBW5Tszuo3B6PTmN9JrN2x9WMNug3cndo9 h40+RSGHGaEHSAVglm1uBWodt6MkL3bkpgc9VrA= X-Google-Smtp-Source: APiQypIPHn/OI0bwOW1htIe3Bxpp1giLK6xV/7eBH/fQ6slcOVbisRNrelIeScDOogzTmgiT/4JsjoI1k3yS/7Dr+3g= X-Received: by 2002:a5e:8e44:: with SMTP id r4mr5567495ioo.47.1588367176518; Fri, 01 May 2020 14:06:16 -0700 (PDT) MIME-Version: 1.0 References: <20200421195446.1730-1-dg@adax.com> <20200421195446.1730-3-dg@adax.com> <5df15087-781e-d27f-b7d8-50b1b8cb0c94@ericsson.com> <80e5cf97-feae-c753-5c65-4f3b121729f3@ericsson.com> <9782db5b1e5d2dfdcfa8e52410fd166df5db938d.camel@debian.org> In-Reply-To: From: Dan Gora Date: Fri, 1 May 2020 18:05:40 -0300 Message-ID: To: Luca Boccassi Cc: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , "dev@dpdk.org" , David Marchand , Jerin Jacob Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH 2/2] eal: resolve getentropy at run time for random seed X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, May 1, 2020 at 1:29 PM Luca Boccassi 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