[PATCH 0/3] crypto: adjust meaning of CRYPTO_ALG_ALLOCATES_MEMORY

[PATCH 0/3] crypto: adjust meaning of CRYPTO_ALG_ALLOCATES_MEMORY

[Giovanni Cabiddu]

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/20200722072932.GA27544@gondor.apana.org.au/
[2] https://lore.kernel.org/linux-crypto/20230523165503.GA864814@google.com/
[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

[PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Giovanni Cabiddu]

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 <herbert@gondor.apana.org.au>
Link: https://lore.kernel.org/linux-crypto/ZILvtASXQKLG43y9@gondor.apana.org.au/
Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Reviewed-by: Fiona Trahe <fiona.trahe@intel.com>
---
 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

Re: [PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Eric Biggers]

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 <herbert@gondor.apana.org.au>
> Link: https://lore.kernel.org/linux-crypto/ZILvtASXQKLG43y9@gondor.apana.org.au/
> Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Reviewed-by: Fiona Trahe <fiona.trahe@intel.com>

This needs:

    Fixes: a7a10bce8a04 ("dm integrity: don't use drivers that have CRYPTO_ALG_ALLOCATES_MEMORY")
    Cc: stable@vger.kernel.org

But, are you 100% sure the explanation in commit a7a10bce8a04 was incorrect?

- Eric

Re: [PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Giovanni Cabiddu]

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

Re: [PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Herbert Xu]

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Meenakshi Aggarwal]

Hi Herbert,

What are your plans for this change?

Thanks,
Meenakshi

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Thursday, July 6, 2023 3:28 AM
> To: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Cc: Eric Biggers <ebiggers@kernel.org>; agk@redhat.com; snitzer@kernel.org;
> linux-crypto@vger.kernel.org; dm-devel@redhat.com; linux-
> kernel@vger.kernel.org; qat-linux@intel.com; heinzm@redhat.com; Meenakshi
> Aggarwal <meenakshi.aggarwal@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Pankaj Gupta
> <pankaj.gupta@nxp.com>; Gaurav Jain <gaurav.jain@nxp.com>;
> davem@davemloft.net; Iuliana Prodan <iuliana.prodan@nxp.com>; Fiona Trahe
> <fiona.trahe@intel.com>
> 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 <herbert@gondor.apana.org.au> 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

Re: [PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Herbert Xu]

On Wed, Feb 07, 2024 at 06:22:06AM +0000, Meenakshi Aggarwal wrote:
> 
> What are your plans for this change?

I finally have a solution for you.

The answer is to use a software fallback.  As software fallbacks
do not need to allocate memory, they can be used in an OOM situation
and you fail to allocate memory in the driver with GFP_ATOMIC.

This can either be done using the existing shash interface through
SHASH_DESC_ON_STACK, or with my new hash interface where you can
use HASH_REQUEST_ON_STACK.

Once you have implemented this fallback strategy you may remove
the CRYPTO_ALG_ALLOCATES_MEMORY flag from your driver.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Meenakshi Aggarwal]

Thank you Herbert for your reply.

