On 5/6/23 02:01, Eric Biggers wrote: ...
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? 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?
Sorry to be late in-game, but as this is intended for LUKS/dm-crypt use, I have a comment here: LUKS2 will no longer support algorithms with the dash in the name for dm-crypt (iow "aes-generic" or something like that will no longer work, and I am afraid you will need aes-kl/keylocker-xts here to force to use AES-KL for dm-crypt). One reason is described in https://gitlab.com/cryptsetup/cryptsetup/-/issues/809, but the major problem is that cryptsetup used CIPHER-MODE-IV syntax (that mixes badly with the dash in algorithm names). And we still rely on internal conversions of common modes to that syntax (currently it worked only by a luck). When I added the "capi" format for dm-crypt for algorithms specification, I made a mistake in that it allows everything, including crypto driver platform-specific names. The intention was to keep the kernel to decide which crypto driver will be used. So, this is perhaps fine for dm-crypt now but LUKS is a portable format, and a generic algorithm (like AES) should not depend on a specific driver or CPU feature. IOW, implement xts(aes) and let the user prioritize the driver (no changes needed for LUKS header then, AES-KL is loaded automatically) or/and create a wrapper (similar to paes, that we already support) that will force to use AES-KL (...but without the dash in the name, please :) If there is a problem with it, please create an issue for cryptsetup upstream to discuss it there (before the kernel part is merged!), so we can find some solution - I would like to avoid incompatibilities later. Thanks, Milan
