[PATCH 0/3] Fix a deadlock in blk-crypto-fallback

[PATCH 0/3] Fix a deadlock in blk-crypto-fallback

[Eric Biggers]

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
-- 
2.55.0

[PATCH 1/3] mm: make mempool_alloc_from_pool() return bool

[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 if
mempool_alloc_from_pool() succeeds, rather than jumping back to the
retry_alloc label to allocate nothing and then returning.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 mm/mempool.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/mempool.c b/mm/mempool.c
index 473a029fa31f..d454bc9f39e9 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -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;
 }
-- 
2.55.0

[PATCH 2/3] mm: support fallible mempool_alloc_bulk()

[Eric Biggers]

To 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().

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 block/blk-crypto-fallback.c |  6 ++----
 include/linux/mempool.h     |  4 ++--
 mm/mempool.c                | 42 +++++++++++++++++++++++++------------
 3 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index 2a5c52ab74b4..bda913c39381 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -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;
 }
diff --git a/include/linux/mempool.h b/include/linux/mempool.h
index a0fa6d43e0dc..f7898cd1512b 100644
--- a/include/linux/mempool.h
+++ b/include/linux/mempool.h
@@ -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__))
 
diff --git a/mm/mempool.c b/mm/mempool.c
index d454bc9f39e9..d741e5f62554 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -473,25 +473,28 @@ static inline gfp_t mempool_adjust_gfp(gfp_t *gfp_mask)
 /**
  * mempool_alloc_bulk - allocate multiple elements from a memory pool
  * @pool:	pointer to the memory pool
- * @elems:	partially or fully populated elements array
- * @count:	number of entries in @elem that need to be allocated
+ * @elems:	pointer to array into which the element pointers will be stored
+ * @count:	number of elements to allocate
+ * @gfp_mask:	GFP_* flags.  %__GFP_ZERO is not supported.  If this mask
+ *		includes %__GFP_DIRECT_RECLAIM, then the allocation is retried
+ *		indefinitely until it succeeds and the return value is always
+ *		%true.  If the mask doesn't include %__GFP_DIRECT_RECLAIM, then
+ *		failure is allowed and %false can be returned.
  *
  * Allocate @count elements into @elems.  This is done by first calling into the
  * alloc_fn supplied at pool initialization time, and dipping into the reserved
- * pool when alloc_fn fails to allocate an element.
- *
- * On return all @count elements in @elems will be populated.
+ * pool to atomically allocate the remaining elements if alloc_fn fails.
  *
- * Return: Always 0.  If it wasn't for %$#^$ alloc tags, it would return void.
+ * Return: %true if the allocation succeeded, or %false if it failed.
  */
-int mempool_alloc_bulk_noprof(struct mempool *pool, void **elems,
-		unsigned int count)
+bool mempool_alloc_bulk_noprof(struct mempool *pool, void **elems,
+		unsigned int count, gfp_t gfp_mask)
 {
-	gfp_t gfp_mask = GFP_KERNEL;
 	gfp_t gfp_temp = mempool_adjust_gfp(&gfp_mask);
 	unsigned int allocated = 0;
 
 	VM_WARN_ON_ONCE(count > pool->min_nr);
+	VM_WARN_ON_ONCE(gfp_mask & __GFP_ZERO);
 	might_alloc(gfp_mask);
 
 	/*
@@ -516,13 +519,26 @@ int mempool_alloc_bulk_noprof(struct mempool *pool, void **elems,
 		allocated++;
 	}
 
-	return 0;
+	return true;
 
 use_pool:
+	/* Try to atomically allocate the remaining elements from the pool. */
 	if (mempool_alloc_from_pool(pool, elems, count, allocated, gfp_temp))
-		return 0;
-	gfp_temp = gfp_mask;
-	goto repeat_alloc;
+		return true;
+	/* Retry if this was just the opportunistic first pass. */
+	if (gfp_temp != gfp_mask) {
+		gfp_temp = gfp_mask;
+		goto repeat_alloc;
+	}
+	/* Retry indefinitely if __GFP_DIRECT_RECLAIM is set. */
+	if (gfp_mask & __GFP_DIRECT_RECLAIM)
+		goto repeat_alloc;
+	/* On failure, roll back any successful allocations from ->alloc(). */
+	while (allocated--) {
+		pool->free(elems[allocated], pool->pool_data);
+		elems[allocated] = NULL;
+	}
+	return false;
 }
 EXPORT_SYMBOL_GPL(mempool_alloc_bulk_noprof);
 
-- 
2.55.0

[PATCH 3/3] blk-crypto-fallback: Fix deadlock when encrypting large bio in dm layer

[Eric Biggers]

Encrypting 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.

Fixes: 488f6682c832 ("block: blk-crypto-fallback for Inline Encryption")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 block/blk-crypto-fallback.c | 70 ++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 5 deletions(-)

diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index bda913c39381..973399011d5a 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -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;
-- 
2.55.0