Will look around this.

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 18 February 2025 14:07
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: Giovanni Cabiddu <giovanni.cabiddu@intel.com>; Eric Biggers
> <ebiggers@kernel.org>; agk@redhat.com; snitzer@kernel.org; linux-
> crypto@vger.kernel.org; dm-devel@redhat.com; linux-
> kernel@vger.kernel.org; qat-linux@intel.com; heinzm@redhat.com; Horia
> Geanta <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Pankaj
> Gupta <pankaj.gupta@nxp.com>; Gaurav Jain <gaurav.jain@nxp.com>;
> davem@davemloft.net; Iuliana Prodan <iuliana.prodan@nxp.com>; Fiona
> Trahe <fiona.trahe@intel.com>
> Subject: Re: [PATCH 1/3] dm integrity: do not filter algos with
> CRYPTO_ALG_ALLOCATES_MEMORY
> 
> On Wed, Feb 07, 2024 at 06:22:06AM +0000, Meenakshi Aggarwal wrote:
> >
> > What are your plans for this change?
> 
> I finally have a solution for you.
> 
> The answer is to use a software fallback.  As software fallbacks do not need to
> allocate memory, they can be used in an OOM situation and you fail to
> allocate memory in the driver with GFP_ATOMIC.
> 
> This can either be done using the existing shash interface through
> SHASH_DESC_ON_STACK, or with my new hash interface where you can use
> HASH_REQUEST_ON_STACK.
> 
> Once you have implemented this fallback strategy you may remove the
> CRYPTO_ALG_ALLOCATES_MEMORY flag from your driver.
> 
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.
> apana.org.au%2F~herbert%2F&data=05%7C02%7Cmeenakshi.aggarwal%40n
> xp.com%7Cf6d4a8e6289b4a9dc1af08dd4ff783d3%7C686ea1d3bc2b4c6fa92c
> d99c5c301635%7C0%7C0%7C638754646705133902%7CUnknown%7CTWFpb
> GZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zM
> iIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=%2BfhgGBR
> wm7gYcwhIu5%2FIqM9H%2BypKSqy4zOG0wOwNiOw%3D&reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.
> apana.org.au%2F~herbert%2Fpubkey.txt&data=05%7C02%7Cmeenakshi.agga
> rwal%40nxp.com%7Cf6d4a8e6289b4a9dc1af08dd4ff783d3%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638754646705157205%7CUnknown%7
> CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJ
> XaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=%2
> BRonzL%2F8MSMEfp2DotM%2FkRx1bIWDStLckhzmH8nVcjc%3D&reserved=0

Re: [PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Horia Geanta]

On 7/6/2023 12:58 AM, Herbert Xu wrote:
> 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.
> 
Reviving this thread, trying to reach a conclusion.

Herbert, do you have any suggestion on how to move forward?

Is preallocating memory at crypto request allocation time worth pursuing?
This would indeed require updating the crypto API, to allow users to provide,
optionally, hints to the drivers / crypto framework wrt. memory needed
(e.g. S/G sizes):
	*_request_alloc(tfm, gfp, prealloc_hint);

Taking dm-integrity as an example, quoting Mikulas
"dm-integrity allocates arbitrarily large sg-lists when encrypting the journal"
so it's unpractical for drivers to use tfm->reqsize for memory preallocation.
OTOH, the sizes of S/Gs are known at crypto request allocation time:
create_journal
	-> dm_integrity_alloc_journal_scatterlist
	-> skcipher_request_alloc

For dm-crypt, we can't use the same logic, since crypto requests are allocated
from a mempool and *_request_alloc API is not used.
Fortunately, the S/G sizes are bounded and fairly small, thus drivers
could use tfm->reqsize to cover this case.

If the user / application logic expects no memory allocation at "runtime",
then it follows it has some information wrt. resources to be used and
either allocates crypto requests early on (dm-integrity) or prepares
a mempool (dm-crypt).
This information should be propagated to the drivers / crypto framework.
It's unreasonable to expect HW-backed implementations to avoid memory
allocations without being informed about the workload to be performed.

Thanks,
Horia

Re: [dm-devel] [PATCH 1/3] dm integrity: do not filter algos with CRYPTO_ALG_ALLOCATES_MEMORY

[Mikulas Patocka]

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 <herbert@gondor.apana.org.au>
> > Link: https://lore.kernel.org/linux-crypto/ZILvtASXQKLG43y9@gondor.apana.org.au/
> > Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> > Reviewed-by: Fiona Trahe <fiona.trahe@intel.com>
> 
> This needs:
> 
>     Fixes: a7a10bce8a04 ("dm integrity: don't use drivers that have CRYPTO_ALG_ALLOCATES_MEMORY")
>     Cc: stable@vger.kernel.org
> 
> But, are you 100% sure the explanation in commit a7a10bce8a04 was incorrect?
> 
> - Eric

