Commit fbb6cda44190 introduced the flag CRYPTO_ALG_ALLOCATES_MEMORY.
This allows to mark algorithms that allocate memory during the datapath
so they are not used for disk encryption.
Following that, cd74693870fb limited dm-crypt to use only
implementations that don't set that flag.
After discussions in the crypto mailing list [1][2][3] about how we
could re-enable algorithms to be used by dm-crypt, we came to the
conclusion that we can change slightly the meaning of the flag
!CRYPTO_ALG_ALLOCATES_MEMORY. If an algorithm does not allocate
memory for requests with a scatterlist of 4 or less entries
(the typical case for dm-crypt), then it can avoid marking the
implementation with the flag CRYPTO_ALG_ALLOCATES_MEMORY.
This set adjusts the meaning of CRYPTO_ALG_ALLOCATES_MEMORY in the
documentation, removes the filtering for algorithms that do not
allocate memory in dm-integrity and removes the
CRYPTO_ALG_ALLOCATES_MEMORY from the algorithms registered in the QAT
driver as this is not allocating memory in the datapath for requests
with 4 or less entries in the source and destination scatterlists.
[1] https://lore.kernel.org/linux-crypto/[email protected]/
[2] https://lore.kernel.org/linux-crypto/[email protected]/
[3] https://lore.kernel.org/linux-crypto/Ysw9E2Az2oK4jfCf@lucas-Virtual-Machine/
Giovanni Cabiddu (3):
dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY
crypto: api - adjust meaning of CRYPTO_ALG_ALLOCATES_MEMORY
crypto: qat - remove CRYPTO_ALG_ALLOCATES_MEMORY flag
drivers/crypto/intel/qat/qat_common/qat_algs.c | 13 ++++++-------
.../intel/qat/qat_common/qat_comp_algs.c | 2 +-
drivers/md/dm-integrity.c | 2 +-
include/linux/crypto.h | 18 ++++++++++++++++--
4 files changed, 24 insertions(+), 11 deletions(-)
--
2.40.1
The CRYPTO_ALG_ALLOCATES_MEMORY flag doesn't allow to distinguish
between implementations which don't allocate memory for scatterlists
with 4 or less entries (the typical case for dm-crypt) and those that
do.
The flag's meaning is adjusted based on the ML discussion below.
This patch removes the need to set the flag if the implementation can
handle scatterlists up to 4 entries without allocating memory.
The documentation is updated accordingly, with an extra clarification
regarding sleeping.
Suggested-by: Herbert Xu <[email protected]>
Suggested-by: Eric Biggers <[email protected]>
Link: https://lore.kernel.org/linux-crypto/[email protected]/
Link: https://lore.kernel.org/linux-crypto/[email protected]/
Signed-off-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Fiona Trahe <[email protected]>
---
include/linux/crypto.h | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 31f6fee0c36c..15884790a3d0 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -87,8 +87,13 @@
/*
* The algorithm may allocate memory during request processing, i.e. during
- * encryption, decryption, or hashing. Users can request an algorithm with this
- * flag unset if they can't handle memory allocation failures.
+ * encryption, decryption, or hashing. Algorithms that do not set this flag will
+ * guarantee that memory is not allocated during request processing, except in
+ * the avoidable exception cases described below.
+ *
+ * Users can request an algorithm with this flag unset if they can't handle
+ * memory allocation failures or sleeping during request processing. They should
+ * also follow the constraints below.
*
* This flag is currently only implemented for algorithms of type "skcipher",
* "aead", "ahash", "shash", and "cipher". Algorithms of other types might not
@@ -102,6 +107,9 @@
* - If the data were to be divided into chunks of size
* crypto_skcipher_walksize() (with any remainder going at the end), no
* chunk can cross a page boundary or a scatterlist element boundary.
+ * - The input and output scatterlists must have no more than 4 entries.
+ * If the scatterlists contain more than 4 entries, the algorithm may
+ * allocate memory.
* aead:
* - The IV buffer and all scatterlist elements must be aligned to the
* algorithm's alignmask.
@@ -110,10 +118,16 @@
* - If the plaintext/ciphertext were to be divided into chunks of size
* crypto_aead_walksize() (with the remainder going at the end), no chunk
* can cross a page boundary or a scatterlist element boundary.
+ * - The input and output scatterlists must have no more than 4 entries.
+ * If the scatterlists contain more than 4 entries, the algorithm may
+ * allocate memory.
* ahash:
* - The result buffer must be aligned to the algorithm's alignmask.
* - crypto_ahash_finup() must not be used unless the algorithm implements
* ->finup() natively.
+ * - The input and output scatterlists must have no more than 4 entries.
+ * If the scatterlists contain more than 4 entries, the algorithm may
+ * allocate memory.
*/
#define CRYPTO_ALG_ALLOCATES_MEMORY 0x00010000
--
2.40.1
Remove the CRYPTO_ALG_ALLOCATES_MEMORY flag from the aead, skcipher
and acomp alg structures since the driver does not allocate memory in
the request processing for scatterlists with 4 or less entries.
This allows the QAT driver to be used by dm-crypt.
Signed-off-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Fiona Trahe <[email protected]>
---
drivers/crypto/intel/qat/qat_common/qat_algs.c | 13 ++++++-------
drivers/crypto/intel/qat/qat_common/qat_comp_algs.c | 2 +-
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/intel/qat/qat_common/qat_algs.c b/drivers/crypto/intel/qat/qat_common/qat_algs.c
index 3c4bba4a8779..a7a6ac33052a 100644
--- a/drivers/crypto/intel/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/intel/qat/qat_common/qat_algs.c
@@ -1278,7 +1278,7 @@ static struct aead_alg qat_aeads[] = { {
.cra_name = "authenc(hmac(sha1),cbc(aes))",
.cra_driver_name = "qat_aes_cbc_hmac_sha1",
.cra_priority = 4001,
- .cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_ALLOCATES_MEMORY,
+ .cra_flags = CRYPTO_ALG_ASYNC,
.cra_blocksize = AES_BLOCK_SIZE,
.cra_ctxsize = sizeof(struct qat_alg_aead_ctx),
.cra_module = THIS_MODULE,
@@ -1295,7 +1295,7 @@ static struct aead_alg qat_aeads[] = { {
.cra_name = "authenc(hmac(sha256),cbc(aes))",
.cra_driver_name = "qat_aes_cbc_hmac_sha256",
.cra_priority = 4001,
- .cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_ALLOCATES_MEMORY,
+ .cra_flags = CRYPTO_ALG_ASYNC,
.cra_blocksize = AES_BLOCK_SIZE,
.cra_ctxsize = sizeof(struct qat_alg_aead_ctx),
.cra_module = THIS_MODULE,
@@ -1312,7 +1312,7 @@ static struct aead_alg qat_aeads[] = { {
.cra_name = "authenc(hmac(sha512),cbc(aes))",
.cra_driver_name = "qat_aes_cbc_hmac_sha512",
.cra_priority = 4001,
- .cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_ALLOCATES_MEMORY,
+ .cra_flags = CRYPTO_ALG_ASYNC,
.cra_blocksize = AES_BLOCK_SIZE,
.cra_ctxsize = sizeof(struct qat_alg_aead_ctx),
.cra_module = THIS_MODULE,
@@ -1330,7 +1330,7 @@ static struct skcipher_alg qat_skciphers[] = { {
.base.cra_name = "cbc(aes)",
.base.cra_driver_name = "qat_aes_cbc",
.base.cra_priority = 4001,
- .base.cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_ALLOCATES_MEMORY,
+ .base.cra_flags = CRYPTO_ALG_ASYNC,
.base.cra_blocksize = AES_BLOCK_SIZE,
.base.cra_ctxsize = sizeof(struct qat_alg_skcipher_ctx),
.base.cra_alignmask = 0,
@@ -1348,7 +1348,7 @@ static struct skcipher_alg qat_skciphers[] = { {
.base.cra_name = "ctr(aes)",
.base.cra_driver_name = "qat_aes_ctr",
.base.cra_priority = 4001,
- .base.cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_ALLOCATES_MEMORY,
+ .base.cra_flags = CRYPTO_ALG_ASYNC,
.base.cra_blocksize = 1,
.base.cra_ctxsize = sizeof(struct qat_alg_skcipher_ctx),
.base.cra_alignmask = 0,
@@ -1366,8 +1366,7 @@ static struct skcipher_alg qat_skciphers[] = { {
.base.cra_name = "xts(aes)",
.base.cra_driver_name = "qat_aes_xts",
.base.cra_priority = 4001,
- .base.cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK |
- CRYPTO_ALG_ALLOCATES_MEMORY,
+ .base.cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK,
.base.cra_blocksize = AES_BLOCK_SIZE,
.base.cra_ctxsize = sizeof(struct qat_alg_skcipher_ctx),
.base.cra_alignmask = 0,
diff --git a/drivers/crypto/intel/qat/qat_common/qat_comp_algs.c b/drivers/crypto/intel/qat/qat_common/qat_comp_algs.c
index b533984906ec..bd1383da1c4a 100644
--- a/drivers/crypto/intel/qat/qat_common/qat_comp_algs.c
+++ b/drivers/crypto/intel/qat/qat_common/qat_comp_algs.c
@@ -442,7 +442,7 @@ static struct acomp_alg qat_acomp[] = { {
.cra_name = "deflate",
.cra_driver_name = "qat_deflate",
.cra_priority = 4001,
- .cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_ALLOCATES_MEMORY,
+ .cra_flags = CRYPTO_ALG_ASYNC,
.cra_ctxsize = sizeof(struct qat_compression_ctx),
.cra_module = THIS_MODULE,
},
--
2.40.1
The flag CRYPTO_ALG_ALLOCATES_MEMORY indicates that an algorithm might
allocate memory in the datapath and therefore sleep.
Dm-integrity is filtering out implementations of skcipher algorithms
that have this flag set. However, in the same function it does
allocations with GFP_KERNEL.
As dm-integrity is re-entrant and capable of handling sleeps that could
occur during allocations with GFP_KERNEL, then it is also capable of
using skcipher algorithm implementations that have
CRYPTO_ALG_ALLOCATES_MEMORY set.
Remove the filtering of skcipher implementations with the flag
CRYPTO_ALG_ALLOCATES_MEMORY set.
Suggested-by: Herbert Xu <[email protected]>
Link: https://lore.kernel.org/linux-crypto/[email protected]/
Signed-off-by: Giovanni Cabiddu <[email protected]>
Reviewed-by: Fiona Trahe <[email protected]>
---
drivers/md/dm-integrity.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 31838b13ea54..a1013eff01b4 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -3785,7 +3785,7 @@ static int create_journal(struct dm_integrity_c *ic, char **error)
struct journal_completion comp;
comp.ic = ic;
- ic->journal_crypt = crypto_alloc_skcipher(ic->journal_crypt_alg.alg_string, 0, CRYPTO_ALG_ALLOCATES_MEMORY);
+ ic->journal_crypt = crypto_alloc_skcipher(ic->journal_crypt_alg.alg_string, 0, 0);
if (IS_ERR(ic->journal_crypt)) {
*error = "Invalid journal cipher";
r = PTR_ERR(ic->journal_crypt);
--
2.40.1
On Wed, Jul 05, 2023 at 05:40:07PM +0100, Giovanni Cabiddu wrote:
> The flag CRYPTO_ALG_ALLOCATES_MEMORY indicates that an algorithm might
> allocate memory in the datapath and therefore sleep.
> Dm-integrity is filtering out implementations of skcipher algorithms
> that have this flag set. However, in the same function it does
> allocations with GFP_KERNEL.
Which function is the above referring to? The actual encryption/decryption
happens in crypt_journal(), and I don't see any memory allocations there.
> As dm-integrity is re-entrant and capable of handling sleeps that could
> occur during allocations with GFP_KERNEL, then it is also capable of
> using skcipher algorithm implementations that have
> CRYPTO_ALG_ALLOCATES_MEMORY set.
>
> Remove the filtering of skcipher implementations with the flag
> CRYPTO_ALG_ALLOCATES_MEMORY set.
What about the use of CRYPTO_ALG_ALLOCATES_MEMORY in get_mac()?
>
> Suggested-by: Herbert Xu <[email protected]>
> Link: https://lore.kernel.org/linux-crypto/[email protected]/
> Signed-off-by: Giovanni Cabiddu <[email protected]>
> Reviewed-by: Fiona Trahe <[email protected]>
This needs:
Fixes: a7a10bce8a04 ("dm integrity: don't use drivers that have CRYPTO_ALG_ALLOCATES_MEMORY")
Cc: [email protected]
But, are you 100% sure the explanation in commit a7a10bce8a04 was incorrect?
- Eric
Thanks Eric.
On Wed, Jul 05, 2023 at 01:12:05PM -0700, Eric Biggers wrote:
> On Wed, Jul 05, 2023 at 05:40:07PM +0100, Giovanni Cabiddu wrote:
> > The flag CRYPTO_ALG_ALLOCATES_MEMORY indicates that an algorithm might
> > allocate memory in the datapath and therefore sleep.
> > Dm-integrity is filtering out implementations of skcipher algorithms
> > that have this flag set. However, in the same function it does
> > allocations with GFP_KERNEL.
>
> Which function is the above referring to? The actual encryption/decryption
> happens in crypt_journal(), and I don't see any memory allocations there.
You are right. I was referring to create_journal() which is allocating
memory right before calling do_crypt().
However, I didn't consider crypt_journal() which might not be allocating
memory before calling do_crypt().
Then we are then back to square one. We need to check how many entries
are present in the scatterlists encrypted by crypt_journal() before
adjusting the meaning of !CRYPTO_ALG_ALLOCATES_MEMORY.
Regards,
--
Giovanni
On Wed, Jul 05, 2023 at 09:57:54PM +0100, Giovanni Cabiddu wrote:
>
> Then we are then back to square one. We need to check how many entries
> are present in the scatterlists encrypted by crypt_journal() before
> adjusting the meaning of !CRYPTO_ALG_ALLOCATES_MEMORY.
Indeed. I missed the fact that it was preallocating memory with
GFP_KERNEL.
So perhaps the answer is to adjust our API to allow the drivers to
pre-allocate memory. I'll look into this.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi
If you allocate memory in crypto processing in dm-integrity, you risk the
low-memory deadlock when swapping to dm-integrity.
I.e. the machine runs out of memory, it needs to swap out pages to free
some memory, the swap-out bio goes to dm-integrity and dm-integrity calls
the crypto API and tries to allocate more memory => deadlock.
On Wed, 5 Jul 2023, Eric Biggers wrote:
> On Wed, Jul 05, 2023 at 05:40:07PM +0100, Giovanni Cabiddu wrote:
> > The flag CRYPTO_ALG_ALLOCATES_MEMORY indicates that an algorithm might
> > allocate memory in the datapath and therefore sleep.
> > Dm-integrity is filtering out implementations of skcipher algorithms
> > that have this flag set. However, in the same function it does
> > allocations with GFP_KERNEL.
It's OK to use GFP_KERNEL in the device mapper target constructor (because
at this point there is no I/O going to the device). But it's not OK to use
it for individual bio processing.
> Which function is the above referring to? The actual encryption/decryption
> happens in crypt_journal(), and I don't see any memory allocations there.
>
> > As dm-integrity is re-entrant and capable of handling sleeps that could
> > occur during allocations with GFP_KERNEL, then it is also capable of
> > using skcipher algorithm implementations that have
> > CRYPTO_ALG_ALLOCATES_MEMORY set.
> >
> > Remove the filtering of skcipher implementations with the flag
> > CRYPTO_ALG_ALLOCATES_MEMORY set.
>
> What about the use of CRYPTO_ALG_ALLOCATES_MEMORY in get_mac()?
>
> >
> > Suggested-by: Herbert Xu <[email protected]>
> > Link: https://lore.kernel.org/linux-crypto/[email protected]/
> > Signed-off-by: Giovanni Cabiddu <[email protected]>
> > Reviewed-by: Fiona Trahe <[email protected]>
>
> This needs:
>
> Fixes: a7a10bce8a04 ("dm integrity: don't use drivers that have CRYPTO_ALG_ALLOCATES_MEMORY")
> Cc: [email protected]
>
> But, are you 100% sure the explanation in commit a7a10bce8a04 was incorrect?
>
> - Eric
Mikulas
Hi Herbert,
What are your plans for this change?
Thanks,
Meenakshi
> -----Original Message-----
> From: Herbert Xu <[email protected]>
> Sent: Thursday, July 6, 2023 3:28 AM
> To: Giovanni Cabiddu <[email protected]>
> Cc: Eric Biggers <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected]; Meenakshi
> Aggarwal <[email protected]>; Horia Geanta
> <[email protected]>; Varun Sethi <[email protected]>; Pankaj Gupta
> <[email protected]>; Gaurav Jain <[email protected]>;
> [email protected]; Iuliana Prodan <[email protected]>; Fiona Trahe
> <[email protected]>
> Subject: Re: [PATCH 1/3] dm integrity: do not filter algos with
> CRYPTO_ALG_ALLOCATES_MEMORY
>
> On Wed, Jul 05, 2023 at 09:57:54PM +0100, Giovanni Cabiddu wrote:
> >
> > Then we are then back to square one. We need to check how many entries
> > are present in the scatterlists encrypted by crypt_journal() before
> > adjusting the meaning of !CRYPTO_ALG_ALLOCATES_MEMORY.
>
> Indeed. I missed the fact that it was preallocating memory with GFP_KERNEL.
>
> So perhaps the answer is to adjust our API to allow the drivers to pre-allocate
> memory. I'll look into this.
>
> Thanks,
> --
> Email: Herbert Xu <[email protected]> Home Page:
> http://gondor.ap/
> ana.org.au%2F~herbert%2F&data=05%7C01%7Cmeenakshi.aggarwal%40nxp.co
> m%7C59d63c0b42d5423abb1108db7da2e431%7C686ea1d3bc2b4c6fa92cd99c5
> c301635%7C0%7C0%7C638241910806938399%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000%7C%7C%7C&sdata=vdLCyhQOSTEhIK1%2FkAO7Z%2Fu6ejrLbRHwM88N
> DmsqaP0%3D&reserved=0
> PGP Key:
> http://gondor.ap/
> ana.org.au%2F~herbert%2Fpubkey.txt&data=05%7C01%7Cmeenakshi.aggarwal
> %40nxp.com%7C59d63c0b42d5423abb1108db7da2e431%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C638241910806938399%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=eAkggsD8FaJzb9OO2p1bcaPYs8xt47Eav
> UdVVssGM7o%3D&reserved=0