[REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[John Stultz]

Hey all,
  In testing Linus' HEAD today I found another[1] regression in the block merge.

This time I see a crash on my hikey960 board shortly after booting
ASOP. See the log below.

I bisected the issue down to "block: split bio_kmalloc from bio_alloc_bioset":
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3175199ab0ac8c874ec25c6bf169f74888917435

But unfortunately that does not revert cleanly against Linus' HEAD.

It seems like the issue is that the function is no longer handling the
case where the bio_set *bs is NULL as is done here:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/blk-crypto-fallback.c#n230

If there's anything I can do to help debug this issue, please let me know!

thanks
-john

[1]: "add a disk_uevent helper" also was giving me trouble as reported here:
https://lore.kernel.org/lkml/CALAqxLU3B8YcS_MTnr2Lpasvn8oLJvD2qO4hkfkZeEwVNfeHXg@mail.gmail.com/


[   34.600558] Unable to handle kernel read from unreadable memory at
virtual address 0000000000000068
[   34.609819] Mem abort info:
[   34.612644]   ESR = 0x96000005
[   34.615707]   EC = 0x25: DABT (current EL), IL = 32 bits
[   34.621157]   SET = 0, FnV = 0
[   34.624288]   EA = 0, S1PTW = 0
[   34.628312] Data abort info:
[   34.631236]   ISV = 0, ISS = 0x00000005
[   34.635222]   CM = 0, WnR = 0
[   34.638186] user pgtable: 4k pages, 39-bit VAs, pgdp=000000000774f000
[   34.644639] [0000000000000068] pgd=0000000000000000,
p4d=0000000000000000, pud=0000000000000000
[   34.653381] Internal error: Oops: 96000005 [#1] PREEMPT SMP
[   34.658954] Modules linked in:
[   34.662007] CPU: 0 PID: 173 Comm: kworker/u16:3 Not tainted
5.11.0-04196-ga8e8932e4ae #559
[   34.670270] Hardware name: HiKey960 (DT)
[   34.674190] Workqueue: writeback wb_workfn (flush-8:48)
[   34.679429] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO BTYPE=--)
[   34.685435] pc : bio_alloc_bioset+0x14/0x230
[   34.689720] lr : bio_clone_fast+0x28/0x80
[   34.693728] sp : ffffffc012a13530
[   34.697036] x29: ffffffc012a13530 x28: 0000000000003400
[   34.702351] x27: 0000000000000000 x26: ffffff8001993040
[   34.703203] dwmmc_k3 ff3ff000.dwmmc2: Unexpected interrupt latency
[   34.707662] x25: 0000000000000001 x24: ffffff8001fc8800
[   34.719145] x23: ffffffffffffffff x22: ffffffc012a137e8
[   34.724456] x21: 0000000000000c00 x20: ffffff8001fc8800
[   34.729767] x19: 0000000000000800 x18: fffffffe013efe80
[   34.735078] x17: 0000000000000068 x16: fffffffe012aa440
[   34.740389] x15: 0000000000001000 x14: ffffff8001fc8800
[   34.745700] x13: ffffff805c3e4000 x12: 00000000001ce000
[   34.751009] x11: 00000000000000fb x10: 0000000000001000
[   34.756320] x9 : 0000000000000033 x8 : 00000000000d9000
[   34.761631] x7 : 0000000000000000 x6 : 000000000000a000
[   34.766941] x5 : 0000000000000c00 x4 : 0000000000001000
[   34.772251] x3 : 0000000000000000 x2 : 0000000000000000
[   34.777561] x1 : 0000000000000000 x0 : 0000000000000c00
[   34.782873] Call trace:
[   34.784901] ueventd: LoadWithAliases was unable to load platform:regulatory
[   34.785313]  bio_alloc_bioset+0x14/0x230
[   34.796189]  bio_clone_fast+0x28/0x80
[   34.799848]  bio_split+0x50/0xd0
[   34.803072]  blk_crypto_fallback_encrypt_bio+0x2ec/0x5e8
[   34.808384]  blk_crypto_fallback_bio_prep+0xfc/0x140
[   34.813345]  __blk_crypto_bio_prep+0x13c/0x150
[   34.817784]  submit_bio_noacct+0x3c0/0x548
[   34.821880]  submit_bio+0x48/0x200
[   34.825278]  ext4_io_submit+0x50/0x68
[   34.828939]  ext4_writepages+0x558/0xca8
[   34.832860]  do_writepages+0x58/0x108
[   34.836522]  __writeback_single_inode+0x44/0x510
[   34.841137]  writeback_sb_inodes+0x1e0/0x4a8
[   34.845404]  __writeback_inodes_wb+0x78/0xe8
[   34.849670]  wb_writeback+0x274/0x3e8
[   34.853328]  wb_workfn+0x308/0x5f0
[   34.856726]  process_one_work+0x1ec/0x4d0
[   34.860734]  worker_thread+0x44/0x478
[   34.864392]  kthread+0x140/0x150
[   34.867618]  ret_from_fork+0x10/0x30
[   34.871197] Code: a9ba7bfd 910003fd f9000bf3 7900bfa1 (f9403441)
[   34.877289] ---[ end trace e6c2a3ab108278f0 ]---
[   34.893636] Kernel panic - not syncing: Oops: Fatal exception

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Chaitanya Kulkarni]

