Re: [PATCH v2] crypto: xts - Drop use of auxiliary buffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ondrej,

On Tue, Sep 04, 2018 at 10:06:42AM +0200, Ondrej Mosnacek wrote:
> Since commit acb9b159c784 ("crypto: gf128mul - define gf128mul_x_* in
> gf128mul.h"), the gf128mul_x_*() functions are very fast and therefore
> caching the computed XTS tweaks has only negligible advantage over
> computing them twice.
> 
> In fact, since the current caching implementation limits the size of
> the calls to the child ecb(...) algorithm to PAGE_SIZE (usually 4096 B),
> it is often actually slower than the simple recomputing implementation.
> 
> This patch simplifies the XTS template to recompute the XTS tweaks from
> scratch in the second pass and thus also removes the need to allocate a
> dynamic buffer using kmalloc().
> 
> As discussed at [1], the use of kmalloc causes deadlocks with dm-crypt.
> 
> PERFORMANCE RESULTS
> I measured time to encrypt/decrypt a memory buffer of varying sizes with
> xts(ecb-aes-aesni) using a tool I wrote ([2]) and the results suggest
> that after this patch the performance is either better or comparable for
> both small and large buffers. Note that there is a lot of noise in the
> measurements, but the overall difference is easy to see.
> 
> Old code:
> ALGORITHM       KEY (b) DATA (B)        TIME ENC (ns)   TIME DEC (ns)
>         xts(aes)     256              64             331             328
>         xts(aes)     384              64             332             333
>         xts(aes)     512              64             338             348
>         xts(aes)     256             512             889             920
>         xts(aes)     384             512            1019             993
>         xts(aes)     512             512            1032             990
>         xts(aes)     256            4096            2152            2292
>         xts(aes)     384            4096            2453            2597
>         xts(aes)     512            4096            3041            2641
>         xts(aes)     256           16384            9443            8027
>         xts(aes)     384           16384            8536            8925
>         xts(aes)     512           16384            9232            9417
>         xts(aes)     256           32768           16383           14897
>         xts(aes)     384           32768           17527           16102
>         xts(aes)     512           32768           18483           17322
> 
> New code:
> ALGORITHM       KEY (b) DATA (B)        TIME ENC (ns)   TIME DEC (ns)
>         xts(aes)     256              64             328             324
>         xts(aes)     384              64             324             319
>         xts(aes)     512              64             320             322
>         xts(aes)     256             512             476             473
>         xts(aes)     384             512             509             492
>         xts(aes)     512             512             531             514
>         xts(aes)     256            4096            2132            1829
>         xts(aes)     384            4096            2357            2055
>         xts(aes)     512            4096            2178            2027
>         xts(aes)     256           16384            6920            6983
>         xts(aes)     384           16384            8597            7505
>         xts(aes)     512           16384            7841            8164
>         xts(aes)     256           32768           13468           12307
>         xts(aes)     384           32768           14808           13402
>         xts(aes)     512           32768           15753           14636

Can you align the headers of these tables?

> +static int xor_tweak(struct rctx *rctx, struct skcipher_request *req)
>  {
> -	struct rctx *rctx = skcipher_request_ctx(req);
> -	le128 *buf = rctx->ext ?: rctx->buf;
> -	struct skcipher_request *subreq;
>  	const int bs = XTS_BLOCK_SIZE;
>  	struct skcipher_walk w;
> -	struct scatterlist *sg;
> -	unsigned offset;
> +	le128 t = rctx->t;
>  	int err;

Maybe you could add a brief comment above xor_tweak() explaining the design
choice for posterity, e.g.:

/*
 * We compute the tweak masks twice (both before and after the ECB encryption or
 * decryption) to avoid having to allocate a temporary buffer, which usually
 * would be slower than just doing the gf128mul_x_ble() calls again.
 */

Otherwise this looks good.  Thanks for doing this!

The new implementation isn't *guaranteed* to be faster, but it should be most of
the time, and it's definitely much simpler.  And the current one has had bugs.

Note that if ever needed there's also still room for optimizing the GF(2^128)
multiplications further, e.g. multiplying by 'x' and 'x^2' in parallel, or maybe
having a version specialized for 32-bit processors.

FYI, I think that 'subreq' can have an alignmask insufficient for 'le128', which
can cause misaligned accesses during the second xor_tweak().  But, the current
version has that bug too...

Reviewed-by: Eric Biggers <ebiggers@xxxxxxxxxx>

- Eric



[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux