Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Thu, 26 Sep 2019 at 13:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Thu, 26 Sep 2019 at 00:15, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Wed, Sep 25, 2019 at 9:14 AM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> 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.