On 2/22/21 19:07, John Stultz wrote:
> [   34.784901] ueventd: LoadWithAliases was unable to load platform:regulatory
> [   34.785313]  bio_alloc_bioset+0x14/0x230
> [   34.796189]  bio_clone_fast+0x28/0x80
> [   34.799848]  bio_split+0x50/0xd0
> [   34.803072]  blk_crypto_fallback_encrypt_bio+0x2ec/0x5e8
> [   34.808384]  blk_crypto_fallback_bio_prep+0xfc/0x140
> [   34.813345]  __blk_crypto_bio_prep+0x13c/0x150
> [   34.817784]  submit_bio_noacct+0x3c0/0x548
> [   34.821880]  submit_bio+0x48/0x200
> [   34.825278]  ext4_io_submit+0x50/0x68
> [   34.828939]  ext4_writepages+0x558/0xca8
> [   34.832860]  do_writepages+0x58/0x108
> [   34.836522]  __writeback_single_inode+0x44/0x510
> [   34.841137]  writeback_sb_inodes+0x1e0/0x4a8
> [   34.845404]  __writeback_inodes_wb+0x78/0xe8
> [   34.849670]  wb_writeback+0x274/0x3e8
> [   34.853328]  wb_workfn+0x308/0x5f0
> [   34.856726]  process_one_work+0x1ec/0x4d0
> [   34.860734]  worker_thread+0x44/0x478
> [   34.864392]  kthread+0x140/0x150
> [   34.867618]  ret_from_fork+0x10/0x30
> [   34.871197] Code: a9ba7bfd 910003fd f9000bf3 7900bfa1 (f9403441)
> [   34.877289] ---[ end trace e6c2a3ab108278f0 ]---
> [   34.893636] Kernel panic - not syncing: Oops: Fatal exception
>

If you have time then until you get the reply from others, can you try
following patch ?

