If the blk_mq_sched_alloc_tags() -> blk_mq_alloc_rqs() call fails, then we
call blk_mq_sched_free_tags() -> blk_mq_free_rqs().
It is incorrect to do so, as any rqs would have already been freed in the
blk_mq_alloc_rqs() call.
Fix by calling blk_mq_free_rq_map() only directly.
Fixes: 6917ff0b5bd41 ("blk-mq-sched: refactor scheduler initialization")
Signed-off-by: John Garry <[email protected]>
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index c838d81ac058..0f006cabfd91 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -515,17 +515,6 @@ void blk_mq_sched_insert_requests(struct blk_mq_hw_ctx *hctx,
percpu_ref_put(&q->q_usage_counter);
}
-static void blk_mq_sched_free_tags(struct blk_mq_tag_set *set,
- struct blk_mq_hw_ctx *hctx,
- unsigned int hctx_idx)
-{
- if (hctx->sched_tags) {
- blk_mq_free_rqs(set, hctx->sched_tags, hctx_idx);
- blk_mq_free_rq_map(hctx->sched_tags, set->flags);
- hctx->sched_tags = NULL;
- }
-}
-
static int blk_mq_sched_alloc_tags(struct request_queue *q,
struct blk_mq_hw_ctx *hctx,
unsigned int hctx_idx)
@@ -539,8 +528,10 @@ static int blk_mq_sched_alloc_tags(struct request_queue *q,
return -ENOMEM;
ret = blk_mq_alloc_rqs(set, hctx->sched_tags, hctx_idx, q->nr_requests);
- if (ret)
- blk_mq_sched_free_tags(set, hctx, hctx_idx);
+ if (ret) {
+ blk_mq_free_rq_map(hctx->sched_tags, set->flags);
+ hctx->sched_tags = NULL;
+ }
return ret;
}
--
2.26.2
On Tue, Jul 27, 2021 at 05:32:53PM +0800, John Garry wrote:
> If the blk_mq_sched_alloc_tags() -> blk_mq_alloc_rqs() call fails, then we
> call blk_mq_sched_free_tags() -> blk_mq_free_rqs().
>
> It is incorrect to do so, as any rqs would have already been freed in the
> blk_mq_alloc_rqs() call.
>
> Fix by calling blk_mq_free_rq_map() only directly.
>
> Fixes: 6917ff0b5bd41 ("blk-mq-sched: refactor scheduler initialization")
> Signed-off-by: John Garry <[email protected]>
Not sure it is one fix, because blk_mq_free_rqs() does nothing when
->static_rqs[] isn't filled, so 'Fixes' tag isn't needed, IMO.
>
> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> index c838d81ac058..0f006cabfd91 100644
> --- a/block/blk-mq-sched.c
> +++ b/block/blk-mq-sched.c
> @@ -515,17 +515,6 @@ void blk_mq_sched_insert_requests(struct blk_mq_hw_ctx *hctx,
> percpu_ref_put(&q->q_usage_counter);
> }
>
> -static void blk_mq_sched_free_tags(struct blk_mq_tag_set *set,
> - struct blk_mq_hw_ctx *hctx,
> - unsigned int hctx_idx)
> -{
> - if (hctx->sched_tags) {
> - blk_mq_free_rqs(set, hctx->sched_tags, hctx_idx);
> - blk_mq_free_rq_map(hctx->sched_tags, set->flags);
> - hctx->sched_tags = NULL;
> - }
> -}
> -
> static int blk_mq_sched_alloc_tags(struct request_queue *q,
> struct blk_mq_hw_ctx *hctx,
> unsigned int hctx_idx)
> @@ -539,8 +528,10 @@ static int blk_mq_sched_alloc_tags(struct request_queue *q,
> return -ENOMEM;
>
> ret = blk_mq_alloc_rqs(set, hctx->sched_tags, hctx_idx, q->nr_requests);
> - if (ret)
> - blk_mq_sched_free_tags(set, hctx, hctx_idx);
> + if (ret) {
> + blk_mq_free_rq_map(hctx->sched_tags, set->flags);
> + hctx->sched_tags = NULL;
> + }
The patch itself looks fine:
Reviewed-by: Ming Lei <[email protected]>
Thanks,
Ming
On 27/07/2021 11:06, Ming Lei wrote:
> On Tue, Jul 27, 2021 at 05:32:53PM +0800, John Garry wrote:
>> If the blk_mq_sched_alloc_tags() -> blk_mq_alloc_rqs() call fails, then we
>> call blk_mq_sched_free_tags() -> blk_mq_free_rqs().
>>
>> It is incorrect to do so, as any rqs would have already been freed in the
>> blk_mq_alloc_rqs() call.
>>
>> Fix by calling blk_mq_free_rq_map() only directly.
>>
>> Fixes: 6917ff0b5bd41 ("blk-mq-sched: refactor scheduler initialization")
>> Signed-off-by: John Garry <[email protected]>
>
> Not sure it is one fix, because blk_mq_free_rqs() does nothing when
> ->static_rqs[] isn't filled, so 'Fixes' tag isn't needed, IMO.
I actually experimented by returning an error from
blk_mq_sched_alloc_tags() -> blk_mq_alloc_rqs() at the function entry
point, and it crashes:
[8.118419]
==================================================================
[8.125677] BUG: KASAN: null-ptr-deref in blk_mq_free_rqs+0x170/0x380
[8.132158] Read of size 8 at addr 0000000000000020 by task swapper/0/1
[8.138806]
[8.140297] CPU: 9 PID: 1 Comm: swapper/0 Not tainted
5.14.0-rc3-gf17b9ea98148 #113
[8.147997] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05
IT21 Nemo 2.0 RC0 04/18/2018
[8.157177] Call trace:
[8.159628] dump_backtrace+0x0/0x2bc
[8.163312] show_stack+0x1c/0x2c
[8.166643] dump_stack_lvl+0x68/0x84
[8.170325] kasan_report+0x12c/0x24c
[8.174005] __asan_load8+0x98/0xd4
[8.177508] blk_mq_free_rqs+0x170/0x380
[8.181450] blk_mq_init_sched+0x604/0x6e0
[8.185567] elevator_init_mq+0x134/0x220
[8.189596] __device_add_disk+0x28c/0x520
[8.193714] device_add_disk+0x18/0x24
[8.197481] loop_add+0x300/0x360
[8.200811] loop_init+0xf4/0x120
[8.204142] do_one_initcall+0xa8/0x270
[8.207995] kernel_init_freeable+0x2c4/0x34c
[8.212376] kernel_init+0x28/0x140
[8.215880] ret_from_fork+0x10/0x18
[8.219472]
==================================================================
[8.226728] Disabling lock debugging due to kernel taint
[8.232074] Unable to handle kernel NULL pointer dereference at virtual
address 0000000000000020
[8.240919] Mem abort info:
[8.243727] ESR = 0x96000004
[8.246797] EC = 0x25: DABT (current EL), IL = 32 bits
[8.252141] SET = 0, FnV = 0
[8.255210] EA = 0, S1PTW = 0
[8.258367] FSC = 0x04: level 0 translation fault
[8.263272] Data abort info:
[8.266165] ISV = 0, ISS = 0x00000004
[8.270021] CM = 0, WnR = 0
[8.272998] [0000000000000020] user address but active_mm is swapper
[8.279387] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[8.284987] Modules linked in:
[8.288055] CPU: 9 PID: 1 Comm: swapper/0 Tainted: GB
5.14.0-rc3-gf17b9ea98148 #113
[8.297149] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05
IT21 Nemo 2.0 RC0 04/18/2018
[8.306329] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
[8.312366] pc : blk_mq_free_rqs+0x170/0x380
[8.316657] lr : blk_mq_free_rqs+0x170/0x380
[8.320947] sp : ffff8000164f7ae0
[8.324273] x29: ffff8000164f7ae0 x28: ffff00105340e960 x27: ffff001053418650
[8.331455] x26: ffff00105340c1e4 x25: ffff00105340c1a0 x24: ffff8000116e0860
[8.338636] x23: ffff00104a679400 x22: ffff00104a6b4c00 x21: fffffffffffff000
[8.345817] x20: 0000000000000000 x19: fffffffffffffff8 x18: 0000000000000000
[8.352996] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[8.360175] x14: 0000000000000000 x13: 746e696174206c65 x12: ffff6002db693ff9
[8.367355] x11: 1fffe002db693ff8 x10: ffff6002db693ff8 x9 : dfff800000000000
[8.374536] x8 : ffff0016db49ffc0 x7 : 0000000000000001 x6 : 00009ffd2496c008
[8.381716] x5 : ffff0016db49ffc0 x4 : dfff800000000000 x3 : ffff8000115b29d4
[8.388896] x2 : 0000000000000007 x1 : ffff0410019e8000 x0 : 0000000000000001
[8.396076] Call trace:
[8.398528] blk_mq_free_rqs+0x170/0x380
[8.402470] blk_mq_init_sched+0x604/0x6e0
[8.406587] elevator_init_mq+0x134/0x220
[8.410614] __device_add_disk+0x28c/0x520
[8.414732] device_add_disk+0x18/0x24
[8.418500] loop_add+0x300/0x360
[8.421828] loop_init+0xf4/0x120
[8.425157] do_one_initcall+0xa8/0x270
[8.429009] kernel_init_freeable+0x2c4/0x34c
[8.433388] kernel_init+0x28/0x140
[8.436891] ret_from_fork+0x10/0x18
[8.440487] Code: 9100a260 d346feb5 8b153035 97f287e9 (f9401676)
[8.446710] ---[ end trace 66db82d449431dd1 ]--
That's even if I include "blk-mq: Change rqs check in blk_mq_free_rqs()"
in my other series.
The crash seems to be in blk_mq_clear_rq_mapping(), which much newer
than 6917ff0b5bd41. But I'd still leave the Fixes tag.
>
>>
>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>> index c838d81ac058..0f006cabfd91 100644
>> --- a/block/blk-mq-sched.c
>> +++ b/block/blk-mq-sched.c
>> @@ -515,17 +515,6 @@ void blk_mq_sched_insert_requests(struct blk_mq_hw_ctx *hctx,
>> percpu_ref_put(&q->q_usage_counter);
>> }
>>
>> -static void blk_mq_sched_free_tags(struct blk_mq_tag_set *set,
>> - struct blk_mq_hw_ctx *hctx,
>> - unsigned int hctx_idx)
>> -{
>> - if (hctx->sched_tags) {
>> - blk_mq_free_rqs(set, hctx->sched_tags, hctx_idx);
>> - blk_mq_free_rq_map(hctx->sched_tags, set->flags);
>> - hctx->sched_tags = NULL;
>> - }
>> -}
>> -
>> static int blk_mq_sched_alloc_tags(struct request_queue *q,
>> struct blk_mq_hw_ctx *hctx,
>> unsigned int hctx_idx)
>> @@ -539,8 +528,10 @@ static int blk_mq_sched_alloc_tags(struct request_queue *q,
>> return -ENOMEM;
>>
>> ret = blk_mq_alloc_rqs(set, hctx->sched_tags, hctx_idx, q->nr_requests);
>> - if (ret)
>> - blk_mq_sched_free_tags(set, hctx, hctx_idx);
>> + if (ret) {
>> + blk_mq_free_rq_map(hctx->sched_tags, set->flags);
>> + hctx->sched_tags = NULL;
>> + }
>
> The patch itself looks fine:
>
> Reviewed-by: Ming Lei <[email protected]>
>
>
>
cheers
> .
>
On Tue, Jul 27, 2021 at 11:30:00AM +0100, John Garry wrote:
> On 27/07/2021 11:06, Ming Lei wrote:
> > On Tue, Jul 27, 2021 at 05:32:53PM +0800, John Garry wrote:
> > > If the blk_mq_sched_alloc_tags() -> blk_mq_alloc_rqs() call fails, then we
> > > call blk_mq_sched_free_tags() -> blk_mq_free_rqs().
> > >
> > > It is incorrect to do so, as any rqs would have already been freed in the
> > > blk_mq_alloc_rqs() call.
> > >
> > > Fix by calling blk_mq_free_rq_map() only directly.
> > >
> > > Fixes: 6917ff0b5bd41 ("blk-mq-sched: refactor scheduler initialization")
> > > Signed-off-by: John Garry <[email protected]>
> >
> > Not sure it is one fix, because blk_mq_free_rqs() does nothing when
> > ->static_rqs[] isn't filled, so 'Fixes' tag isn't needed, IMO.
>
> I actually experimented by returning an error from blk_mq_sched_alloc_tags()
> -> blk_mq_alloc_rqs() at the function entry point, and it crashes:
>
> [8.118419]
> ==================================================================
> [8.125677] BUG: KASAN: null-ptr-deref in blk_mq_free_rqs+0x170/0x380
OK, looks it is caused by un-initialized &tags->page_list, then it is fine
to mark it as fixes.
--
Ming
On 7/27/21 3:32 AM, John Garry wrote:
> If the blk_mq_sched_alloc_tags() -> blk_mq_alloc_rqs() call fails, then we
> call blk_mq_sched_free_tags() -> blk_mq_free_rqs().
>
> It is incorrect to do so, as any rqs would have already been freed in the
> blk_mq_alloc_rqs() call.
>
> Fix by calling blk_mq_free_rq_map() only directly.
Applied, thanks.
--
Jens Axboe