linux-kernel - Re: [PATCH v6 10/12] crypto: x86/aes - Prepare for a new AES implementation

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:   Mon, 8 May 2023 17:55:29 -0700
From:   "Chang S. Bae" <chang.seok.bae@...el.com>
To:     Eric Biggers <ebiggers@...nel.org>
CC:     <linux-kernel@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
        <dm-devel@...hat.com>, <gmazyland@...il.com>, <luto@...nel.org>,
        <dave.hansen@...ux.intel.com>, <tglx@...utronix.de>,
        <mingo@...nel.org>, <x86@...nel.org>,
        <herbert@...dor.apana.org.au>, <ardb@...nel.org>,
        <dan.j.williams@...el.com>, <bernie.keany@...el.com>,
        <charishma1.gairuboyina@...el.com>,
        <lalithambika.krishnakumar@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v6 10/12] crypto: x86/aes - Prepare for a new AES
 implementation

On 5/5/2023 4:27 PM, Eric Biggers wrote:
> On Mon, Apr 10, 2023 at 03:59:34PM -0700, Chang S. Bae wrote:
>> Refactor the common C code to avoid code duplication. The AES-NI code uses
>> it with a function pointer argument to call back the AES-NI assembly code.
>> So will the AES-KL code.
> 
> Actually, the AES-NI XTS glue code currently makes direct calls to the assembly
> code.  This patch changes it to make indirect calls.  Indirect calls are very
> expensive these days, partly due to all the speculative execution mitigations.
> So this patch likely causes a performance regression.  How about making
> xts_crypt_common() and xts_setkey_common() be inline functions?

I guess this series is relevant:
   https://lore.kernel.org/lkml/20201222160629.22268-1-ardb@kernel.org/#t

Yeah, inlining those looks to be just a cut-and-paste work. Then I was 
curious about the performance impact.

So I picked one of my old machines. Then, I was able to quickly run 
through with these cases:

$ cryptsetup benchmark -c aes-xts -s 256

# Tests are approximate using memory only (no storage IO).
# Algorithm |       Key |      Encryption |      Decryption

Upstream (6.4-rc1)
     aes-xts        256b      3949.3 MiB/s      4014.2 MiB/s
     aes-xts        256b      4016.1 MiB/s      4011.6 MiB/s
     aes-xts        256b      4026.2 MiB/s      4018.4 MiB/s
     aes-xts        256b      4009.2 MiB/s      4006.9 MiB/s
     aes-xts        256b      4025.0 MiB/s      4016.4 MiB/s

Upstream + V6
     aes-xts        256b      3876.1 MiB/s      3963.6 MiB/s
     aes-xts        256b      3926.3 MiB/s      3984.2 MiB/s
     aes-xts        256b      3940.8 MiB/s      3961.2 MiB/s
     aes-xts        256b      3929.7 MiB/s      3984.7 MiB/s
     aes-xts        256b      3892.5 MiB/s      3942.5 MiB/s

Upstream + V6 + {inlined helpers}
     aes-xts        256b      3996.9 MiB/s      4085.4 MiB/s
     aes-xts        256b      4087.6 MiB/s      4104.9 MiB/s
     aes-xts        256b      4117.9 MiB/s      4130.2 MiB/s
     aes-xts        256b      4098.4 MiB/s      4120.6 MiB/s
     aes-xts        256b      4095.5 MiB/s      4111.5 MiB/s

Okay, I'm a bit more convinced with this inlining. Will try to comment 
this along with the change.

> Another issue with having the above be exported symbols is that their names are
> too generic, so they could easily collide with another symbols in the kernel.
> To be exported symbols, they would need something x86-specific in their names.

I think that's another good point though, they don't need to be exported 
once moved into the header so that inlined.

>>   arch/x86/crypto/Makefile           |   2 +-
>>   arch/x86/crypto/aes-intel_asm.S    |  26 ++++
>>   arch/x86/crypto/aes-intel_glue.c   | 127 ++++++++++++++++
>>   arch/x86/crypto/aes-intel_glue.h   |  44 ++++++
>>   arch/x86/crypto/aesni-intel_asm.S  |  58 +++----
>>   arch/x86/crypto/aesni-intel_glue.c | 235 +++++++++--------------------
>>   arch/x86/crypto/aesni-intel_glue.h |  17 +++
> 
> It's confusing having aes-intel, aesni-intel, *and* aeskl-intel.  Maybe call the
> first one "aes-helpers" or "aes-common" instead?

Yeah, I can see a few files named with helper. So, maybe 
s/aes-intel/aes-helpers/.

>> +struct aes_xts_ctx {
>> +	u8 raw_tweak_ctx[sizeof(struct crypto_aes_ctx)] AES_ALIGN_ATTR;
>> +	u8 raw_crypt_ctx[sizeof(struct crypto_aes_ctx)] AES_ALIGN_ATTR;
>> +}; >
> This struct does not make sense.  It should look like:
> 
>      struct aes_xts_ctx {
>              struct crypto_aes_ctx tweak_ctx AES_ALIGN_ATTR;
>              struct crypto_aes_ctx crypt_ctx AES_ALIGN_ATTR;
>      };
> 
> The runtime alignment to a 16-byte boundary should happen when translating the
> raw crypto_skcipher_ctx() into the pointer to the aes_xts_ctx.  It should not
> happen when accessing each individual field in the aes_xts_ctx.

Oh, ugly. This came from mindless copy & paste here. I guess the fix 
could be a standalone patch. Or, it can be fixed along with this mess.

>>   /*
>> - * int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
>> - *                   unsigned int key_len)
>> + * int _aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
>> + *                    unsigned int key_len)
>>    */
> 
> It's conventional to use two leading underscores, not one.

Yes, will fix.

Thanks,
Chang