diff mbox series

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

Message ID 20230705164009.58351-2-giovanni.cabiddu@intel.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series crypto: adjust meaning of CRYPTO_ALG_ALLOCATES_MEMORY | expand

Commit Message

Cabiddu, Giovanni July 5, 2023, 4:40 p.m. UTC 
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(-)

Comments

Eric Biggers July 5, 2023, 8:12 p.m. UTC | #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 <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
Cabiddu, Giovanni July 5, 2023, 8:57 p.m. UTC | #2 
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,
Herbert Xu July 5, 2023, 9:57 p.m. UTC | #3 
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,
Mikulas Patocka July 7, 2023, 10:25 a.m. UTC | #4 
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
Meenakshi Aggarwal Feb. 7, 2024, 6:22 a.m. UTC | #5 
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
Horia Geanta July 4, 2024, 10:39 a.m. UTC | #6 
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
diff mbox series

Patch

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);