diff mbox series

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

Message ID 20210224072407.46363-2-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [1/4] block-crypto-fallback: use a bio_set for splitting bios | expand

Commit Message

Christoph Hellwig Feb. 24, 2021, 7:24 a.m. UTC
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(-)

Comments

Satya Tangirala Feb. 24, 2021, 4:17 p.m. UTC | #1
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>
Eric Biggers Feb. 24, 2021, 11:23 p.m. UTC | #2
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
diff mbox series

Patch

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;
 }