On Thu, 26 Sep 2019 at 13:06, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > > On Thu, 26 Sep 2019 at 00:15, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > On Wed, Sep 25, 2019 at 9:14 AM Ard Biesheuvel > > <ard.biesheuvel@xxxxxxxxxx> wrote: > > > > > > Replace the chacha20poly1305() library calls with invocations of the > > > RFC7539 AEAD, as implemented by the generic chacha20poly1305 template. > > > > Honestly, the other patches look fine to me from what I've seen (with > > the small note I had in a separate email for 11/18), but this one I > > consider just nasty, and a prime example of why people hate those > > crypto lookup routines. > > > > Some of it is just the fundamental and pointless silly indirection, > > that just makes things harder to read, less efficient, and less > > straightforward. > > > > That's exemplified by this part of the patch: > > > > > struct noise_symmetric_key { > > > - u8 key[NOISE_SYMMETRIC_KEY_LEN]; > > > + struct crypto_aead *tfm; > > > > which is just one of those "we know what we want and we just want to > > use it directly" things, and then the crypto indirection comes along > > and makes that simple inline allocation of a small constant size > > (afaik it is CHACHA20POLY1305_KEY_SIZE, which is 32) be another > > allocation entirely. > > > > And it's some random odd non-typed thing too, so then you have that > > silly and stupid dynamic allocation using a name lookup: > > > > crypto_alloc_aead("rfc7539(chacha20,poly1305)", 0, CRYPTO_ALG_ASYNC); > > > > to create what used to be (and should be) a simple allocation that was > > has a static type and was just part of the code. > > > > That crypto_alloc_aead() call does a lot of things under the hood: > - use an existing instantiation of rfc7539(chacha20,poly1305) if available, > - look for modules that implement the whole transformation directly, > - if none are found, instantiate the rfc7539 template, which will > essentially do the above for chacha20 and poly1305, potentially using > per-arch accelerated implementations if available (for either), or > otherwise, fall back to the generic versions. > > What *I* see as the issue here is not that we need to do this at all, > but that we have to do it for each value of the key. IMO, it would be > much better to instantiate this thing only once, and have a way of > passing a per-request key into it, permitting us to hide the whole > thing behind the existing library interface. > Note that we don't have to do the whole dance for each new value of the key: subsequent invocations will all succeed at step #1, and grab the existing instantiation, but allocate a new TFM structure that refers to it. It is this step that we should be able to omit as well if the API is changed to allow per-request keys to be passed in via the request structure.