Hi, On Mon, Sep 16, 2024 at 02:27:39PM +0530, Md Sadre Alam wrote: > QCOM SDCC controller supports Inline Crypto Engine > This driver will enables inline encryption/decryption > for ICE. The algorithm supported by ICE are XTS(AES) > and CBC(AES). > > Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx> > --- > > Change in [v2] > > * Added dm-inlinecrypt driver support > > * squash the patch blk-crypto: Add additional algo modes for Inline > encryption and md: dm-crypt: Add additional algo modes for inline > encryption and added in this > > Change in [v1] > > * This patch was not included in [v1] > > block/blk-crypto.c | 21 +++ > drivers/md/Kconfig | 8 + > drivers/md/Makefile | 1 + > drivers/md/dm-inline-crypt.c | 316 +++++++++++++++++++++++++++++++++++ > include/linux/blk-crypto.h | 3 + > 5 files changed, 349 insertions(+) > create mode 100644 drivers/md/dm-inline-crypt.c Thanks for working on this! Android uses a similar device-mapper target called dm-default-key (https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c), and I've been looking for the best way to get the functionality upstream. The main challenge is that dm-default-key is integrated with fscrypt, such that if fscrypt encrypts the data, then the data isn't also encrypted with the block device key. There are also cases such as f2fs garbage collection in which filesystems read/write raw data without en/decryption by any key. So essentially a passthrough mode is supported on individual I/O requests. It looks like this patch not only does not support that, but it ignores the existence of fscrypt (or any other use of inline encryption by filesystems) entirely, and overrides any filesystem-provided key with the block device's. At the very least, this case would need to be explicitly not supported initially, i.e. dm-inlinecrypt would error out if the upper layer already provided a key. But I would like there to be an agreed-upon way to extend the code to support the pass-through mode, so that filesystem and block device level encryption work properly together. It can indeed be done with a device-mapper target (as Android does already), whether it's called "dm-inlinecrypt" or "dm-default-key", but not in a standalone way: pass-through requests also require an addition to struct bio and some support in block and filesystem code. Previously, people have said that supporting this functionality natively in the block layer would be a better fit than a dm target (e.g., see the thread https://lore.kernel.org/all/1658316391-13472-1-git-send-email-israelr@xxxxxxxxxx/T/#u). I'd appreciate people's feedback on which approach they'd prefer. Anyway, assuming the device-mapper target approach, I also have some other feedback on this patch: New algorithms should not be added in the same patch as a new dm target. Also, I do not see why there is any need for the new algorithms you are adding (AES-128-XTS, AES-128-CBC, and AES-256-CBC). XTS is preferable to CBC, and AES-256 is preferable to AES-128. AES-256-XTS is already supported, both by blk-crypto and by Qualcomm ICE. So you should just use AES-256-XTS. There are also a lot of miscellaneous issues with the proposed code. Missing ->io_hints and ->status methods, truncating IVs to 32 bits, unnecessarily using a custom algorithm name syntax that doesn't make the IV generation method explicit, unnecessarily splitting bios, incrementing IVs every 512 bytes instead of each sector, etc. I can comment on all of these in detail if you want, but to start it might be helpful to just check out dm-default-key (https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/md/dm-default-key.c) and maybe base your code on that, as it has handled these issues already. E.g., its syntax is aligned with dm-crypt's, it implements ->io_hints and ->status, and it calculates the maximum DUN correctly so that it can be determined correctly whether the inline encryption hardware can be used or not. Finally, the commit message needs to summarize what the patch does and what its motivation is. Currently it just talks about the Qualcomm ICE driver and doesn't actually say anything about dm-inlinecrypt. Yes, the motivation for dm-inlinecrypt involves being able to take advantage of inline encryption hardware such as Qualcomm ICE, but it's not explained. Thanks, - Eric
