Re: [PATCH v6 11/12] crypto: x86/aes-kl - Support AES algorithm using Key Locker instructions

Chang S. Bae
2023-05-08T18:18:25Z

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