bio_kmalloc related fixes for 5.12

bio_kmalloc related fixes for 5.12

[Christoph Hellwig]

Hi Jens,

a few fixes for 5.12 below.

[PATCH 1/4] block-crypto-fallback: use a bio_set for splitting bios

[Christoph Hellwig]

bio_split with a NULL bs argumen used to fall back to kmalloc the
bio, which does not guarantee forward progress and could to deadlocks.
Now that the overloading of the NULL bs argument to bio_alloc_bioset
has been removed it crashes instead.  Fix all that by using a special
crafted bioset.

Fixes: 3175199ab0ac ("block: split bio_kmalloc from bio_alloc_bioset")
Reported-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: John Stultz <john.stultz@linaro.org>
---
 block/blk-crypto-fallback.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
index e8327c50d7c9f4..c176b7af56a7a5 100644
--- a/block/blk-crypto-fallback.c
+++ b/block/blk-crypto-fallback.c
@@ -80,6 +80,7 @@ static struct blk_crypto_keyslot {
 static struct blk_keyslot_manager blk_crypto_ksm;
 static struct workqueue_struct *blk_crypto_wq;
 static mempool_t *blk_crypto_bounce_page_pool;
+static struct bio_set crypto_bio_split;
 
 /*
  * This is the key we set when evicting a keyslot. This *should* be the all 0's
@@ -224,7 +225,8 @@ static bool blk_crypto_split_bio_if_needed(struct bio **bio_ptr)
 	if (num_sectors < bio_sectors(bio)) {
 		struct bio *split_bio;
 
-		split_bio = bio_split(bio, num_sectors, GFP_NOIO, NULL);
+		split_bio = bio_split(bio, num_sectors, GFP_NOIO,
+				      &crypto_bio_split);
 		if (!split_bio) {
 			bio->bi_status = BLK_STS_RESOURCE;
 			return false;
@@ -538,9 +540,13 @@ static int blk_crypto_fallback_init(void)
 
 	prandom_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
 
-	err = blk_ksm_init(&blk_crypto_ksm, blk_crypto_num_keyslots);
+	err = bioset_init(&crypto_bio_split, 64, 0, 0);
 	if (err)
 		goto out;
+
+	err = blk_ksm_init(&blk_crypto_ksm, blk_crypto_num_keyslots);
+	if (err)
+		goto fail_free_bioset;
 	err = -ENOMEM;
 
 	blk_crypto_ksm.ksm_ll_ops = blk_crypto_ksm_ll_ops;
@@ -591,6 +597,8 @@ static int blk_crypto_fallback_init(void)
 	destroy_workqueue(blk_crypto_wq);
 fail_free_ksm:
 	blk_ksm_destroy(&blk_crypto_ksm);
+fail_free_bioset:
+	bioset_exit(&crypto_bio_split);
 out:
 	return err;
 }
-- 
2.29.2

Re: [PATCH 1/4] block-crypto-fallback: use a bio_set for splitting bios

[Satya Tangirala]

On Wed, Feb 24, 2021 at 08:24:04AM +0100, Christoph Hellwig wrote:
> bio_split with a NULL bs argumen used to fall back to kmalloc the
> bio, which does not guarantee forward progress and could to deadlocks.
> Now that the overloading of the NULL bs argument to bio_alloc_bioset
> has been removed it crashes instead.  Fix all that by using a special
> crafted bioset.
> 
> Fixes: 3175199ab0ac ("block: split bio_kmalloc from bio_alloc_bioset")
> Reported-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: John Stultz <john.stultz@linaro.org>
> ---
>  block/blk-crypto-fallback.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
> index e8327c50d7c9f4..c176b7af56a7a5 100644
> --- a/block/blk-crypto-fallback.c
> +++ b/block/blk-crypto-fallback.c
> @@ -80,6 +80,7 @@ static struct blk_crypto_keyslot {
>  static struct blk_keyslot_manager blk_crypto_ksm;
>  static struct workqueue_struct *blk_crypto_wq;
>  static mempool_t *blk_crypto_bounce_page_pool;
> +static struct bio_set crypto_bio_split;
>  
>  /*
>   * This is the key we set when evicting a keyslot. This *should* be the all 0's
> @@ -224,7 +225,8 @@ static bool blk_crypto_split_bio_if_needed(struct bio **bio_ptr)
>  	if (num_sectors < bio_sectors(bio)) {
>  		struct bio *split_bio;
>  
> -		split_bio = bio_split(bio, num_sectors, GFP_NOIO, NULL);
> +		split_bio = bio_split(bio, num_sectors, GFP_NOIO,
> +				      &crypto_bio_split);
>  		if (!split_bio) {
>  			bio->bi_status = BLK_STS_RESOURCE;
>  			return false;
> @@ -538,9 +540,13 @@ static int blk_crypto_fallback_init(void)
>  
>  	prandom_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
>  
> -	err = blk_ksm_init(&blk_crypto_ksm, blk_crypto_num_keyslots);
> +	err = bioset_init(&crypto_bio_split, 64, 0, 0);
>  	if (err)
>  		goto out;
> +
> +	err = blk_ksm_init(&blk_crypto_ksm, blk_crypto_num_keyslots);
> +	if (err)
> +		goto fail_free_bioset;
>  	err = -ENOMEM;
>  
>  	blk_crypto_ksm.ksm_ll_ops = blk_crypto_ksm_ll_ops;
> @@ -591,6 +597,8 @@ static int blk_crypto_fallback_init(void)
>  	destroy_workqueue(blk_crypto_wq);
>  fail_free_ksm:
>  	blk_ksm_destroy(&blk_crypto_ksm);
> +fail_free_bioset:
> +	bioset_exit(&crypto_bio_split);
>  out:
>  	return err;
>  }
> -- 
> 2.29.2
> 
Thanks for fixing this! If needed, feel free to add
Reviewed-by: Satya Tangirala <satyat@google.com>

Re: [PATCH 1/4] block-crypto-fallback: use a bio_set for splitting bios

[Eric Biggers]

On Wed, Feb 24, 2021 at 08:24:04AM +0100, Christoph Hellwig wrote:
> bio_split with a NULL bs argumen used to fall back to kmalloc the
> bio, which does not guarantee forward progress and could to deadlocks.

"argumen" => "argument"
"could to" => "could lead to"

> Now that the overloading of the NULL bs argument to bio_alloc_bioset
> has been removed it crashes instead.  Fix all that by using a special
> crafted bioset.
> 
> Fixes: 3175199ab0ac ("block: split bio_kmalloc from bio_alloc_bioset")
> Reported-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: John Stultz <john.stultz@linaro.org>
> ---
>  block/blk-crypto-fallback.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-crypto-fallback.c b/block/blk-crypto-fallback.c
> index e8327c50d7c9f4..c176b7af56a7a5 100644
> --- a/block/blk-crypto-fallback.c
> +++ b/block/blk-crypto-fallback.c
> @@ -80,6 +80,7 @@ static struct blk_crypto_keyslot {
>  static struct blk_keyslot_manager blk_crypto_ksm;
>  static struct workqueue_struct *blk_crypto_wq;
>  static mempool_t *blk_crypto_bounce_page_pool;
> +static struct bio_set crypto_bio_split;

Something like "blk_crypto_bioset" might be a better name, since the other
variables above are prefixed with "blk_crypto".

>  /*
>   * This is the key we set when evicting a keyslot. This *should* be the all 0's
> @@ -224,7 +225,8 @@ static bool blk_crypto_split_bio_if_needed(struct bio **bio_ptr)
>  	if (num_sectors < bio_sectors(bio)) {
>  		struct bio *split_bio;
>  
> -		split_bio = bio_split(bio, num_sectors, GFP_NOIO, NULL);
> +		split_bio = bio_split(bio, num_sectors, GFP_NOIO,
> +				      &crypto_bio_split);
>  		if (!split_bio) {
>  			bio->bi_status = BLK_STS_RESOURCE;
>  			return false;
> @@ -538,9 +540,13 @@ static int blk_crypto_fallback_init(void)
>  
>  	prandom_bytes(blank_key, BLK_CRYPTO_MAX_KEY_SIZE);
>  
> -	err = blk_ksm_init(&blk_crypto_ksm, blk_crypto_num_keyslots);
> +	err = bioset_init(&crypto_bio_split, 64, 0, 0);
>  	if (err)
>  		goto out;
> +
> +	err = blk_ksm_init(&blk_crypto_ksm, blk_crypto_num_keyslots);
> +	if (err)
> +		goto fail_free_bioset;
>  	err = -ENOMEM;
>  
>  	blk_crypto_ksm.ksm_ll_ops = blk_crypto_ksm_ll_ops;
> @@ -591,6 +597,8 @@ static int blk_crypto_fallback_init(void)
>  	destroy_workqueue(blk_crypto_wq);
>  fail_free_ksm:
>  	blk_ksm_destroy(&blk_crypto_ksm);
> +fail_free_bioset:
> +	bioset_exit(&crypto_bio_split);
>  out:
>  	return err;
>  }

Anyway, this works.  Feel free to add:

	Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric

[PATCH 2/4] block: fix bounce_clone_bio for passthrough bios

[Christoph Hellwig]

Now that bio_alloc_bioset does not fall back to kmalloc for a NULL
bio_set, handle that case explicitly and simplify the calling
conventions.

Based on an earlier patch from Chaitanya Kulkarni.

Fixes: 3175199ab0ac ("block: split bio_kmalloc from bio_alloc_bioset")
Reported-by: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bounce.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/bounce.c b/block/bounce.c
index fc55314aa4269a..79d81221446dfe 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -214,8 +214,7 @@ static void bounce_end_io_read_isa(struct bio *bio)
 	__bounce_end_io_read(bio, &isa_page_pool);
 }
 
-static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask,
-		struct bio_set *bs)
+static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask)
 {
 	struct bvec_iter iter;
 	struct bio_vec bv;
@@ -242,8 +241,11 @@ static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask,
 	 *    asking for trouble and would force extra work on
 	 *    __bio_clone_fast() anyways.
 	 */
-
-	bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);
+	if (bio_is_passthrough(bio_src))
+		bio = bio_kmalloc(gfp_mask, bio_segments(bio_src));
+	else
+		bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src),
+				       &bounce_bio_set);
 	if (!bio)
 		return NULL;
 	bio->bi_bdev		= bio_src->bi_bdev;