diff --git a/block/bio.c b/block/bio.c
index a1c4d2900c7a..9976400ec66a 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -663,7 +663,10 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t
gfp_mask, struct bio_set *bs)
 {
        struct bio *b;
 
-       b = bio_alloc_bioset(gfp_mask, 0, bs);
+       if (bs)
+               b = bio_alloc_bioset(gfp_mask, 0, bs);
+       else
+               b = bio_kmalloc(gfp_mask, 0);
        if (!b)
                return NULL;

P.S.This is purely based on the code inspection and it may not solve your
issue. Proceed with the caution as it may *break* your system.

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[John Stultz]

On Mon, Feb 22, 2021 at 7:39 PM Chaitanya Kulkarni
<Chaitanya.Kulkarni@wdc.com> wrote:
>
> On 2/22/21 19:07, John Stultz wrote:
> > [   34.784901] ueventd: LoadWithAliases was unable to load platform:regulatory
> > [   34.785313]  bio_alloc_bioset+0x14/0x230
> > [   34.796189]  bio_clone_fast+0x28/0x80
> > [   34.799848]  bio_split+0x50/0xd0
> > [   34.803072]  blk_crypto_fallback_encrypt_bio+0x2ec/0x5e8
> > [   34.808384]  blk_crypto_fallback_bio_prep+0xfc/0x140
> > [   34.813345]  __blk_crypto_bio_prep+0x13c/0x150
> > [   34.817784]  submit_bio_noacct+0x3c0/0x548
> > [   34.821880]  submit_bio+0x48/0x200
> > [   34.825278]  ext4_io_submit+0x50/0x68
> > [   34.828939]  ext4_writepages+0x558/0xca8
> > [   34.832860]  do_writepages+0x58/0x108
> > [   34.836522]  __writeback_single_inode+0x44/0x510
> > [   34.841137]  writeback_sb_inodes+0x1e0/0x4a8
> > [   34.845404]  __writeback_inodes_wb+0x78/0xe8
> > [   34.849670]  wb_writeback+0x274/0x3e8
> > [   34.853328]  wb_workfn+0x308/0x5f0
> > [   34.856726]  process_one_work+0x1ec/0x4d0
> > [   34.860734]  worker_thread+0x44/0x478
> > [   34.864392]  kthread+0x140/0x150
> > [   34.867618]  ret_from_fork+0x10/0x30
> > [   34.871197] Code: a9ba7bfd 910003fd f9000bf3 7900bfa1 (f9403441)
> > [   34.877289] ---[ end trace e6c2a3ab108278f0 ]---
> > [   34.893636] Kernel panic - not syncing: Oops: Fatal exception
> >
>
> If you have time then until you get the reply from others, can you try
> following patch ?
>
> diff --git a/block/bio.c b/block/bio.c
> index a1c4d2900c7a..9976400ec66a 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -663,7 +663,10 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t
> gfp_mask, struct bio_set *bs)
>  {
>         struct bio *b;
>
> -       b = bio_alloc_bioset(gfp_mask, 0, bs);
> +       if (bs)
> +               b = bio_alloc_bioset(gfp_mask, 0, bs);
> +       else
> +               b = bio_kmalloc(gfp_mask, 0);
>         if (!b)
>                 return NULL;
>
> P.S.This is purely based on the code inspection and it may not solve your
> issue. Proceed with the caution as it may *break* your system.

So with an initial quick test, this patch (along with the follow-on
one you sent) seems to avoid the issue.

I'm wondering if given there are multiple call sites, that in
bio_alloc_bioset() would something like the following make more sense?
(apologies, copy pasted so this is whitespace corrupted)
thanks
-john

diff --git a/block/bio.c b/block/bio.c
index a1c4d2900c7a..391d5cde79fc 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -402,6 +402,9 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask,
unsigned short nr_iovecs,
        struct bio *bio;
        void *p;

+       if(!bs)
+               return bio_kmalloc(gfp_mask, 0);
+
        /* should not use nobvec bioset for nr_iovecs > 0 */
        if (WARN_ON_ONCE(!mempool_initialized(&bs->bvec_pool) && nr_iovecs > 0))
                return NULL;

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Chaitanya Kulkarni]

