lkml.org 
[lkml]   [2018]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library
    From
    Date


    > On Sep 16, 2018, at 10:26 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

    >
    > As far as I can tell (i.e., as a user not a network dev), WireGuard is
    > an excellent piece of code, and I would like to see it merged. I also
    > think there is little disagreement about the quality of the proposed
    > algorithm implementations and the usefulness of having a set of easy
    > to use solid crypto primitives in addition to or complementing the
    > current crypto API.
    >
    > I do have some concerns over how the code is organized though:
    >
    > * 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)
    >
    > * Using a cute name for the crypto library [that will end up being the
    > preferred choice for casual use only] may confuse people, given that
    > we have lots of code in crypto/ already. I'd much prefer using, e.g.,
    > crypto/lib and crypto/api (regardless of where the arch specific
    > pieces live)
    >
    > * I'd prefer the arch specific pieces to live under arch, but I can
    > live with keeping them in a single place, as long as the arch
    > maintainers have some kind of jurisdiction over them. I also think
    > there should be some overlap between the maintainership
    > responsibilities of the two generic pieces (api and lib).
    >
    > * (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.


    > * 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). It also makes it impossible to
    > simply blacklist a module if you, for instance, prefer to use the
    > [potentially more power efficient] scalar code over the SIMD code when
    > using a distro kernel.

    I think the module organization needs to change. It needs to be possible to have chacha20 built in but AES or whatever as a module.

    >
    > [To summarize the 4 points above, I'd much rather see a more
    > conventional organization where different parts are provided by
    > different modules. I don't think the argument that inlining is needed
    > for performance is actually valid, given that we have branch
    > predictors and static keys, and the arch SIMD code is built as
    > separate object files anyway]

    I might have agreed before Spectre :(. Unfortunately, unless we do some magic, I think the code would look something like:

    if (static_branch_likely(have_simd)) arch_chacha20();

    ...where arch_chacha20 is a *pointer*. And that will generate a retpoline and run very, very slowly. (I just rewrote some of the x86 entry code to eliminate one retpoline. I got a 5% speedup on some tests according to the kbuild bot.)

    So, if we really wanted modules, we’d need a new dynamic patching mechanism.

    I would suggest instead adding two boot (or debugfs) options:

    simd=off: disables simd_get using a static branch.

    crypto_chacha20_nosimd: Does what it sounds like.

    \
     
     \ /
      Last update: 2018-09-17 16:52    [W:4.377 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site