[SOLVED] Be aware, marvell_cesa no more used by dm_crypt because of potential deadlocks

I've noticed a performance drop using LUKS in recent kernel (>=5.9) with marvell_cesa acceleration. After some investigation I found that marvell_cesa is not used anymore by dm_crypt because of potential deadlocks.
Even if "cryptsetup benchmark" shows the HW accelerated speed, the performance writing and reading on a disk are poor, aligned to software only encryption.

For more information, please refer to:
[REGRESSION] dm_crypt essiv ciphers do not use async driver mv-aes-cbc anymore
https://lore.kernel.org/all/20230929224327.GA11839@google.com/T/

*** edit ***
Following the information on the regression, I've created the attached patch for kernel 6.6.15, but it doesn't work for me.
even if cryptsetup benchmark shows the hardware accelerated performace, if I try a direct write on disk, it isn't accelerated by hardware.

on kernel 5.5.3
cryptsetup benchmark --cipher aes-cbc
# Tests are approximate using memory only (no storage IO).
# Algorithm | Key | Encryption | Decryption
aes-cbc 256b 79.7 MiB/s 81.3 MiB/s

cryptsetup benchmark --cipher aes-xts
# Tests are approximate using memory only (no storage IO).
# Algorithm | Key | Encryption | Decryption
aes-xts 256b 72.3 MiB/s 76.6 MiB/s

dmsetup table sda-crypt
...omissis... crypt aes-xts-plain64 ...omissis...
dd if=/dev/zero of=/dev/mapper/sda-crypt bs=5M count=100
100+0 records in
100+0 records out
524288000 bytes (524 MB, 500 MiB) copied, 9.04862 s, 57.9 MB/s

dmsetup table sdb-crypt
...omissis... crypt aes-cbc-essiv:sha256 ...omissis...

dd if=/dev/zero of=/dev/mapper/sdb-crypt bs=5M count=100
100+0 records in
100+0 records out
524288000 bytes (524 MB, 500 MiB) copied, 7.344 s, 71.4 MB/s
dd if=/dev/zero of=/dev/mapper/sdb-crypt bs=5M count=100


on kernel 6.6.15 patched
cryptsetup benchmark --cipher aes-cbc
# Tests are approximate using memory only (no storage IO).
# Algorithm | Key | Encryption | Decryption
aes-cbc 256b 68.5 MiB/s 78.0 MiB/s

cryptsetup benchmark --cipher aes-xts
# Tests are approximate using memory only (no storage IO).
# Algorithm | Key | Encryption | Decryption
aes-xts 256b 72.6 MiB/s 74.0 MiB/s

dmsetup table sda-crypt
...omissis... crypt aes-xts-plain64 ...omissis...

dd if=/dev/zero of=/dev/mapper/sda-crypt bs=5M count=100
100+0 records in
100+0 records out
524288000 bytes (524 MB, 500 MiB) copied, 14.9565 s, 35.1 MB/s

dmsetup table sdb-crypt
...omissis... crypt aes-cbc-essiv:sha256 ...omissis...

dd if=/dev/zero of=/dev/mapper/sdb-crypt bs=5M count=100
100+0 records in
100+0 records out
524288000 bytes (524 MB, 500 MiB) copied, 11.1328 s, 47.1 MB/s

on kernel 6.6.15 without patch also the writing using aes-cbc-essiv:sha256 is about 35 mb/s. Maybe the patch is working but there is something else to restore, but what? Any idea?



Edited 10 time(s). Last edit at 03/04/2024 03:24PM by CyberPK.

CyberPK,

I'm hesitate to restore the driver to what it was before, given what was said in the email thread.

Quote
From: Mikulas Patocka

The driver drivers/crypto/marvell/cesa/cipher.c uses GFP_ATOMIC
allocations (see mv_cesa_skcipher_dma_req_init). So, it is not really safe
to use it for dm-crypt.

GFP_ATOMIC allocations may fail anytime (for example, they fill fail if
the machine receives too many network packets in a short timeframe and
runs temporarily out of memory). And when the GFP_ATOMIC allocation fails,
you get a write I/O error and data corruption.

It could be possible to change it to use GFP_NOIO allocations, then we
would risk deadlock instead of data corruption. The best thing would be to
convert the driver to use mempools.

I don't think we'd want to take a chance having the risk of data corruption.

-bodhi
===========================
Forum Wiki
bodhi's corner (buy bodhi a beer)