On 2/22/21 20:22, John Stultz wrote:
> On Mon, Feb 22, 2021 at 7:39 PM Chaitanya Kulkarni
> <Chaitanya.Kulkarni@wdc.com> wrote:
>> On 2/22/21 19:07, John Stultz wrote:
>>> [   34.784901] ueventd: LoadWithAliases was unable to load platform:regulatory
>>> [   34.785313]  bio_alloc_bioset+0x14/0x230
>>> [   34.796189]  bio_clone_fast+0x28/0x80
>>> [   34.799848]  bio_split+0x50/0xd0
>>> [   34.803072]  blk_crypto_fallback_encrypt_bio+0x2ec/0x5e8
>>> [   34.808384]  blk_crypto_fallback_bio_prep+0xfc/0x140
>>> [   34.813345]  __blk_crypto_bio_prep+0x13c/0x150
>>> [   34.817784]  submit_bio_noacct+0x3c0/0x548
>>> [   34.821880]  submit_bio+0x48/0x200
>>> [   34.825278]  ext4_io_submit+0x50/0x68
>>> [   34.828939]  ext4_writepages+0x558/0xca8
>>> [   34.832860]  do_writepages+0x58/0x108
>>> [   34.836522]  __writeback_single_inode+0x44/0x510
>>> [   34.841137]  writeback_sb_inodes+0x1e0/0x4a8
>>> [   34.845404]  __writeback_inodes_wb+0x78/0xe8
>>> [   34.849670]  wb_writeback+0x274/0x3e8
>>> [   34.853328]  wb_workfn+0x308/0x5f0
>>> [   34.856726]  process_one_work+0x1ec/0x4d0
>>> [   34.860734]  worker_thread+0x44/0x478
>>> [   34.864392]  kthread+0x140/0x150
>>> [   34.867618]  ret_from_fork+0x10/0x30
>>> [   34.871197] Code: a9ba7bfd 910003fd f9000bf3 7900bfa1 (f9403441)
>>> [   34.877289] ---[ end trace e6c2a3ab108278f0 ]---
>>> [   34.893636] Kernel panic - not syncing: Oops: Fatal exception
>>>
>> If you have time then until you get the reply from others, can you try
>> following patch ?
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index a1c4d2900c7a..9976400ec66a 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -663,7 +663,10 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t
>> gfp_mask, struct bio_set *bs)
>>  {
>>         struct bio *b;
>>
>> -       b = bio_alloc_bioset(gfp_mask, 0, bs);
>> +       if (bs)
>> +               b = bio_alloc_bioset(gfp_mask, 0, bs);
>> +       else
>> +               b = bio_kmalloc(gfp_mask, 0);
>>         if (!b)
>>                 return NULL;
>>
>> P.S.This is purely based on the code inspection and it may not solve your
>> issue. Proceed with the caution as it may *break* your system.
> So with an initial quick test, this patch (along with the follow-on
> one you sent) seems to avoid the issue.
Thanks for bisecting, the patch you are referring to is with the similar
fix for bio bounce ? 
> I'm wondering if given there are multiple call sites, that in
> bio_alloc_bioset() would something like the following make more sense?
> (apologies, copy pasted so this is whitespace corrupted)
> thanks
> -john
I've asked Christoph about how we can go about this issue, based on his
reply I can send a patch.

I think having kmalloc is what we want to avoid in the fast path, with
that in
mind in my opinion we need to fix the call sites with kmalloc call if number
is small,  which I think it is. Let's wait.

Meanwhile it will be great if you can share the result of the thorough
testing.

> diff --git a/block/bio.c b/block/bio.c
> index a1c4d2900c7a..391d5cde79fc 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -402,6 +402,9 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask,
> unsigned short nr_iovecs,
>         struct bio *bio;
>         void *p;
>
> +       if(!bs)
> +               return bio_kmalloc(gfp_mask, 0);
it may not work since nr_iovecs needs to be passed
> +
>         /* should not use nobvec bioset for nr_iovecs > 0 */
>         if (WARN_ON_ONCE(!mempool_initialized(&bs->bvec_pool) && nr_iovecs > 0))
>                 return NULL;
>

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Christoph Hellwig]

On Mon, Feb 22, 2021 at 08:22:09PM -0800, John Stultz wrote:
> I'm wondering if given there are multiple call sites, that in
> bio_alloc_bioset() would something like the following make more sense?
> (apologies, copy pasted so this is whitespace corrupted)
> thanks

No.  The fallback from the mempool backing to plain kmalloc is highly
dangerous, which is one of the reasons it was removed.  The other being
that we don't want to disturb the fast path with this slow path.

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Chaitanya Kulkarni]

Christoph,

On 2/22/21 19:07, John Stultz wrote:
> [   34.785313]  bio_alloc_bioset+0x14/0x230
> [   34.796189]  bio_clone_fast+0x28/0x80
> [   34.799848]  bio_split+0x50/0xd0
> [   34.803072]  blk_crypto_fallback_encrypt_bio+0x2ec/0x5e8
> [   34.808384]  blk_crypto_fallback_bio_prep+0xfc/0x140
> [   34.813345]  __blk_crypto_bio_prep+0x13c/0x150
> [   34.817784]  submit_bio_noacct+0x3c0/0x548
> [   34.821880]  submit_bio+0x48/0x200
> [   34.825278]  ext4_io_submit+0x50/0x68
> [   34.828939]  ext4_writepages+0x558/0xca8
> [   34.832860]  do_writepages+0x58/0x108
> [   34.836522]  __writeback_single_inode+0x44/0x510
> [   34.841137]  writeback_sb_inodes+0x1e0/0x4a8
> [   34.845404]  __writeback_inodes_wb+0x78/0xe8
> [   34.849670]  wb_writeback+0x274/0x3e8
> [   34.853328]  wb_workfn+0x308/0x5f0
> [   34.856726]  process_one_work+0x1ec/0x4d0
> [   34.860734]  worker_thread+0x44/0x478
> [   34.864392]  kthread+0x140/0x150
> [   34.867618]  ret_from_fork+0x10/0x30
> [   34.871197] Code: a9ba7bfd 910003fd f9000bf3 7900bfa1 (f9403441)
> [   34.877289] ---[ end trace e6c2a3ab108278f0 ]---
> [   34.893636] Kernel panic - not syncing: Oops: Fatal exception
>
Looking at the other call sites do we need something like following ?

