Deadlock when using crypto API for block devices

Deadlock when using crypto API for block devices

[Mikulas Patocka]

Hi

There's a deadlock reported here: 
https://bugzilla.kernel.org/show_bug.cgi?id=200835

* dm-crypt calls crypt_convert in xts mode
* init_crypt from xts.c calls kmalloc(GFP_KERNEL)
* kmalloc(GFP_KERNEL) recurses into the XFS filesystem, the filesystem 
	tries to submit some bios and wait for them, causing a deadlock

I was thinking how to fix it, there are several possibilities, but all 
have some caveats. So, I'd like to ask you as crypto code maintainers, 
what's the appropriate solution.


1. don't set CRYPTO_TFM_REQ_MAY_SLEEP in dm-crypt
=================================================
If we don't set it, dm-crypt will use GFP_ATOMIC and GFP_ATOMIC may fail. 
The function init_crypt in xts.c handles the failure gracefully - but the 
question is - does the whole crypto code handle allocation failures 
gracefully? If not and if it returns -ENOMEM somewhere, it would result in 
I/O errors and data corruption.

I'd like to ask if you as maintainers of the crypto API could say, whether 
the crypto code handles GFP_ATOMIC allocations failure gracefully or not.

Note that dm-crypt currently uses encryption, hashes and authernticated 
encryption, so if we go down this path, we must make sure that the code 
for these operations will never return -ENOMEM.

2. use memalloc_noio_save/memalloc_noio_restore in dm-crypt
===========================================================
This makes all allocations use GFP_NOIO implicitly, so it would fix the 
deadlock. But - the crypto API can offload encryption to the cryptd 
thread, and if the offload happens, the memalloc_noio_save flag is lost 
and the allocation can still be done with GFP_KERNEL and cause a deadlock.

3. introduce new flag CRYPTO_TFM_REQ_MAY_SLEEP_NOIO
===================================================
Would you like to introduce it?

4. make the whole crypto code use GFP_NOIO instead of GFP_KERNEL?
=================================================================


... any other suggestions?

Mikulas

Re: Deadlock when using crypto API for block devices

[Herbert Xu]

On Thu, Aug 23, 2018 at 04:39:23PM -0400, Mikulas Patocka wrote:
>
> 1. don't set CRYPTO_TFM_REQ_MAY_SLEEP in dm-crypt
> =================================================
> If we don't set it, dm-crypt will use GFP_ATOMIC and GFP_ATOMIC may fail. 
> The function init_crypt in xts.c handles the failure gracefully - but the 
> question is - does the whole crypto code handle allocation failures 
> gracefully? If not and if it returns -ENOMEM somewhere, it would result in 
> I/O errors and data corruption.

It is safe to use GFP_ATOMIC.  First of the allocation is really
small (less than a page) so it will only fail when the system is
almost completely out of memory.  Even when it does fail the crypto
operation will still succeed, albeit it will process things at a
smaller granularity so the performance will degrade.

The sleeping part of that flag is also not an issue because it
only kicks in once per page.  As this is going to be less than
or equal to a page it shouldn't matter.
 
> 3. introduce new flag CRYPTO_TFM_REQ_MAY_SLEEP_NOIO
> ===================================================
> Would you like to introduce it?

For now I don't think this is necessary given that this allocation
and similar ones in lrw and other places in the crypto API are just
performance enhancements and unlikely to fail except in very dire
situations.

But if new problems arise I'm certainly not opposed to 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: Deadlock when using crypto API for block devices

[Mikulas Patocka]

On Fri, 24 Aug 2018, Herbert Xu wrote:

> On Thu, Aug 23, 2018 at 04:39:23PM -0400, Mikulas Patocka wrote:
> >
> > 1. don't set CRYPTO_TFM_REQ_MAY_SLEEP in dm-crypt
> > =================================================
> > If we don't set it, dm-crypt will use GFP_ATOMIC and GFP_ATOMIC may fail. 
> > The function init_crypt in xts.c handles the failure gracefully - but the 
> > question is - does the whole crypto code handle allocation failures 
> > gracefully? If not and if it returns -ENOMEM somewhere, it would result in 
> > I/O errors and data corruption.
> 
> It is safe to use GFP_ATOMIC.  First of the allocation is really
> small (less than a page) so it will only fail when the system is
> almost completely out of memory.

