Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAKv+Gu-VqfFsW+nrG+-2g1-eu6S+ZuD7qaN9aTchwD=Bcj_giw@mail.gmail.com>
 <04D32F59-34D4-4EBF-80E3-69088D14C5D8@amacapital.net> <CAKv+Gu8s6AuZfdVUSmpgi-eY_9oZr-j4sdFygUOR3uvQXji+rQ@mail.gmail.com>
 <CALCETrXWSu_fY5BetMah=iEOqSgkOphMOKcMrtiWyN0QqbZspw@mail.gmail.com>
In-Reply-To: <CALCETrXWSu_fY5BetMah=iEOqSgkOphMOKcMrtiWyN0QqbZspw@mail.gmail.com>
From:   Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Mon, 7 Oct 2019 17:12:10 +0200
Message-ID: <CAKv+Gu8u8Oco==YRPSa4mq_eZyUcB_Apj-k_vo=7WvTwCp8k+A@mail.gmail.com>
Subject: Re: [PATCH v2 00/20] crypto: crypto API library interfaces for WireGuard
To:     Andy Lutomirski <luto@amacapital.net>
Cc:     "Jason A. Donenfeld" <Jason@zx2c4.com>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@vger.kernel.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        David Miller <davem@davemloft.net>,
        Greg KH <gregkh@linuxfoundation.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Samuel Neves <sneves@dei.uc.pt>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Eric Biggers <ebiggers@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Martin Willi <martin@strongswan.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Mon, 7 Oct 2019 at 17:02, Andy Lutomirski <luto@amacapital.net> wrote:
>
> On Sun, Oct 6, 2019 at 10:24 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Mon, 7 Oct 2019 at 06:44, Andy Lutomirski <luto@amacapital.net> wrote:
> > >
>
> > > > Actually, this can be addressed by retaining the module dependencies
> > > > as before, but permitting the arch module to be omitted at load time.
> > >
> > > I think that, to avoid surprises, you should refuse to load the arch module if the generic module is loaded, too.
> > >
> >
> > Most arch code depends on CPU features that may not be available given
> > the context, either because they are SIMD or because they are optional
> > CPU instructions. So we need both modules at the same time anyway, so
> > that we can fall back to the generic code at runtime.
> >
> > So what I'd like is to have the generic module provide the library
> > interface, but rely on arch modules that are optional.
> >
> > We already have 95% of what we need with weak references. We have the
> > ability to test for presence of the arch code at runtime, and we even
> > have code patching for all architectures (through static relocations).
> >
> > However, one could argue that this is more a [space] optimization than
> > anything else, so I am willing to park this discussion until support
> > for static calls has been merged, and proceed with something simpler.
>
> I'd suggest tabling it until static calls are merged.  PeterZ just
> sent a new patchbomb for it anyway.
>

As it turns out, static calls are a poor fit for this. Imagine an interface like

poly1305_init(state)
poly1305_update(state, input)
poly1305_fina(state, digest)

which can be implemented by different libraries. The problem is that
state is opaque, and so it is generally not guaranteed that a sequence
that was started using one implementation can be completed using
another one.

Since the whole point is having a simple library interface,
complicating this with RCU hooks or other crazy plumbing to ensure
that no calls are in progress when you switch one out for another one,
I don't think static calls are suitable for this use case.

> What I'm trying to get at here and apparently saying badly is that I
> want to avoid a situation where lsmod shows the arch module loaded but
> the arch code isn't actually executing.

My goal here is to allow the generic library to be loaded with or
without the arch code, with the arch code always being used when it is
loaded. This is what I implemented using weak references, but it
requires a tweak in the module loader (two lines but not pretty).
Using weak references preserves the dependency order, since the
generic module will depend on the arch module (and up the refcount) it
any of its weak references were fulfilled by the arch module in
question. Using static calls will invert the dependency relation,
since the arch code will need to perform a static_call_update() to
make [users of] the generic library point to its code. How this works
with managing the module refcounts and unload order is an open
question afaict.

> Regardless of how everything
> gets wired up (static calls, weak refs, etc), the system's behavior
> should match the system's configuration, which means that we should
> not allow any improper order of loading things so that everything
> *appears* to be loaded but does not actually function.
>

Yes. that is the whole point.

> Saying "modprobe will do the right thing and let's not worry about
> silly admins using insmod directly" is not a good solution.

Agreed.

I have disregarded static calls and weak references for the time
being, and I will proceed with an implementation that uses neither.
The downside of this is that, e.g., we are forced to load the NEON
module on non-NEON capable hardware without calling any of its code,
but this is basically the Zinc situation only with the NEON and the
generic code living in different modules.