diff --git a/block/bounce.c b/block/bounce.c
index fc55314aa426..f8a3656e89c3 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -217,6 +217,7 @@ static void bounce_end_io_read_isa(struct bio *bio)
 static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask,
                struct bio_set *bs)
 {
+       unsigned int nr_iovecs = bio_segments(bio_src);
        struct bvec_iter iter;
        struct bio_vec bv;
        struct bio *bio;
@@ -243,7 +244,11 @@ static struct bio *bounce_clone_bio(struct bio
*bio_src, gfp_t gfp_mask,
         *    __bio_clone_fast() anyways.
         */
 
-       bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);
+       if (bs)
+               bio = bio_alloc_bioset(gfp_mask, nr_iovecs, bs);
+       else
+               bio = bio_kmalloc(gfp_mask, nr_iovecs);
+
        if (!bio)
                return NULL;
        bio->bi_bdev            = bio_src->bi_bdev;

Since __blk_queue_bounce() passes the NULL for the passthru case as a
bio_set value ?

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Christoph Hellwig]

On Tue, Feb 23, 2021 at 03:51:23AM +0000, Chaitanya Kulkarni wrote:
> Looking at the other call sites do we need something like following ?

> Since __blk_queue_bounce() passes the NULL for the passthru case as a
> bio_set value ?

Well, that is a somewhat odd calling convention.  What about the patch below
instead?  That being we really need to kill this bouncing code off..