GFP_ATOMIC is used by networking code. If the system is in a situation 
when packets arrive faster than the swapper is able to swap, it will 
happen. It does happen during netwoking surge and corrupting the 
filesystem in tris situation is not acceptable.

> Even when it does fail the crypto
> operation will still succeed, albeit it will process things at a
> smaller granularity so the performance will degrade.

A quick search through the crypto code shows that ahash_save_req and 
seqiv_aead_encrypt return -ENOMEM.

Will you fix them?

> The sleeping part of that flag is also not an issue because it
> only kicks in once per page.  As this is going to be less than
> or equal to a page it shouldn't matter.
>  
> > 3. introduce new flag CRYPTO_TFM_REQ_MAY_SLEEP_NOIO
> > ===================================================
> > Would you like to introduce it?
> 
> For now I don't think this is necessary given that this allocation
> and similar ones in lrw and other places in the crypto API are just
> performance enhancements and unlikely to fail except in very dire
> situations.
> 
> But if new problems arise I'm certainly not opposed to 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

Mikulas

Re: Deadlock when using crypto API for block devices

[Mikulas Patocka]

On Fri, 24 Aug 2018, Mikulas Patocka wrote:

> 
> 
> On Fri, 24 Aug 2018, Herbert Xu wrote:
> 
> > On Thu, Aug 23, 2018 at 04:39:23PM -0400, Mikulas Patocka wrote:
> > >
> > > 1. don't set CRYPTO_TFM_REQ_MAY_SLEEP in dm-crypt
> > > =================================================
> > > If we don't set it, dm-crypt will use GFP_ATOMIC and GFP_ATOMIC may fail. 
> > > The function init_crypt in xts.c handles the failure gracefully - but the 
> > > question is - does the whole crypto code handle allocation failures 
> > > gracefully? If not and if it returns -ENOMEM somewhere, it would result in 
> > > I/O errors and data corruption.
> > 
> > It is safe to use GFP_ATOMIC.  First of the allocation is really
> > small (less than a page) so it will only fail when the system is
> > almost completely out of memory.
> 
> GFP_ATOMIC is used by networking code. If the system is in a situation 
> when packets arrive faster than the swapper is able to swap, it will 
> happen. It does happen during netwoking surge and corrupting the 
> filesystem in tris situation is not acceptable.
> 
> > Even when it does fail the crypto
> > operation will still succeed, albeit it will process things at a
> > smaller granularity so the performance will degrade.
> 
> A quick search through the crypto code shows that ahash_save_req and 
> seqiv_aead_encrypt return -ENOMEM.
> 
> Will you fix them?

And also ablkcipher_next_slow, ablkcipher_copy_iv, skcipher_next_slow, 
skcipher_next_copy, skcipher_copy_iv, blkcipher_next_slow, 
blkcipher_copy_iv.

So, I think that dropping CRYPTO_TFM_REQ_MAY_SLEEP is not possible.

Mikulas

Re: Deadlock when using crypto API for block devices

[Herbert Xu]

On Fri, Aug 24, 2018 at 07:22:19AM -0400, Mikulas Patocka wrote:
>
> And also ablkcipher_next_slow, ablkcipher_copy_iv, skcipher_next_slow, 
> skcipher_next_copy, skcipher_copy_iv, blkcipher_next_slow, 
> blkcipher_copy_iv.
> 
> So, I think that dropping CRYPTO_TFM_REQ_MAY_SLEEP is not possible.

These should never trigger for dm-crypt AFAICS since it only
happens if you provide unaligned input/output.

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: Deadlock when using crypto API for block devices

[Herbert Xu]

On Fri, Aug 24, 2018 at 07:06:32AM -0400, Mikulas Patocka wrote:
>
> A quick search through the crypto code shows that ahash_save_req and 
> seqiv_aead_encrypt return -ENOMEM.
> 
> Will you fix them?

These only trigger for unaligned buffers.  It would be much better
if dm-crypt can ensure that the input/output is properly unaligned
and if otherwise do the allocation in dm-crypt.

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: Deadlock when using crypto API for block devices

[Mikulas Patocka]

On Fri, 24 Aug 2018, Herbert Xu wrote:

> On Fri, Aug 24, 2018 at 07:06:32AM -0400, Mikulas Patocka wrote:
> >
> > A quick search through the crypto code shows that ahash_save_req and 
> > seqiv_aead_encrypt return -ENOMEM.
> > 
> > Will you fix them?
> 
> These only trigger for unaligned buffers.  It would be much better
> if dm-crypt can ensure that the input/output is properly unaligned
> and if otherwise do the allocation in dm-crypt.

