Re: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library
> On Sep 17, 2018, at 8:28 AM, Jason A. Donenfeld wrote: > > On Mon, Sep 17, 2018 at 4:52 PM Andy Lutomirski wrote: >>> * (Nit) The GCC command line -include'd .h files contain variable and >>> function definitions so they are actually .c files. >> >> Hmm. I would suggest just getting rid of the -include magic entirely. The >> resulting ifdef will be more comprehensible. > > I really don't think so, actually. The way the -include stuff works > now is that the glue code is inlined in the same place that the > assembly object file is added to the build object list, so it gels > together cleanly, as the thing is defined and set in one single place. > I could go back to the ifdefs - and even make them as clean as > possible - but I think that puts more things in more places and is > therefore more confusing. The -include system now works extremely > well. Is it really better than: #ifdef CONFIG_X86_64 #include "whatever" #endif It seems a more obfuscated than needed to put the equivalent of that into the Makefile, and I don't think people really like searching through the Makefile to figure out why the code does what it does.
Re: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library
> On Sep 16, 2018, at 10:07 PM, Jason A. Donenfeld wrote: > > Hey Andy, > > Thanks a lot for your feedback. > >> On Mon, Sep 17, 2018 at 6:09 AM Andy Lutomirski wrote: >> 1. Zinc conflates the addition of a new API with the replacement of >> some algorithm implementations. This is problematic. Look at the >> recent benchmarks of ipsec before and after this series. Apparently >> big packets get faster and small packets get slower. It would be >> really nice to bisect the series to narrow down *where* the regression >> came from, but, as currently structured, you can't. >> >> The right way to do this is to rearrange the series. First, the new >> Zinc APIs should be added, and they should be backed with the >> *existing* crypto code. (If the code needs to be moved or copied to a >> new location, so be it. The patch will be messy because somehow the >> Zinc API is going to have to dispatch to the arch-specific code, and >> the way that the crypto API handles it is not exactly friendly to this >> type of use. So be it.) Then another patch should switch the crypto >> API to use the Zinc interface. That patch, *by itself*, can be >> benchmarked. If it causes a regression for small ipsec packets, then >> it can be tracked down relatively easily. Once this is all done, the >> actual crypto implementation can be changed, and that changed can be >> reviewed on its own merits. > > That ipsec regression was less related to the implementation and more > related to calling kernel_fpu_begin() unnecessarily, something I've > now fixed. So I'm not sure that's such a good example. However, I can > try to implement Zinc over the existing assembly (Martin's and Ard's), > first, as you've described. This will be a pretty large amount of > work, but if you think it's worth it for the commit history, then I'll > do it. Ard, what do you think? I think it would be nice, but if the authors of that assembly are convinced it should be replaced, then this step is optional IMO. > >> 2. The new Zinc crypto implementations look like they're brand new. I >> realize that they have some history, some of them are derived from >> OpenSSL, etc, but none of this is really apparent in the patches >> themselves. > > The whole point of going with these is that they _aren't_ brand new, > yet they are very fast. Eyeballs and fuzzer hours are important, and > AndyP's seems to get the most eyeballs and fuzzer hours, generally. > >> it would be nice if >> the patches made it more clear how the code differs from its origin. >> At the very least, though, if the replacement of the crypto code were, >> as above, a patch that just replaced the crypto code, it would be much >> easier to review and benchmark intelligently. > > You seem to have replied to the v3 thread, not the v4 thread. I've > already started to include lots of detail about the origins of the > code and [any] important differences in v4, and I'll continue to add > more detail for v5. This is indeed better. Ard’s reply covers this better.
Re: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library
From: Andy Lutomirski Date: Sun, 16 Sep 2018 21:09:11 -0700 > CRYPTO API > M: Herbert Xu > M: "David S. Miller" > L: linux-cry...@vger.kernel.org > > Herbert hasn't replied to any of these submissions. You're the other > maintainer :) Herbert is the primary crypto maintainer, I haven't done a serious review of crypto code in ages. So yes, Herbert review is what is important here.
Re: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library
On Tue, Sep 11, 2018 at 4:57 PM David Miller wrote: > > From: Andrew Lunn > Date: Wed, 12 Sep 2018 01:30:15 +0200 > > > Just as an FYI: > > > > 1) I don't think anybody in netdev has taken a serious look at the > > network code yet. There is little point until the controversial part > > of the code, Zinc, has been sorted out. > > > > 2) I personally would be surprised if DaveM took this code without > > having an Acked-by from the crypto subsystem people. In the same way, > > i doubt the crypto people would take an Ethernet driver without having > > DaveM's Acked-by. > > Both of Andrew's statements are completely true. > > I'm not looking at any of the networking bits until the crypto stuff > is fully sorted and fully supported and Ack'd by crypto folks. So, as a process question, whom exactly are we waiting for: CRYPTO API M: Herbert Xu M: "David S. Miller" L: linux-cry...@vger.kernel.org Herbert hasn't replied to any of these submissions. You're the other maintainer :) To the extent that you (DaveM) want my ack, here's what I think of the series so far: The new APIs to the crypto primitives are good. For code that wants to do a specific known crypto operation, they are much, much more pleasant to use than the existing crypto API. The code cleanups in the big keys patch speak for themselves IMO. I have no problem with the addition of a brand-new API to the kernel, especially when it's a nice one like Zinc's, even if that API starts out with only a couple of users. Zinc's arrangement of arch code is just fine. Sure, there are arguments that putting arch-specific code in arch/ is better, but this is mostly bikeshedding IMO. There has been some discussion of the exact best way to handle simd_relax() and some other minor nitpicks of API details. This kind of stuff doesn't need to block the series -- it can always be reworked down the road if needed. There are two things I don't like right now, though: 1. Zinc conflates the addition of a new API with the replacement of some algorithm implementations. This is problematic. Look at the recent benchmarks of ipsec before and after this series. Apparently big packets get faster and small packets get slower. It would be really nice to bisect the series to narrow down *where* the regression came from, but, as currently structured, you can't. The right way to do this is to rearrange the series. First, the new Zinc APIs should be added, and they should be backed with the *existing* crypto code. (If the code needs to be moved or copied to a new location, so be it. The patch will be messy because somehow the Zinc API is going to have to dispatch to the arch-specific code, and the way that the crypto API handles it is not exactly friendly to this type of use. So be it.) Then another patch should switch the crypto API to use the Zinc interface. That patch, *by itself*, can be benchmarked. If it causes a regression for small ipsec packets, then it can be tracked down relatively easily. Once this is all done, the actual crypto implementation can be changed, and that changed can be reviewed on its own merits. As a simplistic example, I wrote this code a while back: https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6 and its two parents. This series added a more Zinc-like API to SHA256. And it did it without replacing the SHA256 implementation. Doing this for Zinc would be a bit more complication, since the arch code would need to be invoked too, but it should be doable. FWIW, Wireguard should not actually depend on the replacement of the crypto implementation. 2. The new Zinc crypto implementations look like they're brand new. I realize that they have some history, some of them are derived from OpenSSL, etc, but none of this is really apparent in the patches themselves. It would be great if the kernel could literally share the same code as something like OpenSSL, since OpenSSL gets much more attention than the kernel's crypto. Failing that, it would be nice if the patches made it more clear how the code differs from its origin. At the very least, though, if the replacement of the crypto code were, as above, a patch that just replaced the crypto code, it would be much easier to review and benchmark intelligently. --Andy
Re: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library
On 13 September 2018 at 17:58, Jason A. Donenfeld wrote: > On Thu, Sep 13, 2018 at 5:43 PM Ard Biesheuvel > wrote: >> I'd prefer it if all the accelerated software implementations live in >> the same place. But I do strongly prefer arch code to live in >> arch/$arch > > Zinc follows the scheme of the raid6 code, as well as of most other > crypto libraries: code is grouped by cipher, making it easy for people > to work with and understand differing implementations. It also allows > us to trivially link these together at compile time rather than at > link time, which makes cipher selection much more efficient. It's > really much more maintainable this way. > >> I think AES-GCM is a useful example here. I really like the SIMD token >> abstraction a lot, but I would like to understand how this would work >> in Zinc if you have >> a) a generic implementation >> b) perhaps an arch specific scalar implementation >> c) a pure NEON implementation >> d) an implementation using AES instructions but not the PMULL instructions >> e) an implementation that uses AES and PMULL instructions. > > The same way that Zinc currently chooses between the five different > implementations for, say, x86_64 ChaCha20: > > - Generic C scalar > - SSSE3 > - AVX2 > - AVX512F > - AVX512VL > > We make a decision based on CPU capabilities, SIMD context, and input > length, and then choose the right function. > OK, so given random.c's future dependency on Zinc (for ChaCha20), and the fact that Zinc is one monolithic piece of code, all versions of all algorithms will always be statically linked into the kernel proper. I'm not sure that is acceptable. >> You know what? If you're up for it, let's not wait until Plumbers, but >> instead, let's collaborate off list to get this into shape. > > Sure, sounds good. > BTW you haven't answered my question yet about what happens when the WireGuard protocol version changes: will we need a flag day and switch all deployments over at the same time?