@@ -296,7 +298,6 @@ static void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig,
 	unsigned i = 0;
 	bool bounce = false;
 	int sectors = 0;
-	bool passthrough = bio_is_passthrough(*bio_orig);
 
 	bio_for_each_segment(from, *bio_orig, iter) {
 		if (i++ < BIO_MAX_PAGES)
@@ -307,14 +308,14 @@ static void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig,
 	if (!bounce)
 		return;
 
-	if (!passthrough && sectors < bio_sectors(*bio_orig)) {
+	if (!bio_is_passthrough(*bio_orig) &&
+	    sectors < bio_sectors(*bio_orig)) {
 		bio = bio_split(*bio_orig, sectors, GFP_NOIO, &bounce_bio_split);
 		bio_chain(bio, *bio_orig);
 		submit_bio_noacct(*bio_orig);
 		*bio_orig = bio;
 	}
-	bio = bounce_clone_bio(*bio_orig, GFP_NOIO, passthrough ? NULL :
-			&bounce_bio_set);
+	bio = bounce_clone_bio(*bio_orig, GFP_NOIO);
 
 	/*
 	 * Bvec table can't be updated by bio_for_each_segment_all(),
-- 
2.29.2

Re: [PATCH 2/4] block: fix bounce_clone_bio for passthrough bios

[Chaitanya Kulkarni]

On 2/23/21 23:29, Christoph Hellwig wrote:
> Now that bio_alloc_bioset does not fall back to kmalloc for a NULL
> bio_set, handle that case explicitly and simplify the calling
> conventions.
>
> Based on an earlier patch from Chaitanya Kulkarni.
>
> Fixes: 3175199ab0ac ("block: split bio_kmalloc from bio_alloc_bioset")
> Reported-by: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[PATCH 3/4] block: remove the gfp_mask argument to bounce_clone_bio

[Christoph Hellwig]

The only caller always passes GFP_NOIO.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bounce.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/bounce.c b/block/bounce.c
index 79d81221446dfe..417faaac36b691 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -214,7 +214,7 @@ static void bounce_end_io_read_isa(struct bio *bio)
 	__bounce_end_io_read(bio, &isa_page_pool);
 }
 
-static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask)
+static struct bio *bounce_clone_bio(struct bio *bio_src)
 {
 	struct bvec_iter iter;
 	struct bio_vec bv;
@@ -242,9 +242,9 @@ static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask)
 	 *    __bio_clone_fast() anyways.
 	 */
 	if (bio_is_passthrough(bio_src))
-		bio = bio_kmalloc(gfp_mask, bio_segments(bio_src));
+		bio = bio_kmalloc(GFP_NOIO, bio_segments(bio_src));
 	else
-		bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src),
+		bio = bio_alloc_bioset(GFP_NOIO, bio_segments(bio_src),
 				       &bounce_bio_set);
 	if (!bio)
 		return NULL;
@@ -271,11 +271,11 @@ static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask)
 		break;
 	}
 