Yes, I've read the warnings.
I think it should be investigated because until now there were no report of data corruption/deadlocks on encrypted drives using Marvel CESA engine, so I think that the problem is more theorical then real. Also, if the problem is not fixed, should be made clear to all that the hardware acceleration is not used anymore by dm-crypt, and the encrypted reading/writing will not beneficiate.

CyberPK Wrote:
-------------------------------------------------------
> Yes, I've read the warnings.
> I think it should be investigated because until
> now there were no report of data
> corruption/deadlocks on encrypted drives using
> Marvel CESA engine, so I think that the problem is
> more theorical then real. Also, if the problem is
> not fixed, should be made clear to all that the
> hardware acceleration is not used anymore by
> dm-crypt, and the encrypted reading/writing will
> not beneficiate.

I'll be monitoring this, but only for encryption matters other than drive encryption. are any of the algos supported by the cesa module still in active use for anything else in a standard debian (or gentoo, for that matter) install? I know this was supposedly something openssl could make use of, and possibly openssh, and jdk is also something that could have benefitted from it, but if this isn't something that is currently usable by anything it would be best just to ignore it.

I made a post about half an hour ago about how the bookworm rootfs doesn't include marvell_cesa enabled by default (aside from a couple other modules) and this by itself might be a blessing in disguise, no need to disable it on fresh installs at the current if it's not already enabled from the get-go.

I've only heard edge cases of disk encryption being used and not even on debian, but on OSes like OpenWRT and very sparsely at that given the kirkwood encryption stuff is in a separate package for space conservation. if this gets fixed to use mempools that's fine and dandy, but even if it doesn't get fixed, if nothing is making clear use of the crypto engines on-die then I don't see any issue with leaving it as a warning and not enabling it by default going forward.



Edited 1 time(s). Last edit at 02/23/2024 04:10AM by sudos.

Some background information in case anyone else is wondering

Initial short problem description and consideration in [dm-devel] crypto API and GFP_ATOMIC
https://listman.redhat.com/archives/dm-devel/2020-June/041316.html

Led to solution [dm-devel] [PATCH 1/4] crypto: introduce CRYPTO_ALG_ALLOCATES_MEMORY
https://listman.redhat.com/archives/dm-devel/2020-June/041341.html

fbb6cda44190 crypto: algapi - introduce the flag CRYPTO_ALG_ALLOCATES_MEMORY
b8aa7dc5c753 crypto: drivers - set the flag CRYPTO_ALG_ALLOCATES_MEMORY
cd74693870fb dm crypt: don't use drivers that have CRYPTO_ALG_ALLOCATES_MEMORY
(a7a10bce8a04 ("dm integrity: don't use drivers that have CRYPTO_ALG_ALLOCATES_MEMORY"))

>[PATCH v2 0/7] crypto: add CRYPTO_ALG_ALLOCATES_MEMORY
https://lore.kernel.org/linux-crypto/20200710062042.113842-1-ebiggers@kernel.org/

7bcb2c99f8ed crypto: algapi - use common mechanism for inheriting flags
2eb27c11937e crypto: algapi - add NEED_FALLBACK to INHERITED_FLAGS


One example where memory corruption did occur with another accelerator

>Intel QAT on A2SDi-8C-HLN4F causes massive data corruption with dm-crypt + xfs
https://lore.kernel.org/all/CACsaVZ+mt3CfdXV0_yJh7d50tRcGcRZ12j3n6-hoX2cz3+njsg@mail.gmail.com/T/

Discussion on how to re-enable
Intel (QAT) https://lore.kernel.org/lkml/20230705164009.58351-2-giovanni.cabiddu@intel.com/T/
NXP (CAAM) https://lore.kernel.org/linux-crypto/20230523153421.1528359-1-meenakshi.aggarwal@nxp.com/

edit: I think [dm-devel] crypto API and GFP_ATOMIC covers the problem short and precise together with the Mikulas Patocka quote in this thread, the rest is just if you need more information.

edit: https://wiki.kobol.io/helios4/cesa/ reverts the patch in case anyone has a use case where the memory corruption risk isnt a problem.



Edited 4 time(s). Last edit at 02/23/2024 08:00AM by someguys561235.

@someguys561235

Thanks!

-bodhi
===========================
Forum Wiki
bodhi's corner (buy bodhi a beer)

someguys561235 Wrote:
-------------------------------------------------------

