linux-kernel - Re: [PATCH v1 1/1] lib/crypto: tests: Add KUnit tests for AES

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260115220558.25390c0e@pumpkin>
Date: Thu, 15 Jan 2026 22:05:58 +0000
From: David Laight <david.laight.linux@...il.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Holger Dengler <dengler@...ux.ibm.com>, Ard Biesheuvel
 <ardb@...nel.org>, "Jason A . Donenfeld" <Jason@...c4.com>, Herbert Xu
 <herbert@...dor.apana.org.au>, Harald Freudenberger <freude@...ux.ibm.com>,
 linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org
Subject: Re: [PATCH v1 1/1] lib/crypto: tests: Add KUnit tests for AES

On Thu, 15 Jan 2026 12:43:32 -0800
Eric Biggers <ebiggers@...nel.org> wrote:

> On Thu, Jan 15, 2026 at 07:38:31PM +0100, Holger Dengler wrote:
> > Add a KUnit test suite for AES library functions, including KAT and
> > benchmarks.
> > 
> > Signed-off-by: Holger Dengler <dengler@...ux.ibm.com>  
> 
> The cover letter had some more information.  Could you put it in the
> commit message directly?  Normally cover letters aren't used for a
> single patch: the explanation should just be in the patch itself.
> 
> > diff --git a/lib/crypto/tests/aes-testvecs.h b/lib/crypto/tests/aes-testvecs.h
> > new file mode 100644
> > index 000000000000..dfa528db7f02
> > --- /dev/null
> > +++ b/lib/crypto/tests/aes-testvecs.h
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _AES_TESTVECS_H
> > +#define _AES_TESTVECS_H
> > +
> > +#include <crypto/aes.h>
> > +
> > +struct buf {
> > +	size_t blen;
> > +	u8 b[];
> > +};  
> 
> 'struct buf' is never used.
> 
> > +static const struct aes_testvector aes128_kat = {  
> 
> Where do these test vectors come from?  All test vectors should have a
> documented source.
> 
> > +static void benchmark_aes(struct kunit *test, const struct aes_testvector *tv)
> > +{
> > +	const size_t num_iters = 10000000;  
> 
> 10000000 iterations is too many.  That's 160 MB of data in each
> direction per AES key length.  Some CPUs without AES instructions can do
> only ~20 MB AES per second.  In that case, this benchmark would take 16
> seconds to run per AES key length, for 48 seconds total.

Probably best to first do a test that would take a 'reasonable' time
on a cpu without AES. If that is 'very fast' then do a longer test
to get more accuracy on a faster implementation.

> 
> hash-test-template.h and crc_kunit.c use 10000000 / (len + 128)
> iterations.  That would be 69444 in this case (considering len=16),
> which is less than 1% of the iterations you've used.  Choosing a number
> similar to that would seem more appropriate.
> 
> Ultimately these are just made-up numbers.  But I think we should aim
> for the benchmark test in each KUnit test suite to take less than a
> second or so.  The existing tests roughly achieve that, whereas it seems
> this one can go over it by quite a bit due to the 10000000 iterations.

Even 1 second is a long time, you end up getting multiple interrupts included.
I think a lot of these benchmarks are far too long.
Timing differences less that 1% can be created by scheduling noise.
Running a test that takes 200 'quanta' of the timer used has an
error margin of under 1% (100 quanta might be enough).
While the kernel timestamps have a resolution of 1ns the accuracy is worse.
If you run a test for even just 10us you ought to get reasonable accuracy
with a reasonable hope of not getting an interrupt.
Run the test 10 times and report the fastest value.

You'll then find the results are entirely unstable because the cpu clock
frequency keeps changing.
And long enough buffers can get limited by the d-cache loads.

For something as slow as AES you can count the number of cpu cycles for
a single call and get a reasonably consistent figure.
That will tell you whether the loop is running at the speed you might
expect it to run at.
(You need to use data dependencies between the start/end 'times' and
start/end of the code being timed, x86 lfence/mfence are too slow and
can hide the 'setup' cost of some instructions.)

	David

> 
> > +	kunit_info(test, "enc (iter. %zu, duration %lluns)",
> > +		   num_iters, t_enc);
> > +	kunit_info(test, "enc (len=%zu): %llu MB/s",
> > +		   (size_t)AES_BLOCK_SIZE,
> > +		   div64_u64((u64)AES_BLOCK_SIZE * num_iters * NSEC_PER_SEC,
> > +			     (t_enc ?: 1) * SZ_1M));
> > +
> > +	kunit_info(test, "dec (iter. %zu, duration %lluns)",
> > +		   num_iters, t_dec);
> > +	kunit_info(test, "dec (len=%zu): %llu MB/s",
> > +		   (size_t)AES_BLOCK_SIZE,
> > +		   div64_u64((u64)AES_BLOCK_SIZE * num_iters * NSEC_PER_SEC,
> > +			     (t_dec ?: 1) * SZ_1M));  
> 
> Maybe delete the first line of each pair, and switch from power-of-2
> megabytes to power-of-10?  That would be consistent with how the other
> crypto and CRC benchmarks print their output.
> 
> > +MODULE_DESCRIPTION("KUnit tests and benchmark aes library");  
> 
> "aes library" => "for the AES library"
> 
> - Eric
>