But we are relying here on an implementation detail and not on contract.

Mikulas

Re: Deadlock when using crypto API for block devices

[Mikulas Patocka]

On Fri, 24 Aug 2018, Mikulas Patocka wrote:

> 
> 
> On Fri, 24 Aug 2018, Herbert Xu wrote:
> 
> > On Fri, Aug 24, 2018 at 07:06:32AM -0400, Mikulas Patocka wrote:
> > >
> > > A quick search through the crypto code shows that ahash_save_req and 
> > > seqiv_aead_encrypt return -ENOMEM.
> > > 
> > > Will you fix them?
> > 
> > These only trigger for unaligned buffers.  It would be much better
> > if dm-crypt can ensure that the input/output is properly unaligned
> > and if otherwise do the allocation in dm-crypt.
> 
> But we are relying here on an implementation detail and not on contract.
> 
> Mikulas

BTW. gcmaes_crypt_by_sg also contains GFP_ATOMIC and -ENOMEM, behind a 
pretty complex condition. Do you mean that this condition is part of the 
contract that the crypto API provides?

Should "req->src->offset + req->src->length < PAGE_SIZE" use "<=" instead? 
Because if the data ends up at page boundary, it will use the atomic 
allocation that can fail.

Mikulas

Re: Deadlock when using crypto API for block devices

[Herbert Xu]

On Fri, Aug 24, 2018 at 09:00:00AM -0400, Mikulas Patocka wrote:
>
> BTW. gcmaes_crypt_by_sg also contains GFP_ATOMIC and -ENOMEM, behind a 
> pretty complex condition. Do you mean that this condition is part of the 
> contract that the crypto API provides?

This is an implementation defect.  I think for this case we should
fall back to software GCM if the accelerated version fails.

> Should "req->src->offset + req->src->length < PAGE_SIZE" use "<=" instead? 
> Because if the data ends up at page boundary, it will use the atomic 
> allocation that can fail.

This condition does look strange.  It's introduced by the commit
e845520707f85c539ce04bb73c6070e9441480be.   Dave, what exactly is
it meant to do?

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: Deadlock when using crypto API for block devices

[Herbert Xu]

On Fri, Aug 24, 2018 at 09:21:45PM +0800, Herbert Xu wrote:
> On Fri, Aug 24, 2018 at 09:00:00AM -0400, Mikulas Patocka wrote:
> >
> > BTW. gcmaes_crypt_by_sg also contains GFP_ATOMIC and -ENOMEM, behind a 
> > pretty complex condition. Do you mean that this condition is part of the 
> > contract that the crypto API provides?
> 
> This is an implementation defect.  I think for this case we should
> fall back to software GCM if the accelerated version fails.
> 
> > Should "req->src->offset + req->src->length < PAGE_SIZE" use "<=" instead? 
> > Because if the data ends up at page boundary, it will use the atomic 
> > allocation that can fail.
> 
> This condition does look strange.  It's introduced by the commit
> e845520707f85c539ce04bb73c6070e9441480be.   Dave, what exactly is
> it meant to do?

I forgot to Cc Dave Watson.

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: Deadlock when using crypto API for block devices

[Dave Watson]

On 08/24/18 09:22 PM, Herbert Xu wrote:
> > > BTW. gcmaes_crypt_by_sg also contains GFP_ATOMIC and -ENOMEM, behind a 
> > > pretty complex condition. Do you mean that this condition is part of the 
> > > contract that the crypto API provides?
> > 
> > This is an implementation defect.  I think for this case we should
> > fall back to software GCM if the accelerated version fails.
> > 
> > > Should "req->src->offset + req->src->length < PAGE_SIZE" use "<=" instead? 
> > > Because if the data ends up at page boundary, it will use the atomic 
> > > allocation that can fail.
> > 
> > This condition does look strange.  It's introduced by the commit
> > e845520707f85c539ce04bb73c6070e9441480be.   Dave, what exactly is
> > it meant to do?

The aesni routines still require linear AAD data, the condition checks
that the AAD data is linear, and if not, kmallocs and copies it to a
linear buffer.

Yes, the condition looks like it could be <= PAGE_SIZE, similar to the
one in gcmaes_encrypt.

Yes, we should fall back to software gcm if kmalloc fails, although
AAD data is usually small, and it is probably worth having a small
stack buffer before attempting to kmalloc.