diff --git a/block/bounce.c b/block/bounce.c
index fc55314aa4269a..789fbcacb1e92a 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -214,9 +214,9 @@ 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, bool passthrough)
 {
+	unsigned int nr_vecs = bio_segments(bio_src);
 	struct bvec_iter iter;
 	struct bio_vec bv;
 	struct bio *bio;
@@ -242,8 +242,10 @@ 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 (passthrough)
+		bio = bio_kmalloc(GFP_NOIO, nr_vecs);
+	else
+		bio = bio_alloc_bioset(GFP_NOIO, nr_vecs, &bounce_bio_set);
 	if (!bio)
 		return NULL;
 	bio->bi_bdev		= bio_src->bi_bdev;
@@ -269,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);
@@ -313,8 +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, passthrough ? NULL :
-			&bounce_bio_set);
+	bio = bounce_clone_bio(*bio_orig, passthrough);
 
 	/*
 	 * Bvec table can't be updated by bio_for_each_segment_all(),

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Chaitanya Kulkarni]

On 2/22/21 23:10, Christoph Hellwig wrote:
> Well, that is a somewhat odd calling convention.  What about the patch below
> instead?  That being we really need to kill this bouncing code off..
If we can kill it off soon it will be great.
>
> diff --git a/block/bounce.c b/block/bounce.c
> index fc55314aa4269a..789fbcacb1e92a 100644
> --- a/block/bounce.c
> +++ b/block/bounce.c
> @@ -214,9 +214,9 @@ 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, bool passthrough)
>  {
> +	unsigned int nr_vecs = bio_segments(bio_src);
>  	struct bvec_iter iter;
>  	struct bio_vec bv;
>  	struct bio *bio;
> @@ -242,8 +242,10 @@ 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 (passthrough)
> +		bio = bio_kmalloc(GFP_NOIO, nr_vecs);
> +	else
> +		bio = bio_alloc_bioset(GFP_NOIO, nr_vecs, &bounce_bio_set);
>  	if (!bio)
>  		return NULL;
>  	bio->bi_bdev		= bio_src->bi_bdev;
> @@ -269,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);
> @@ -313,8 +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, passthrough ? NULL :
> -			&bounce_bio_set);
> +	bio = bounce_clone_bio(*bio_orig, passthrough);
>  
>  	/*
>  	 * Bvec table can't be updated by bio_for_each_segment_all(),
>
Seems like this fixes the issue and does the cleanup in one patch, looks
good.

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Chaitanya Kulkarni]

On 2/22/21 23:10, Christoph Hellwig wrote:
> On Tue, Feb 23, 2021 at 03:51:23AM +0000, Chaitanya Kulkarni wrote:
>> Looking at the other call sites do we need something like following ?
>> Since __blk_queue_bounce() passes the NULL for the passthru case as a
>> bio_set value ?
> Well, that is a somewhat odd calling convention.  What about the patch below
> instead?  That being we really need to kill this bouncing code off..
I assume you are sending this patch, let me know otherwise.
If you do please add, looks good.

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

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Christoph Hellwig]

On Tue, Feb 23, 2021 at 07:37:52AM +0000, Chaitanya Kulkarni wrote:
> On 2/22/21 23:10, Christoph Hellwig wrote:
> > On Tue, Feb 23, 2021 at 03:51:23AM +0000, Chaitanya Kulkarni wrote:
> >> Looking at the other call sites do we need something like following ?
> >> Since __blk_queue_bounce() passes the NULL for the passthru case as a
> >> bio_set value ?
> > Well, that is a somewhat odd calling convention.  What about the patch below
> > instead?  That being we really need to kill this bouncing code off..
> I assume you are sending this patch, let me know otherwise.
> If you do please add, looks good.

I'll split the gfp_mask cleanup out, and will submit it with your as
the author if that is ok.  I'll need a signoff, though.

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Christoph Hellwig]

On Tue, Feb 23, 2021 at 04:08:52PM +0100, Christoph Hellwig wrote:
> On Tue, Feb 23, 2021 at 07:37:52AM +0000, Chaitanya Kulkarni wrote:
> > On 2/22/21 23:10, Christoph Hellwig wrote:
> > > On Tue, Feb 23, 2021 at 03:51:23AM +0000, Chaitanya Kulkarni wrote:
> > >> Looking at the other call sites do we need something like following ?
> > >> Since __blk_queue_bounce() passes the NULL for the passthru case as a
> > >> bio_set value ?
> > > Well, that is a somewhat odd calling convention.  What about the patch below
> > > instead?  That being we really need to kill this bouncing code off..
> > I assume you are sending this patch, let me know otherwise.
> > If you do please add, looks good.
> 
> I'll split the gfp_mask cleanup out, and will submit it with your as
> the author if that is ok.  I'll need a signoff, though.

Actually, I ended up reworking it once more as there is no point for
the parameter either.

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Chaitanya Kulkarni]

On 2/23/21 07:09, Christoph Hellwig wrote:
> I assume you are sending this patch, let me know otherwise.
> If you do please add, looks good.

I'll split the gfp_mask cleanup out, and will submit it with your as
the author if that is ok.  I'll need a signoff, though.

Okay, here it is :-

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

I'll review the patch(es) once you post again.

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Christoph Hellwig]

The problem is that the blk-crypto fallback code calls bio_split
with a NULL bioset.  That was aready broken before, as the mempool
needed to guarantee forward progress was missing, but is not fatal.

Satya, can you look into adding a mempool that can guarantees forward
progress here?

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[Christoph Hellwig]

On Tue, Feb 23, 2021 at 08:04:08AM +0100, Christoph Hellwig wrote:
> The problem is that the blk-crypto fallback code calls bio_split
> with a NULL bioset.  That was aready broken before, as the mempool
> needed to guarantee forward progress was missing, but is not fatal.
> 
> Satya, can you look into adding a mempool that can guarantees forward
> progress here?

Something like this would be the minimum viable fix:

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

Re: [REGRESSION] "split bio_kmalloc from bio_alloc_bioset" causing crash shortly after bootup

[John Stultz]

On Mon, Feb 22, 2021 at 11:22 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Feb 23, 2021 at 08:04:08AM +0100, Christoph Hellwig wrote:
> > The problem is that the blk-crypto fallback code calls bio_split
> > with a NULL bioset.  That was aready broken before, as the mempool
> > needed to guarantee forward progress was missing, but is not fatal.
> >
> > Satya, can you look into adding a mempool that can guarantees forward
> > progress here?
>
> Something like this would be the minimum viable fix:

This seems to work for me!

Tested-by: John Stultz <john.stultz@linaro.org>

thanks
-john