-	if (bio_crypt_clone(bio, bio_src, gfp_mask) < 0)
+	if (bio_crypt_clone(bio, bio_src, GFP_NOIO) < 0)
 		goto err_put;
 
 	if (bio_integrity(bio_src) &&
-	    bio_integrity_clone(bio, bio_src, gfp_mask) < 0)
+	    bio_integrity_clone(bio, bio_src, GFP_NOIO) < 0)
 		goto err_put;
 
 	bio_clone_blkg_association(bio, bio_src);
@@ -315,7 +315,7 @@ static void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig,
 		submit_bio_noacct(*bio_orig);
 		*bio_orig = bio;
 	}
-	bio = bounce_clone_bio(*bio_orig, GFP_NOIO);
+	bio = bounce_clone_bio(*bio_orig);
 
 	/*
 	 * Bvec table can't be updated by bio_for_each_segment_all(),
-- 
2.29.2

Re: [PATCH 3/4] block: remove the gfp_mask argument to bounce_clone_bio

[Chaitanya Kulkarni]

On 2/23/21 23:29, Christoph Hellwig wrote:
> The only caller always passes GFP_NOIO.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

[PATCH 4/4] block: memory allocations in bounce_clone_bio must not fail

[Christoph Hellwig]

The caller can't cope with a failure from bounce_clone_bio, so
use __GFP_NOFAIL for the passthrough case.  bio_alloc_bioset already
won't fail due to the use of mempools.

And yes, we need to get rid of this bock layer bouncing code entirely
sooner or later..

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bounce.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/bounce.c b/block/bounce.c
index 417faaac36b691..87983a35079c22 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -242,12 +242,11 @@ static struct bio *bounce_clone_bio(struct bio *bio_src)
 	 *    __bio_clone_fast() anyways.
 	 */
 	if (bio_is_passthrough(bio_src))
