Re: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library

[Jason A. Donenfeld]

Hi Ard,

On Mon, Sep 17, 2018 at 7:26 AM Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> OK, so let me summarize my remaining concerns as well. I may be a bit
> more finicky than Andy, though.

Yes, and generally hostile to this whole initiative since the
beginning. But I am very grateful for your reviews nonetheless, and
I'll do my best to incorporate as much as is reasonable.

> I would like to urge Jason to
> bear with us and bring this discussion to a close before resubmitting.

What I fear is that either:
- You don't like the Zinc initiative in one way or another, and the
desire to "keep discussing" and adding more things is less out of
their necessity and more out of a desire to stall it indefinitely.
- You're going to bikeshed and bikeshed and waste tons of time until
Zinc copies lots of the same design decisions from the present crypto
API.

I do sincerely hope these are only fears and not what actually is
going on. I'll do my best to take into serious consideration what you
say -- many are indeed extremely helpful --  but I certainly won't be
wholesale accepting all the things you've mentioned.

Nevertheless, I'll make sure v5 has a pretty healthy quantity of
improvements in it before resubmitting.

> * simd_relax() is currently not called by the crypto routines
> themselves. This means that the worst case scheduling latency is
> unbounded, which is unacceptable for the -rt kernel. The worst case
> scheduling latency should never be proportional to the input size.
> (Apologies for not spotting that earlier)

Good catch. I actually did this correct when porting the crypto API to
use Zinc (in those later top commits in v4), but I hadn't in the Zinc
code itself. I'll address this for v5.

> maintainership
> responsibilities

Samuel and I intend to maintain Zinc in lib/zinc/ and send future
updates to it through Greg's tree, as mentioned in the 00/ cover
letter. The maintainership of the existing crypto API won't change.

> * The current organization of the code puts all available (for the
> arch) versions of all routines into a single module, which can only be
> built in once we update random.c to use Zinc's chacha20 routines. This
> bloats the core kernel (which is a huge deal for embedded systems that
> have very strict boot requirements).

I'll split each Zinc primitive into a separate module for v5, per your
and Andy's desire. And the SIMD code is already toggle-able via a
Kconfig menu option.

> we should
> work with Andy Polyakov (as I have done several times over the past 5+
> years) to upstream the changes we apply to the kernel version of the
> code.

Indeed this is the intent.

> The same applies to code from other sources, btw, but I am not
> personally familiar with them.

Good news on this front:
- Rene wrote the MIPS code specifically for WireGuard, so that _is_ upstream.
- Samuel wrote the BLAKE2 assembly, and he's the co-maintainer of Zinc with me.
- I talk to Dan and Peter a decent amount about qhasm.
- I'm in very close contact with the team behind HACL*, and they're
treating Zinc as a target -- stylistically and with regards to kernel
requirements -- which means they're looking at what's happening in
this patchset and adjusting accordingly.


> * If upstreaming the changes is not an option, they should be applied
> as a separate patch and not hidden in a 5000 line patch without any
> justification or documentation (but Jason is already working on that)

Indeed this is already underway.

Thanks again for your review.

Jason