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