-		bio = bio_kmalloc(GFP_NOIO, bio_segments(bio_src));
+		bio = bio_kmalloc(GFP_NOIO | __GFP_NOFAIL,
+				  bio_segments(bio_src));
 	else
 		bio = bio_alloc_bioset(GFP_NOIO, bio_segments(bio_src),
 				       &bounce_bio_set);
-	if (!bio)
-		return NULL;
 	bio->bi_bdev		= bio_src->bi_bdev;
 	if (bio_flagged(bio_src, BIO_REMAPPED))
 		bio_set_flag(bio, BIO_REMAPPED);
-- 
2.29.2

Re: [PATCH 4/4] block: memory allocations in bounce_clone_bio must not fail

[Ming Lei]

On Wed, Feb 24, 2021 at 08:24:07AM +0100, Christoph Hellwig wrote:
> The caller can't cope with a failure from bounce_clone_bio, so
> use __GFP_NOFAIL for the passthrough case.  bio_alloc_bioset already
> won't fail due to the use of mempools.
> 
> And yes, we need to get rid of this bock layer bouncing code entirely
> sooner or later..
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/bounce.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/block/bounce.c b/block/bounce.c
> index 417faaac36b691..87983a35079c22 100644
> --- a/block/bounce.c
> +++ b/block/bounce.c
> @@ -242,12 +242,11 @@ static struct bio *bounce_clone_bio(struct bio *bio_src)
>  	 *    __bio_clone_fast() anyways.
>  	 */
>  	if (bio_is_passthrough(bio_src))
> -		bio = bio_kmalloc(GFP_NOIO, bio_segments(bio_src));
> +		bio = bio_kmalloc(GFP_NOIO | __GFP_NOFAIL,
> +				  bio_segments(bio_src));