Mikulas

[PATCH 2/3] crypto: api - adjust meaning of CRYPTO_ALG_ALLOCATES_MEMORY

[Giovanni Cabiddu]

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 <herbert@gondor.apana.org.au>
Suggested-by: Eric Biggers <ebiggers@google.com>
Link: https://lore.kernel.org/linux-crypto/20200722072932.GA27544@gondor.apana.org.au/
Link: https://lore.kernel.org/linux-crypto/20230523165503.GA864814@google.com/
Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Reviewed-by: Fiona Trahe <fiona.trahe@intel.com>
---
 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

Re: [PATCH 2/3] crypto: api - adjust meaning of CRYPTO_ALG_ALLOCATES_MEMORY

[Eric Biggers]

On Wed, Jul 05, 2023 at 05:40:08PM +0100, Giovanni Cabiddu wrote:

> Algorithms that do not set this flag will guarantee

"will guarantee" => "guarantee"
 
> that memory is not allocated during request processing, except in
> the avoidable exception cases described below.

"avoidable exception cases" => "exception cases"

Whether they are avoidable depends on the user.

> * Users can request an algorithm with this flag unset if they can't handle
> * memory allocation failures or sleeping during request processing.

Why add the "sleeping during request processing" part?  Isn't that controlled on
a per-request basis by CRYPTO_TFM_REQ_MAY_SLEEP which is a separate thing?

> * They should also follow the constraints below.

"should" => "must"

> + *	- 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.

"If the scatterlists contains" => "If either scatterlist contains"

Otherwise it is unclear whether this is talking about the length of each
scatterlist individually, or the sum of their lengths.

- Eric

Re: [dm-devel] [PATCH 2/3] crypto: api - adjust meaning of CRYPTO_ALG_ALLOCATES_MEMORY

[Mikulas Patocka]

On Wed, 5 Jul 2023, Eric Biggers wrote:

> On Wed, Jul 05, 2023 at 05:40:08PM +0100, Giovanni Cabiddu wrote:
> 
> > Algorithms that do not set this flag will guarantee
> 
> "will guarantee" => "guarantee"
>  
> > that memory is not allocated during request processing, except in
> > the avoidable exception cases described below.
> 
> "avoidable exception cases" => "exception cases"
> 
> Whether they are avoidable depends on the user.
> 
> > * Users can request an algorithm with this flag unset if they can't handle
> > * memory allocation failures or sleeping during request processing.
> 
> Why add the "sleeping during request processing" part?  Isn't that controlled on
> a per-request basis by CRYPTO_TFM_REQ_MAY_SLEEP which is a separate thing?
> 
> > * They should also follow the constraints below.
> 
> "should" => "must"
> 
> > + *	- 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.
> 
> "If the scatterlists contains" => "If either scatterlist contains"
> 
> Otherwise it is unclear whether this is talking about the length of each
> scatterlist individually, or the sum of their lengths.
> 
> - Eric

Hi

I wouldn't change the meaning of CRYPTO_ALG_ALLOCATES_MEMORY (because 
people will forget about this subtle change anyway).

Also note that dm-integrity allocates arbitrarily large sg-lists when 
encrypting the journal, so if you change the meaning of 
CRYPTO_ALG_ALLOCATES_MEMORY, there would be no flag left for dm-integrity 
to test.

I would introduce a new flag, something like 
CRYPTO_ALG_ALLOCATES_MEMORY_FOR_5_OR_MORE_SG_ENTRIES. dm-crypt can then 
filter the algorithms based on this flag - and the rest of the kernel code 
may stay unchanged.

Mikulas

[PATCH 3/3] crypto: qat - remove CRYPTO_ALG_ALLOCATES_MEMORY flag

[Giovanni Cabiddu]

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 <giovanni.cabiddu@intel.com>
Reviewed-by: Fiona Trahe <fiona.trahe@intel.com>
---
 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