> edit: https://wiki.kobol.io/helios4/cesa/ reverts
> the patch in case anyone has a use case where the
> memory corruption risk isnt a problem.

I was not aware of the patch proposed on the page that you linked.
The patch I've proposed on the first post is more aggressive because impact also hash.c and essiv.c, but I cannot understand why the performance gap is still present.
Maybe the speed testing method I'm using is wrong (dd if=/dev/zero of=/dev/mapper/sdX-crypt bs=5M count=100)?
I had no time to test other patches, so I'm still stuck. Any idea?

**edit**
sudos Wrote:
-------------------------------------------------------
> ...but even if it
> doesn't get fixed, if nothing is making clear use
> of the crypto engines on-die then I don't see any
> issue with leaving it as a warning and not
> enabling it by default going forward.

I would make clear that even if cesa appear to be working when testing with crypsetup benchmark, it is not used for disk encryption by dm-crypt



Edited 2 time(s). Last edit at 02/26/2024 01:46AM by CyberPK.

Today I've brutally tried the very aggressive attached patch on kernel 6.7.6 to fully disable CRYPTO_ALG_ALLOCATES_MEMORY flag but still getting 40mb/s on aes-xts (instead of about 73mb/s) and 53 mb/s on aes-cbc instead of about 80mb/s.
There should be something else impacting the performance. Any idea?



Edited 1 time(s). Last edit at 02/26/2024 01:31PM by CyberPK.

CyberPK,

I've re-uploaded linux-5.3.5-mvebu-tld-1 tarball. See

Old kernel and rootfs releases.

Quote

Updated 12 Oct 2019:

Kernel linux-5.3.5-mvebu-tld-1 package has been uploaded.
......

Download at Dropbox

linux-5.3.5-mvebu-tld-1-bodhi.tar.bz2 (This tarball was re-uploaded on 01 Mar 2024)

-bodhi
===========================
Forum Wiki
bodhi's corner (buy bodhi a beer)

bodhi Wrote:
-------------------------------------------------------
> I've re-uploaded linux-5.3.5-mvebu-tld-1 tarball.
> See
>
> Old
> kernel and rootfs releases.

Thank you very much!

Using kernel 5.4.270, I can measure the expected performance.
cryptsetup benchmark --cipher aes-cbc
# Tests are approximate using memory only (no storage IO).
# Algorithm | Key | Encryption | Decryption
aes-cbc 256b 79.0 MiB/s 81.0 MiB/s

cryptsetup benchmark --cipher aes-cbc
# Tests are approximate using memory only (no storage IO).
# Algorithm | Key | Encryption | Decryption
aes-xts 256b 72.7 MiB/s 76.5 MiB/s


using aes-cbc
dd if=/dev/zero of=/dev/mapper/sdX-crypt bs=5M count=100
100+0 records in
100+0 records out
524288000 bytes (524 MB, 500 MiB) copied, 7.55462 s, 69.4 MB/s

using aes-xts
dd if=/dev/zero of=/dev/mapper/sdX-crypt bs=5M count=100
100+0 records in
100+0 records out
524288000 bytes (524 MB, 500 MiB) copied, 10.4226 s, 50.3 MB/s

Any idea on what can affect the performances on kernel 6.7.y even if patched to remove the new flag CRYPTO_ALG_ALLOCATES_MEMORY?



Edited 3 time(s). Last edit at 03/02/2024 03:01PM by CyberPK.

> Any idea on what can affect the performances on
> kernel 6.7.y even if patched to remove the new
> flag CRYPTO_ALG_ALLOCATES_MEMORY?

Perhaps it might be that the whole patch must be reversed, not just the part about CRYPTO_ALG_ALLOCATES_MEMORY.

-bodhi
===========================
Forum Wiki
bodhi's corner (buy bodhi a beer)

After countless attempts, I found the commit impacting the performances: 28ee8b0912ca2ff68c2c03ff97bf1c22634c7942
Ironically it's reported a 2x speed increase, but it's not true for me.

Apply both the attached patches tested and adapted to work on kernel 6.7.7; cesa_reenable as patch, 28338b... as patch reverse (-R).
Consider also compiling using
CONFIG_VFP=y
CONFIG_NEON=y
CONFIG_KERNEL_MODE_NEON=y


*** BE AWARE ***
This patch could result in a memory corruption



Edited 1 time(s). Last edit at 03/04/2024 05:19PM by CyberPK.