Message-ID: <288de217-f0ff-658c-5490-6fbf5f57f5a7@intel.com>
Date:   Mon, 8 May 2023 11:18:06 -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>, <bp@...e.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>,
        "Nathan Chancellor" <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>
Subject: Re: [PATCH v6 11/12] crypto: x86/aes-kl - Support AES algorithm using
 Key Locker instructions

On 5/5/2023 5:01 PM, Eric Biggers wrote:
> On Mon, Apr 10, 2023 at 03:59:35PM -0700, Chang S. Bae wrote:
>> [PATCH v6 11/12] crypto: x86/aes-kl - Support AES algorithm using Key Locker instructions
> 
> Thanks for dropping the unnecessary modes of operation (CBC, ECB, CTR).  It
> simplified the patchset quite a bit!

Yeah. But, there are more things to go away here as you pointed out here.

I thought some generic establishment (patch10) then introduce some 
mode-specific code (patch11). Considerably, this incremental change was 
expected to help reviewers.

Now I realize this introduces dead code on its hindsight. And this 
approach seems not helping that much.

> Now that only AES-XTS is included, can you please also merge this patch with the
> following patch?  As-is, this patch is misleading since it doesn't actually add
> "support" for anything at all.  It actually just adds an unfinished AES-XTS
> implementation, which patch 12 then finishes.  I assume that the current
> patchset organization is left over from when you were trying to support multiple
> modes of operation.  IMO, it would be much easier to review if patches 11-12
> were merged into one patch that adds the new AES-XTS implementation.

Yes, I agree to merge them.

>> For disk encryption, storage bandwidth may be the bottleneck
>> before encryption bandwidth, but the potential performance difference is
>> why AES-KL is advertised as a distinct cipher in /proc/crypto rather than
>> the kernel transparently replacing AES-NI usage with AES-KL.
> 
> This does not correctly describe what is going on.  Actually, this patchset
> registers the AES-KL XTS algorithm with the usual name "xts(aes)".  So, it can
> potentially be used by any AES-XTS user.  It seems that you're actually relying
> on the algorithm priorities to prioritize AES-NI, as you've assigned priority
> 200 to AES-KL, whereas AES-NI has priority 401.  Is that what you intend, and if
> so can you please update your explanation to properly explain this?

I think AES-KL could be a drop-in replacement for AES-NI IF it performs 
well -- something on par with AES-NI or better, AND it also supports all 
the key sizes. But, it can't be the default because that's not the case 
(at least for now).

> The alternative would be to use a unique algorithm name, such as
> "keylocker-xts(aes)".  I'm not sure that would be better, given that the
> algorithms are compatible.  However, that actually would seem to match the
> explanation you gave more closely, so perhaps that's what you actually intended?

I think those AES implementations are functionally the same to end 
users. The key envelopment is not something user-visible to my 
understanding. So, I thought that same name makes sense.

Now looking at the changelog, this text in the 'performance' section 
appears to be relevant:

  > the potential performance difference is why AES-KL is advertised as
  > a distinct cipher in /proc/crypto rather than the kernel
  > transparently replacing AES-NI usage with AES-KL.

But, this does not seem to be clear enough. Maybe, this exposition story 
can go under a new section. The changelog is already tl;dr...

> I strongly recommend skipping the 32-bit support, as it's unlikely to be worth
> the effort.
> 
> And actually, aeskl-intel_glue.c only registers the algorithm for 64-bit
> anyway...  So the 32-bit code paths are untested dead code.

Yeah, will do. Also, I'd make the module available only with X86-64. 
Then, a bit of comments for the reason should come along.

>> +static inline int aeskl_enc(const void *ctx, u8 *out, const u8 *in)
>> +{
>> +	if (unlikely(keylength(ctx) == AES_KEYSIZE_192))
>> +		return -EINVAL;
>> +	else if (!valid_keylocker())
>> +		return -ENODEV;
>> +	else if (_aeskl_enc(ctx, out, in))
>> +		return -EINVAL;
>> +	else
>> +		return 0;
>> +}
>> +
>> +static inline int aeskl_dec(const void *ctx, u8 *out, const u8 *in)
>> +{
>> +	if (unlikely(keylength(ctx) == AES_KEYSIZE_192))
>> +		return -EINVAL;
>> +	else if (!valid_keylocker())
>> +		return -ENODEV;
>> +	else if (_aeskl_dec(ctx, out, in))
>> +		return -EINVAL;
>> +	else
>> +		return 0;
>> +}
> 
> I don't think the above two functions should exist.  keylength() and
> valid_keylocker() should be checked before calling xts_crypt_common(), and the
> assembly functions should just return the correct error code (-EINVAL,
> apparently) instead of an unspecified nonzero value.  That would leave nothing
> for a wrapper function to do.
> 
> Note: if you take this suggestion, the assembly functions would need to use
> SYM_TYPED_FUNC_START instead of SYM_FUNC_START.

I thought this is something benign to stay here. But, yes, I agree that 
it is better to simplify the code.

Thanks,
Chang

Powered by blists - more mailing lists