[PATCH] dm-crypt: unlock allocation mutex before retry

[PATCH] dm-crypt: unlock allocation mutex before retry

[Runyu Xiao]

If the initial GFP_NOWAIT page allocation fails, crypt_alloc_buffer()
sets __GFP_DIRECT_RECLAIM and retries while holding bio_alloc_lock. If a
later page allocation fails, the retry edge bypasses the common unlock and
reacquires the same mutex. This can deadlock the dm-crypt I/O path and
leave I/O for the affected mapping stalled.

Release the mutex before retrying, but only when this attempt took the
direct-reclaim path; the first attempt did not acquire it.

This issue was identified by a static-analysis checker and manually
confirmed by following the retry control flow in v6.1.66 and current
mainline. A source-level control-flow check verified the vulnerable
ordering and the unlock-before-retry ordering after this change. A small
POSIX-thread model checked only the mutex re-acquisition condition; it does
not exercise dm-crypt.

Fixes: 7145c241a1bf ("dm crypt: avoid deadlock in mempools")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/md/dm-crypt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 608b617fb817..aabb9a5f85a7 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1674,6 +1674,8 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
 		if (!pages) {
 			crypt_free_buffer_pages(cc, clone);
 			bio_put(clone);
+			if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
+				mutex_unlock(&cc->bio_alloc_lock);
 			gfp_mask |= __GFP_DIRECT_RECLAIM;
 			order = 0;
 			goto retry;
-- 
2.34.1

Re: [PATCH] dm-crypt: unlock allocation mutex before retry

[Bart Van Assche]

On 8/8/26 10:18 PM, Runyu Xiao wrote:
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 608b617fb817..aabb9a5f85a7 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1674,6 +1674,8 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
>   		if (!pages) {
>   			crypt_free_buffer_pages(cc, clone);
>   			bio_put(clone);
> +			if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> +				mutex_unlock(&cc->bio_alloc_lock);
>   			gfp_mask |= __GFP_DIRECT_RECLAIM;
>   			order = 0;
>   			goto retry;

Please refactor crypt_alloc_buffer() such that context analysis can be
enabled instead of making this function more complex. One way to do this
is by moving the crypt_alloc_buffer() code that occurs between the
mutex_lock() and mutex_unlock() calls into a helper function.

See also https://docs.kernel.org/dev-tools/context-analysis.html.

Thanks,

Bart.

Re: [PATCH] dm-crypt: unlock allocation mutex before retry

[Mikulas Patocka]

On Sun, 9 Aug 2026, Runyu Xiao wrote:

> If the initial GFP_NOWAIT page allocation fails, crypt_alloc_buffer()
> sets __GFP_DIRECT_RECLAIM and retries while holding bio_alloc_lock. If a
> later page allocation fails, the retry edge bypasses the common unlock and
> reacquires the same mutex. This can deadlock the dm-crypt I/O path and
> leave I/O for the affected mapping stalled.

Hi

When __GFP_DIRECT_RECLAIM is set, mempool_alloc can't return NULL. So, the 
bug can't happen.

Mikulas

> Release the mutex before retrying, but only when this attempt took the
> direct-reclaim path; the first attempt did not acquire it.
> 
> This issue was identified by a static-analysis checker and manually
> confirmed by following the retry control flow in v6.1.66 and current
> mainline. A source-level control-flow check verified the vulnerable
> ordering and the unlock-before-retry ordering after this change. A small
> POSIX-thread model checked only the mutex re-acquisition condition; it does
> not exercise dm-crypt.
> 
> Fixes: 7145c241a1bf ("dm crypt: avoid deadlock in mempools")
> Cc: stable@vger.kernel.org
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>  drivers/md/dm-crypt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 608b617fb817..aabb9a5f85a7 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1674,6 +1674,8 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
>  		if (!pages) {
>  			crypt_free_buffer_pages(cc, clone);
>  			bio_put(clone);
> +			if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> +				mutex_unlock(&cc->bio_alloc_lock);
>  			gfp_mask |= __GFP_DIRECT_RECLAIM;
>  			order = 0;
>  			goto retry;
> -- 
> 2.34.1
> 

[PATCH v2] dm-crypt: refactor buffer allocation retry handling

[Runyu Xiao]

crypt_alloc_buffer() conditionally acquires bio_alloc_lock around the
allocation path and also contains the retry logic for the reclaim
fallback.

Move one allocation attempt into crypt_alloc_buffer_try() so that
bio_alloc_lock is always acquired and released in crypt_alloc_buffer(),
and the retry decision is made only after the mutex has been dropped.

This keeps the retry path outside the locked region and makes the
locking context easier to analyze.

Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

---
Changes in v2:
- Move one allocation attempt into crypt_alloc_buffer_try(), as suggested
  by Bart Van Assche.
- Keep bio_alloc_lock acquisition and release in crypt_alloc_buffer(), and
  make the retry decision only after releasing the lock.
- Preserve the distinction between a page-pool retry and a final integrity
  allocation failure.

v1: https://lore.kernel.org/r/20260809051839.3504080-1-runyu.xiao@seu.edu.cn

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 608b617fb817..7c68fad12960 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1627,18 +1627,16 @@ static void crypt_free_buffer_pages(struct crypt_config *cc, struct bio *clone);
  * In order to reduce allocation overhead, we try to allocate compound pages in
  * the first pass. If they are not available, we fall back to the mempool.
  */
-static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
+static struct bio *crypt_alloc_buffer_try(struct dm_crypt_io *io,
+					  unsigned int size, gfp_t gfp_mask,
+					  unsigned int order, bool *retry)
 {
 	struct crypt_config *cc = io->cc;
 	struct bio *clone;
 	unsigned int nr_iovecs = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM;
 	unsigned int remaining_size;
-	unsigned int order = MAX_PAGE_ORDER;
 
-retry:
-	if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
-		mutex_lock(&cc->bio_alloc_lock);
+	*retry = false;
 
 	clone = bio_alloc_bioset(cc->dev->bdev, nr_iovecs, io->base_bio->bi_opf,
 				 GFP_NOIO, &cc->bs);
@@ -1674,9 +1672,8 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
 		if (!pages) {
 			crypt_free_buffer_pages(cc, clone);
 			bio_put(clone);
-			gfp_mask |= __GFP_DIRECT_RECLAIM;
-			order = 0;
-			goto retry;
+			*retry = true;
+			return NULL;
 		}
 
 have_pages:
@@ -1692,12 +1689,34 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
 		clone = NULL;
 	}
 
-	if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
-		mutex_unlock(&cc->bio_alloc_lock);
-
 	return clone;
 }
 
+static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
+{
+	struct crypt_config *cc = io->cc;
+	struct bio *clone;
+	gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM;
+	unsigned int order = MAX_PAGE_ORDER;
+	bool retry;
+
+	for (;;) {
+		if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
+			mutex_lock(&cc->bio_alloc_lock);
+
+		clone = crypt_alloc_buffer_try(io, size, gfp_mask, order, &retry);
+
+		if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
+			mutex_unlock(&cc->bio_alloc_lock);
+
+		if (!retry)
+			return clone;
+
+		gfp_mask |= __GFP_DIRECT_RECLAIM;
+		order = 0;
+	}
+}
+
 static void crypt_free_buffer_pages(struct crypt_config *cc, struct bio *clone)
 {
 	struct folio_iter fi;
-- 
2.34.1

Re: [PATCH v2] dm-crypt: refactor buffer allocation retry handling

[Mikulas Patocka]

On Tue, 11 Aug 2026, Runyu Xiao wrote:

> crypt_alloc_buffer() conditionally acquires bio_alloc_lock around the
> allocation path and also contains the retry logic for the reclaim
> fallback.
> 
> Move one allocation attempt into crypt_alloc_buffer_try() so that
> bio_alloc_lock is always acquired and released in crypt_alloc_buffer(),
> and the retry decision is made only after the mutex has been dropped.
> 
> This keeps the retry path outside the locked region and makes the
> locking context easier to analyze.
> 
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

Hi

I wouldn't do this. The patch just moves code around and increases code 
size with no benefit.

Mikulas

> ---
> Changes in v2:
> - Move one allocation attempt into crypt_alloc_buffer_try(), as suggested
>   by Bart Van Assche.
> - Keep bio_alloc_lock acquisition and release in crypt_alloc_buffer(), and
>   make the retry decision only after releasing the lock.
> - Preserve the distinction between a page-pool retry and a final integrity
>   allocation failure.
> 
> v1: https://lore.kernel.org/r/20260809051839.3504080-1-runyu.xiao@seu.edu.cn
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 608b617fb817..7c68fad12960 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1627,18 +1627,16 @@ static void crypt_free_buffer_pages(struct crypt_config *cc, struct bio *clone);
>   * In order to reduce allocation overhead, we try to allocate compound pages in
>   * the first pass. If they are not available, we fall back to the mempool.
>   */
> -static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
> +static struct bio *crypt_alloc_buffer_try(struct dm_crypt_io *io,
> +					  unsigned int size, gfp_t gfp_mask,
> +					  unsigned int order, bool *retry)
>  {
>  	struct crypt_config *cc = io->cc;
>  	struct bio *clone;
>  	unsigned int nr_iovecs = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> -	gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM;
>  	unsigned int remaining_size;
> -	unsigned int order = MAX_PAGE_ORDER;
>  
> -retry:
> -	if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> -		mutex_lock(&cc->bio_alloc_lock);
> +	*retry = false;
>  
>  	clone = bio_alloc_bioset(cc->dev->bdev, nr_iovecs, io->base_bio->bi_opf,
>  				 GFP_NOIO, &cc->bs);
> @@ -1674,9 +1672,8 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
>  		if (!pages) {
>  			crypt_free_buffer_pages(cc, clone);
>  			bio_put(clone);
> -			gfp_mask |= __GFP_DIRECT_RECLAIM;
> -			order = 0;
> -			goto retry;
> +			*retry = true;
> +			return NULL;
>  		}
>  
>  have_pages:
> @@ -1692,12 +1689,34 @@ static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
>  		clone = NULL;
>  	}
>  
> -	if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> -		mutex_unlock(&cc->bio_alloc_lock);
> -
>  	return clone;
>  }
>  
> +static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned int size)
> +{
> +	struct crypt_config *cc = io->cc;
> +	struct bio *clone;
> +	gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM;
> +	unsigned int order = MAX_PAGE_ORDER;
> +	bool retry;
> +
> +	for (;;) {
> +		if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> +			mutex_lock(&cc->bio_alloc_lock);
> +
> +		clone = crypt_alloc_buffer_try(io, size, gfp_mask, order, &retry);
> +
> +		if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> +			mutex_unlock(&cc->bio_alloc_lock);
> +
> +		if (!retry)
> +			return clone;
> +
> +		gfp_mask |= __GFP_DIRECT_RECLAIM;
> +		order = 0;
> +	}
> +}
> +
>  static void crypt_free_buffer_pages(struct crypt_config *cc, struct bio *clone)
>  {
>  	struct folio_iter fi;
> -- 
> 2.34.1
> 

Re:Re: [PATCH v2] dm-crypt: refactor buffer allocation retry handling

[Runyu Xiao]

Hi Mikulas,

Thanks for reviewing this.

I initially interpreted the goto retry path as being reachable after
bio_alloc_lock was acquired. After rechecking mempool_alloc() in v6.1.66,
I found that it returns NULL only without __GFP_DIRECT_RECLAIM. With that
flag set, it waits and retries when the pool is empty.

The first allocation failure happens before bio_alloc_lock is acquired,
while the later allocation is performed with the mutex held and does not
take the NULL retry path in normal process context. Therefore, I cannot
justify the suspected double acquisition, and the refactoring provides no
functional benefit.

I will drop this patch.

Regards,
Runyu