bio_kmalloc() still may fail if bio_segments(bio_src) is > UIO_MAXIOV.


-- 
Ming

Re: [PATCH 4/4] block: memory allocations in bounce_clone_bio must not fail

[Christoph Hellwig]

On Wed, Feb 24, 2021 at 07:19:52PM +0800, Ming Lei wrote:
> >  	if (bio_is_passthrough(bio_src))
> > -		bio = bio_kmalloc(GFP_NOIO, bio_segments(bio_src));
> > +		bio = bio_kmalloc(GFP_NOIO | __GFP_NOFAIL,
> > +				  bio_segments(bio_src));
> 
> bio_kmalloc() still may fail if bio_segments(bio_src) is > UIO_MAXIOV.

Yes, but bio_kmalloc is what is used to allocate the passthrough
requests to start with, so we'd not even make it here.

Re: [PATCH 4/4] block: memory allocations in bounce_clone_bio must not fail

[Ming Lei]

On Wed, Feb 24, 2021 at 05:12:36PM +0100, Christoph Hellwig wrote:
> On Wed, Feb 24, 2021 at 07:19:52PM +0800, Ming Lei wrote:
> > >  	if (bio_is_passthrough(bio_src))
> > > -		bio = bio_kmalloc(GFP_NOIO, bio_segments(bio_src));
> > > +		bio = bio_kmalloc(GFP_NOIO | __GFP_NOFAIL,
> > > +				  bio_segments(bio_src));
> > 
> > bio_kmalloc() still may fail if bio_segments(bio_src) is > UIO_MAXIOV.
> 
> Yes, but bio_kmalloc is what is used to allocate the passthrough
> requests to start with, so we'd not even make it here.

The original bio_kmalloc() may start with allowed nr_iovecs , but
later more pages are retrieved from iov_iter and added to the bio,
see bio_map_user_iov(). Then bounce_clone_bio() will see too big
bio_segments(bio_src) to be held in UIO_MAXIOV vecs.

This behavior is similar with blkdev_direct_IO().

-- 
Ming

Re: [PATCH 4/4] block: memory allocations in bounce_clone_bio must not fail

[Chaitanya Kulkarni]

On 2/23/21 23:29, Christoph Hellwig wrote:
> The caller can't cope with a failure from bounce_clone_bio, so
> use __GFP_NOFAIL for the passthrough case.  bio_alloc_bioset already
> won't fail due to the use of mempools.
>
> And yes, we need to get rid of this bock layer bouncing code entirely
> sooner or later..
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Re: bio_kmalloc related fixes for 5.12

[Jens Axboe]

On 2/24/21 12:24 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> a few fixes for 5.12 below.

Applied, thanks.

-- 
Jens Axboe