linux-kernel - Re: [PATCH v4 10/24] crypto: x86/poly

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]

Date:   Wed, 16 Nov 2022 12:13:51 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Robert Elliott <elliott@....com>
Cc:     herbert@...dor.apana.org.au, davem@...emloft.net,
        tim.c.chen@...ux.intel.com, ap420073@...il.com, ardb@...nel.org,
        David.Laight@...LAB.COM, ebiggers@...nel.org,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 10/24] crypto: x86/poly - limit FPU preemption

On Tue, Nov 15, 2022 at 10:13:28PM -0600, Robert Elliott wrote:
> +/* avoid kernel_fpu_begin/end scheduler/rcu stalls */
> +static const unsigned int bytes_per_fpu = 337 * 1024;
> +

Use an enum for constants like this:

    enum { BYTES_PER_FPU = ... };

You can even make it function-local, so it's near the code that uses it,
which will better justify its existence.

Also, where did you get this number? Seems kind of weird.

>  asmlinkage void nh_avx2(const u32 *key, const u8 *message, size_t message_len,
>  			u8 hash[NH_HASH_BYTES]);
>  
> @@ -26,18 +29,20 @@ static void _nh_avx2(const u32 *key, const u8 *message, size_t message_len,
>  static int nhpoly1305_avx2_update(struct shash_desc *desc,
>  				  const u8 *src, unsigned int srclen)
>  {
> +	BUILD_BUG_ON(bytes_per_fpu == 0);

Make the constant function local and remove this check.

> +7
>  	if (srclen < 64 || !crypto_simd_usable())
>  		return crypto_nhpoly1305_update(desc, src, srclen);
>  
> -	do {
> -		unsigned int n = min_t(unsigned int, srclen, SZ_4K);
> +	while (srclen) {

Does this add a needless additional check or does it generate better
code? Would be nice to have some explanation of the rationale.

Same comments as above apply for the rest of this patch ans series.