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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <76089e1f-dfc9-44e8-8e16-b965cd43d848@linux.ibm.com>
Date: Thu, 15 Jan 2026 22:51:25 +0100
From: Holger Dengler <dengler@...ux.ibm.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: 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 15/01/2026 21:43, Eric Biggers 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.

Ok, I'll move the explanation to the commit message. I assume that the example
output of the test can be dropped?

>> 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.

This is a left-over, will be removed in the next series.

> 
>> +static const struct aes_testvector aes128_kat = {
> 
> Where do these test vectors come from?  All test vectors should have a
> documented source.

ok, I will add this information as well.

>> +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.
> 
> 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.

As we have a fixed length, I would go stay with a fix value for the iterations
(instead of calculating it based on len).

The benchmark has a separate loop for encrypt and decrypt, so I will do the
half iterations on encrypt and the other half on decrypt. I will also reduce
the iterations for the warm-ups.

What about 100 iterations for each warm-up and 500.000 iterations for each
real measurement? Means processing 2x 8MiB with preemption disabled.

>> +	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"

ok

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@...ux.ibm.com