Fix a deadlock in blk-crypto-fallback
Subsystems (4)
cover Fix a deadlock in blk-crypto-fallback
This series fixes a deadlock in blk-crypto-fallback that could occur when a large WRITE bio was submitted to dm-inlinecrypt.
As a prerequisite, it makes mempool_alloc_bulk() support fallible allocations in the same way that mempool_alloc() does.
Please consider this series for the block tree for 7.3.
Eric Biggers (3):
mm: make mempool_alloc_from_pool() return bool
mm: support fallible mempool_alloc_bulk()
blk-crypto-fallback: Fix deadlock when encrypting large bio in dm
layer
block/blk-crypto-fallback.c | 72 +++++++++++++++++++++++++++++++++---- include/linux/mempool.h | 4 +-- mm/mempool.c | 50 +++++++++++++++++--------- 3 files changed, 100 insertions(+), 26 deletions(-)
base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
Signature · 1 line
2.55.0
PATCH 1/3 mm: make mempool_alloc_from_pool() return bool
mempool_alloc_from_pool() is intentionally all-or-nothing, so make it return a bool rather than the number of elements allocated.
Then make mempool_alloc_bulk() return right away if mempool_alloc_from_pool() succeeds, rather than jumping back to the retry_alloc label to allocate nothing and then returning.
--- mm/mempool.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
mm/mempool.c +5 -5
@@ -409,7 +409,7 @@ int mempool_resize(struct mempool *pool, int new_min_nr) }EXPORT_SYMBOL (mempool_resize); -static unsigned int mempool_alloc_from_pool (struct mempool *pool,void **elems, +static bool mempool_alloc_from_pool (struct mempool *pool,void **elems,unsigned int count,unsigned int allocated,gfp_t gfp_mask) {
@@ -432,7 +432,7 @@ static unsigned int mempool_alloc_from_pool(struct mempool *pool, void **elems,*/ for (i =0 ; i < count; i++)kmemleak_update_trace (elems[i]); -return allocated; +return true; fail:if (gfp_mask & __GFP_DIRECT_RECLAIM) {
@@ -454,7 +454,7 @@ static unsigned int mempool_alloc_from_pool(struct mempool *pool, void **elems,spin_unlock_irqrestore (&pool->lock, flags); } -return allocated; +return false; }/*
@@ -519,8 +519,8 @@ int mempool_alloc_bulk_noprof(struct mempool *pool, void **elems,return 0 ; use_pool: - allocated =mempool_alloc_from_pool (pool, elems, count, allocated, - gfp_temp); +if (mempool_alloc_from_pool (pool, elems, count, allocated, gfp_temp)) +return 0 ; gfp_temp = gfp_mask; goto repeat_alloc; }
Signature · 1 line
2.55.0
Re: [PATCH 1/3] mm: make mempool_alloc_from_pool() return bool
Looks good and matches what I recently did on the slab side:
Re: [PATCH 1/3] mm: make mempool_alloc_from_pool() return bool
In reply to Eric BiggersQuoted 9 lines · Eric Biggers
mempool_alloc_from_pool()is intentionally all-or-nothing, so make it return a bool rather than the number of elements allocated.Then make
mempool_alloc_bulk()return right away ifmempool_alloc_from_pool()succeeds, rather than jumping back to theretry_alloclabel to allocate nothing and then returning.Signed-off-by Eric Biggers <[email protected]>---
Looks good to me,
Signature · 2 lines
Cheers, Harry / Hyeonggon
PATCH 2/3 mm: support fallible mempool_alloc_bulk()
In reply to Eric BiggersTo fix a deadlock, blk-crypto-fallback needs to be able to make fallible mempool_alloc_bulk() allocations. But mempool_alloc_bulk() hardcodes GFP_KERNEL and infinite retries, which differs from mempool_alloc() which supports fallible allocations via its gfp_mask argument.
Therefore, add a gfp_mask argument to mempool_alloc_bulk(). As with mempool_alloc(), the presence of __GFP_DIRECT_RECLAIM in the mask selects between the fallible and infallible modes.
For now it just provides all-or-nothing semantics and returns a bool, similar to kmem_cache_alloc_bulk().
--- block/blk-crypto-fallback.c | 6 ++---- include/linux/mempool.h | 4 ++-- mm/mempool.c | 42 +++++++++++++++++++++++++------------ 3 files changed, 33 insertions(+), 19 deletions(-)
block/blk-crypto-fallback.c +2 -4
@@ -172,7 +172,6 @@ static void blk_crypto_fallback_encrypt_endio(struct bio *enc_bio)static struct bio *blk_crypto_alloc_enc_bio (struct bio *bio_src,unsigned int nr_segs,struct page ***pages_ret) { -unsigned int memflags =memalloc_noio_save ();unsigned int nr_allocated;struct page **pages;struct bio *bio;
@@ -206,12 +205,11 @@ static struct bio *blk_crypto_alloc_enc_bio(struct bio *bio_src,* any non-zero slot already contains a valid allocation. */ memset (pages,0 ,sizeof (struct page *) * nr_segs); - nr_allocated =alloc_pages_bulk (GFP_KERNEL , nr_segs, pages); + nr_allocated =alloc_pages_bulk (GFP_NOIO , nr_segs, pages);if (nr_allocated < nr_segs)mempool_alloc_bulk (blk_crypto_bounce_page_pool, (void **)pages + nr_allocated, - nr_segs - nr_allocated); -memalloc_noio_restore (memflags); + nr_segs - nr_allocated,GFP_NOIO ); *pages_ret = pages;return bio; }
include/linux/mempool.h +2 -2
@@ -65,8 +65,8 @@ void mempool_destroy(struct mempool *pool);void *mempool_alloc_noprof (struct mempool *pool,gfp_t gfp_mask) __malloc;#define mempool_alloc (...) \ alloc_hooks(mempool_alloc_noprof(__VA_ARGS__)) -int mempool_alloc_bulk_noprof (struct mempool *pool,void **elem, -unsigned int count); +bool mempool_alloc_bulk_noprof (struct mempool *pool,void **elem, +unsigned int count,gfp_t gfp_mask);#define mempool_alloc_bulk (...) \ alloc_hooks(mempool_alloc_bulk_noprof(__VA_ARGS__))
mm/mempool.c +29 -13
67 lines not on this page · raw message
Signature · 1 line
2.55.0
Re: [PATCH 2/3] mm: support fallible mempool_alloc_bulk()
Quoted 2 lines · Eric Biggers
To fix a deadlock, blk-crypto-fallback needs to be able to make fallible
mempool_alloc_bulk()allocations.
It doesn't. fallible mempool allocations are a concept that doesn't make much sense. Please just go straight to the backing page allocator instead for callers that do not need the mempool guarantees.
Re: [PATCH 2/3] mm: support fallible mempool_alloc_bulk()
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Eric Biggers
To fix a deadlock, blk-crypto-fallback needs to be able to make fallible
mempool_alloc_bulk()allocations.It doesn't. fallible mempool allocations are a concept that doesn't make much sense. Please just go straight to the backing page allocator instead for callers that do not need the mempool guarantees.
It does make sense. When alloc_pages_bulk() doesn't completely succeed, there still might be pages available in the mempool. blk_crypto_alloc_enc_bio() should try to take them before falling back to scheduling the rescuer kworker. The rescuer encounters scheduling overhead and is single-threaded, so it's slow and should be used only when absolutely necessary.
- Eric
Re: [PATCH 2/3] mm: support fallible mempool_alloc_bulk()
Quoted 11 lines · Eric Biggers
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Eric Biggers
To fix a deadlock, blk-crypto-fallback needs to be able to make fallible
mempool_alloc_bulk()allocations.It doesn't. fallible mempool allocations are a concept that doesn't make much sense. Please just go straight to the backing page allocator instead for callers that do not need the mempool guarantees.
It does make sense. When
alloc_pages_bulk()doesn't completely succeed, there still might be pages available in the mempool.
But they should not go to a caller that does not need the mempool.
Quoted 4 lines
blk_crypto_alloc_enc_bio()should try to take them before falling back to scheduling the rescuer kworker. The rescuer encounters scheduling overhead and is single-threaded, so it's slow and should be used only when absolutely necessary.
No, it should just try a regular non-bulk alloc_pages (and eventually alloc_pages_bulk should do that fallback for the callers, but that's a separate discussion).
Re: [PATCH 2/3] mm: support fallible mempool_alloc_bulk()
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Eric Biggers
Quoted 4 lines · Christoph Hellwig
Quoted 2 lines · Eric Biggers
To fix a deadlock, blk-crypto-fallback needs to be able to make fallible
mempool_alloc_bulk()allocations.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
It doesn't. fallible mempool allocations are a concept that doesn't make much sense. Please just go straight to the backing page allocator instead for callers that do not need the mempool guarantees.
It does make sense. When
alloc_pages_bulk()doesn't completely succeed, there still might be pages available in the mempool.But they should not go to a caller that does not need the mempool.
The caller does need the mempool.
Quoted 8 lines
Quoted 4 lines
blk_crypto_alloc_enc_bio()should try to take them before falling back to scheduling the rescuer kworker. The rescuer encounters scheduling overhead and is single-threaded, so it's slow and should be used only when absolutely necessary.No, it should just try a regular non-bulk
alloc_pages(and eventuallyalloc_pages_bulkshould do that fallback for the callers, but that's a separate discussion).
It already does that. blk_crypto_alloc_enc_bio() first calls alloc_pages_bulk(). If it doesn't completely succeed, it calls mempool_alloc_bulk() to get the rest. That itself tries regular allocations again before actually using the mempool. It needs a guaranteed allocation, so it needs the mempool.
Now, as I explained in this patchset, whether this code can wait forever for the mempool actually depends on whether it's a recursive bio submission or not. If it is, then it cannot wait, but ultimately it does still need to use the mempool to get a guaranteed allocation.
Falling back to the rescuer kthread (which can wait on the mempool) solves that. But before taking that slow fallback, it's much more efficient to check the mempool directly first, since pages may be available there (and in fact it's fairly likely that they will be, since regular allocations are always used first). The whole point of the mempool is that it can be used when the regular allocation fails.
This is also the only caller of mempool_alloc_bulk(). So I'm kind of confused why it would not be allowed to implement the behavior that is desired here, especially when the non-bulk API offers it already.
- Eric
Re: [PATCH 2/3] mm: support fallible mempool_alloc_bulk()
Quoted 11 lines · Eric Biggers
Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · Eric Biggers
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Eric Biggers
To fix a deadlock, blk-crypto-fallback needs to be able to make fallible
mempool_alloc_bulk()allocations.It doesn't. fallible mempool allocations are a concept that doesn't make much sense. Please just go straight to the backing page allocator instead for callers that do not need the mempool guarantees.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
It does make sense. When
alloc_pages_bulk()doesn't completely succeed, there still might be pages available in the mempool.But they should not go to a caller that does not need the mempool.
The caller does need the mempool.
If it does not specify a blocking GFP_* mask, it by definition does not.
Quoted 11 lines
Now, as I explained in this patchset, whether this code can wait forever for the mempool actually depends on whether it's a recursive bio submission or not. If it is, then it cannot wait, but ultimately it does still need to use the mempool to get a guaranteed allocation.
Falling back to the rescuer kthread (which can wait on the mempool) solves that. But before taking that slow fallback, it's much more efficient to check the mempool directly first, since pages may be available there (and in fact it's fairly likely that they will be, since regular allocations are always used first). The whole point of the mempool is that it can be used when the regular allocation fails.
What is actually efficient is do skip the mempool entirely from the original submission context and do a non-mempool GFP_NOIO allocation, and only in the extremely unlikely case that this fails fall back to the rescruer thread.
Quoted 3 lines
This is also the only caller of
mempool_alloc_bulk(). So I'm kind of confused why it would not be allowed to implement the behavior that is desired here, especially when the non-bulk API offers it already.
Because passing GFP_ flags with all their warts is a bad idea where we can avoid, as is using up mempool resources where not needed, and as is having a possible failure path from a function that can only happen for non-standard flags. (having a return value at all is also stupid, but that is forced on us by the braindead alloc_hooks).
Re: [PATCH 2/3] mm: support fallible mempool_alloc_bulk()
Quoted 4 lines · Christoph Hellwig
What is actually efficient is do skip the mempool entirely from the original submission context and do a non-mempool
GFP_NOIOallocation, and only in the extremely unlikely case that this fails fall back to the rescruer thread.
It definitely is *not* more efficient to fall back to a kworker for a small task (removing elements from a mempool that has them available) that could just be done synchronously. Scheduling overhead is a huge problem for kernel features doing I/O pre or post-processing. The idea that scheduling a kworker is lightweight compared to the actual I/O is quite outdated.
Sure, this case triggers only when the regular allocations fail anyway. So it shouldn't be a big deal, and I'll send a patch that implements it the less efficient way that you prefer. But it's a little unfortunate that you're not going to allow it to be implemented properly.
- Eric
Re: [PATCH 2/3] mm: support fallible mempool_alloc_bulk()
Quoted 9 lines · Eric Biggers
Quoted 4 lines · Christoph Hellwig
What is actually efficient is do skip the mempool entirely from the original submission context and do a non-mempool
GFP_NOIOallocation, and only in the extremely unlikely case that this fails fall back to the rescruer thread.It definitely is *not* more efficient to fall back to a kworker for a small task (removing elements from a mempool that has them available) that could just be done synchronously.
That is not the point. The point is that mempool_alloc_bulk frequently fails to allocate all pages, and just filling that with a plain alloc_pages is way faster than calling into the mempool code in any form. The case where we actually need the mempool entries is extremely rare.
PATCH 3/3 blk-crypto-fallback: Fix deadlock when encrypting large bio in dm layer
In reply to Eric BiggersEncrypting a large bio with more than BIO_MAX_VECS pages can still deadlock, even after it was attempted to be fixed by
commit b37fbce460ad ("blk-crypto: optimize bio splitting in blk_crypto_fallback_encrypt_bio") and commit 3d939695e682 ("blk-crypto:
use mempool_alloc_bulk for encrypted bio page allocation").
This is because __blk_crypto_fallback_encrypt_bio() assumes that the bounce bios that it submits will eventually complete, unblocking it from allocating additional bounce bios and pages. However, dm-inlinecrypt.c calls __blk_crypto_submit_bio() from within submit_bio() itself. In this case, the recursive submit_bio() simply adds the bounce bio to current->bio_list without actually submitting it yet. That breaks the guarantee that forward progress is being made.
To fix this, allocate the bio and bounce pages with GFP_NOWAIT if current->bio_list is set. If it fails, punt the encryption of the remaining part of the bio to a kworker.
--- block/blk-crypto-fallback.c | 70 ++++++++++++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 5 deletions(-)
block/blk-crypto-fallback.c +65 -5
@@ -83,6 +83,10 @@ static struct workqueue_struct *blk_crypto_wq;static mempool_t *blk_crypto_bounce_page_pool;static struct bio_set enc_bio_set; +static DEFINE_SPINLOCK (enc_rescue_list_lock); +static struct bio_list enc_rescue_list; +static struct work_struct enc_rescue_work; +/* * This is the key we set when evicting a keyslot. This *should* be the all 0's * key, but AES-XTS rejects that key, so we use some random bytes instead.
@@ -175,9 +179,23 @@ static struct bio *blk_crypto_alloc_enc_bio(struct bio *bio_src,unsigned int nr_allocated;struct page **pages;struct bio *bio; +gfp_t gfp_mask; + +/* +* During recursive bio submission (current->bio_list != NULL) any +* submitted bounce bios just get added to current->bio_list; they +* cannot complete and release resources yet. Therefore, to avoid +* deadlocks, don't wait indefinitely for additional resources. +*/ +if (current->bio_list) + gfp_mask =GFP_NOWAIT ; +else + gfp_mask =GFP_NOIO ; bio =bio_alloc_bioset (bio_src->bi_bdev, nr_segs, bio_src->bi_opf, -GFP_NOIO , &enc_bio_set); + gfp_mask, &enc_bio_set); +if (unlikely (!bio)) +return NULL ;if (bio_flagged (bio_src,BIO_REMAPPED ))bio_set_flag (bio,BIO_REMAPPED ); bio->bi_private = bio_src;
@@ -205,11 +223,15 @@ static struct bio *blk_crypto_alloc_enc_bio(struct bio *bio_src,* any non-zero slot already contains a valid allocation. */ memset (pages,0 ,sizeof (struct page *) * nr_segs); - nr_allocated =alloc_pages_bulk (GFP_NOIO , nr_segs, pages); -if (nr_allocated < nr_segs) -mempool_alloc_bulk (blk_crypto_bounce_page_pool, + nr_allocated =alloc_pages_bulk (gfp_mask, nr_segs, pages); +if (unlikely (nr_allocated < nr_segs) && + !mempool_alloc_bulk (blk_crypto_bounce_page_pool, (void **)pages + nr_allocated, - nr_segs - nr_allocated,GFP_NOIO ); + nr_segs - nr_allocated, gfp_mask)) { +free_pages_bulk (pages, nr_allocated); +bio_put (bio); +return NULL ; + } *pages_ret = pages;return bio; }
@@ -237,6 +259,25 @@ static void blk_crypto_dun_to_iv(const u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE], iv->dun[i] =cpu_to_le64 (dun[i]); } +static void blk_crypto_fallback_encrypt_bio (struct bio *src_bio); + +/* Encrypt a list of bios whose encryption was punted to a kworker. */ +static void blk_crypto_fallback_encrypt_work_fn (struct work_struct *work) +{ +struct bio_list list; +struct bio *src_bio; + +WARN_ON_ONCE (current->bio_list); + +spin_lock (&enc_rescue_list_lock); + list = enc_rescue_list; +bio_list_init (&enc_rescue_list); +spin_unlock (&enc_rescue_list_lock); + +while ((src_bio =bio_list_pop (&list))) +blk_crypto_fallback_encrypt_bio (src_bio); +} +static void __blk_crypto_fallback_encrypt_bio (struct bio *src_bio,struct crypto_sync_skcipher *tfm) {
@@ -271,6 +312,23 @@ static void __blk_crypto_fallback_encrypt_bio(struct bio *src_bio, new_bio: nr_enc_pages =min (bio_segments (src_bio),BIO_MAX_VECS ); enc_bio =blk_crypto_alloc_enc_bio (src_bio, nr_enc_pages, &enc_pages); +if (unlikely (!enc_bio)) { +/* +* Failed to allocate a bounce bio during recursive bio +* submission. We might be blocked on bios in current->bio_list +* holding mempool elements. To enable forward progress, punt +* the remaining encryption work for src_bio to a kworker. +* +* The DUN may have been advanced, so make sure to update it. +*/ +WARN_ON_ONCE (!current->bio_list); +memcpy (bc->bc_dun, curr_dun,sizeof (curr_dun)); +spin_lock (&enc_rescue_list_lock); +bio_list_add (&enc_rescue_list, src_bio); +spin_unlock (&enc_rescue_list_lock); +queue_work (blk_crypto_wq, &enc_rescue_work); +return ; + } enc_idx =0 ;for (;;) { struct bio_vec src_bv =
@@ -589,6 +647,8 @@ static int blk_crypto_fallback_init(void)if (!bio_fallback_crypt_ctx_pool) goto fail_free_crypt_ctx_cache; +INIT_WORK (&enc_rescue_work, blk_crypto_fallback_encrypt_work_fn); + blk_crypto_fallback_inited = true;return 0 ;
Signature · 1 line
2.55.0