From: Andy Lutomirski <luto@amacapital.net>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Andy Lutomirski <luto@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Netdev <netdev@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
"Jason A. Donenfeld" <Jason@zx2c4.com>,
Hannes Frederic Sowa <hannes@stressinduktion.org>,
Alexei Starovoitov <alexei.starovoitov@gmail.com>,
Eric Dumazet <edumazet@google.com>,
Eric Biggers <ebiggers3@gmail.com>,
Tom Herbert <tom@herbertland.com>,
"David S. Miller" <davem@davemloft.net>,
Herbert Xu <herbert@gondor.apana.org.au>
Subject: Re: [RFC PATCH 4.10 1/6] crypto/sha256: Refactor the API so it can be used without shash
Date: Sat, 24 Dec 2016 09:57:53 -0800 [thread overview]
Message-ID: <CALCETrVbT_1=cdU1+a-+KbhoFqeT3XvbHjY0s_U7C5JVgiPx_Q@mail.gmail.com> (raw)
In-Reply-To: <CAKv+Gu-UCqwchgbhg8VN4s=Yxk=PkjXG46NZo=8P4wAQtJ2TXw@mail.gmail.com>
On Sat, Dec 24, 2016 at 2:33 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> Hi Andy,
>
> On 24 December 2016 at 02:22, Andy Lutomirski <luto@kernel.org> wrote:
>> There are some pieecs of kernel code that want to compute SHA256
>> directly without going through the crypto core. Adjust the exported
>> API to decouple it from the crypto core.
>>
>
> There are a bunch of things happening at the same time in this patch,
> i.e., unnecessary renames of functions with static linkage, return
> type changes to the base prototypes (int (*)(...) to void (*)(...))
> and the change for the base functions to take a struct sha256_state
> ctx rather than a shash_desc. I suppose you are mainly after the
> latter, so could we please drop the other changes?
>
> For the name clashes, could we simply use the crypto_ prefix for the
> globally visible functions rather than using names that are already in
> use? (and having to go around clean up the conflicts)
> As for the return type changes, the base functions intentionally
> return int to allow tail calls from the functions exposed by the
> crypto API (whose prototypes cannot be changed). Unlikely to matter in
> the grand scheme of things (especially now that the base layer
> consists of static inline functions primarily), but it is equally
> pointless to go around and change them to return void IMO.
>
> So what remains is the struct shash_desc to struct sha256_state
> change, which makes sense given that you are after a sha256_digest()
> function that does not require the crypto API. But it seems your use
> case does not rely on incremental hashing, and so there is no reason
> for the state to be exposed outside of the implementation, and we
> could simply expose a crypto_sha256_digest() routine from the
> sha256_generic.c implementation instead.
I actually do use incremental hashing later on. BPF currently
vmallocs() a big temporary buffer just so it can fill it and hash it.
I change it to hash as it goes.
I painted the bike shed the other way because I thought that crypto_
names should indicate that they're the versions compatible with the
crypto API, but I take your point about churn. Part of the reason I
didn't want to use crypto_sha256_update is because that function is
currently like this:
int crypto_sha256_update(struct shash_desc *desc, const u8 *data,
unsigned int len)
and I wanted to avoid churn. The sha256_update() functions scattered
all over were static, so I didn't worry about them.
I'm going to give this another try as a split-up series that tries to
avoid making any changes beyond simple function renames to the
drivers.
>
> Also, I strongly feel that crypto and other security related patches
> should be tested before being posted, even if they are only RFC,
> especially when they are posted by high profile kernel devs like
> yourself. (Your code incorrectly calls crypto_sha2_final() in a couple
> of places, resulting in the finalization being performed twice, once
> with the accelerated block transform and again with the generic
> transform)
>
I tested it, albeit poorly. I wanted feedback on the API (thanks!)
and I figured I could more carefully check the implementation once the
API survives a bit of review. Since it looks like I have to rework
this, I'd need to re-test anyway.
>> I suspect this will very slightly speed up the SHA256 shash operations
>> as well by reducing the amount of indirection involved.
>>
>
> I think you have a valid point when it comes to the complexity of the
> crypto API in general. But the struct sha256_state is embedded in the
> shash_desc rather than referred to via a pointer, so the level of
> indirection does not appear to change. And given how 99.9% of the
> SHA256 execution time is spent in the block transform routine anyway,
> I expect the performance delta to be in the noise tbh.
s/very slightly/negligibly? There's an extra speedup from avoiding a
variable-length stack allocation, but that probably doesn't matter
much either.
>
> Finally, another thing to keep in mind is that the base layers of
> SHA-1, SHA-256 and SHA-512 are intentionally structured in the same
> way. If there is a need for a digest() entry point, I'd prefer to add
> them for all flavours.
I want to get sha256 right first. Once it's in good shape, making the
same changes to the other variants should be easy.
>
> Whether this still belongs under crypto or under lib/sha256.c as a
> library function (allowing archs to override it) is open for debate.
> If hashing BPF programs becomes a hot spot, we probably have bigger
> problems.
>
> Regards,
> Ard.
>
> P.S. I do take your point regarding the arch_sha256_block_transform()
> proposed in your follow up email, but there are some details (SIMD,
> availability of the instructions etc) that would make it only suitable
> for the generic implementation anyway, and the base layer was already
> a huge improvement compared to the open coded implementations of the
> SHA boilerplate.
Agreed, and my model may not be quite right. It might have to be
something like:
if (arch_begin_sha256(len)) {
... do it with arch helpers...
arch_end_sha256();
} else {
... do it generically ...
}
>> - return sha256_base_finish(desc, out);
>> + return crypto_sha2_final(desc, out);
>
> This is wrong: your crypto_sha2_final also calls sha256_base_do_finalize()
Ugh, right. I clearly need to organize this change better to avoid
this kind of mistake.
next prev parent reply other threads:[~2016-12-24 17:58 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-24 2:22 [RFC PATCH 4.10 0/6] Switch BPF's digest to SHA256 Andy Lutomirski
2016-12-24 2:22 ` [RFC PATCH 4.10 1/6] crypto/sha256: Refactor the API so it can be used without shash Andy Lutomirski
2016-12-24 2:26 ` Andy Lutomirski
2016-12-24 10:33 ` Ard Biesheuvel
2016-12-24 17:57 ` Andy Lutomirski [this message]
2016-12-26 7:57 ` Herbert Xu
2016-12-26 17:51 ` Ard Biesheuvel
2016-12-26 18:08 ` Andy Lutomirski
2016-12-27 9:58 ` Herbert Xu
2016-12-27 14:16 ` Daniel Borkmann
2016-12-27 19:00 ` Andy Lutomirski
2016-12-24 2:22 ` [RFC PATCH 4.10 2/6] crypto/sha256: Make the sha256 library functions selectable Andy Lutomirski
2016-12-24 2:22 ` [RFC PATCH 4.10 3/6] bpf: Use SHA256 instead of SHA1 for bpf digests Andy Lutomirski
2016-12-24 19:59 ` Daniel Borkmann
2016-12-27 1:36 ` Alexei Starovoitov
2016-12-27 2:08 ` Andy Lutomirski
2016-12-24 2:22 ` [RFC PATCH 4.10 4/6] bpf: Avoid copying the entire BPF program when hashing it Andy Lutomirski
2016-12-24 2:22 ` [RFC PATCH 4.10 5/6] bpf: Rename fdinfo's prog_digest to prog_sha256 Andy Lutomirski
2016-12-24 2:22 ` [RFC PATCH 4.10 6/6] net: Rename TCA*BPF_DIGEST to ..._SHA256 Andy Lutomirski
2016-12-26 8:20 ` [RFC PATCH 4.10 0/6] Switch BPF's digest to SHA256 Herbert Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALCETrVbT_1=cdU1+a-+KbhoFqeT3XvbHjY0s_U7C5JVgiPx_Q@mail.gmail.com' \
--to=luto@amacapital.net \
--cc=Jason@zx2c4.com \
--cc=alexei.starovoitov@gmail.com \
--cc=ard.biesheuvel@linaro.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=ebiggers3@gmail.com \
--cc=edumazet@google.com \
--cc=hannes@stressinduktion.org \
--cc=herbert@gondor.apana.org.au \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=tom@herbertland.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).