LKML Archive on lore.kernel.org help / color / mirror / Atom feed
* [PATCH] blk-mq-sched: Fix blk_mq_sched_alloc_tags() error handling
@ 2021-07-27 9:32 John Garry
2021-07-27 10:06 ` Ming Lei
2021-07-27 22:44 ` Jens Axboe
0 siblings, 2 replies; 5+ messages in thread
From: John Garry @ 2021-07-27 9:32 UTC (permalink / raw)
To: axboe; +Cc: osandov, linux-block, linux-kernel, ming.lei, John Garry
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 <john.garry@huawei.com>
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] blk-mq-sched: Fix blk_mq_sched_alloc_tags() error handling 2021-07-27 9:32 [PATCH] blk-mq-sched: Fix blk_mq_sched_alloc_tags() error handling John Garry @ 2021-07-27 10:06 ` Ming Lei 2021-07-27 10:30 ` John Garry 2021-07-27 22:44 ` Jens Axboe 1 sibling, 1 reply; 5+ messages in thread From: Ming Lei @ 2021-07-27 10:06 UTC (permalink / raw) To: John Garry; +Cc: axboe, osandov, linux-block, linux-kernel 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 <john.garry@huawei.com> 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 <ming.lei@redhat.com> Thanks, Ming ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-mq-sched: Fix blk_mq_sched_alloc_tags() error handling 2021-07-27 10:06 ` Ming Lei @ 2021-07-27 10:30 ` John Garry 2021-07-27 10:43 ` Ming Lei 0 siblings, 1 reply; 5+ messages in thread From: John Garry @ 2021-07-27 10:30 UTC (permalink / raw) To: Ming Lei; +Cc: axboe, osandov, linux-block, linux-kernel 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 <john.garry@huawei.com> > > 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 <ming.lei@redhat.com> > > > cheers > . > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-mq-sched: Fix blk_mq_sched_alloc_tags() error handling 2021-07-27 10:30 ` John Garry @ 2021-07-27 10:43 ` Ming Lei 0 siblings, 0 replies; 5+ messages in thread From: Ming Lei @ 2021-07-27 10:43 UTC (permalink / raw) To: John Garry; +Cc: axboe, osandov, linux-block, linux-kernel 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 <john.garry@huawei.com> > > > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] blk-mq-sched: Fix blk_mq_sched_alloc_tags() error handling 2021-07-27 9:32 [PATCH] blk-mq-sched: Fix blk_mq_sched_alloc_tags() error handling John Garry 2021-07-27 10:06 ` Ming Lei @ 2021-07-27 22:44 ` Jens Axboe 1 sibling, 0 replies; 5+ messages in thread From: Jens Axboe @ 2021-07-27 22:44 UTC (permalink / raw) To: John Garry; +Cc: osandov, linux-block, linux-kernel, ming.lei 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-27 22:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-27 9:32 [PATCH] blk-mq-sched: Fix blk_mq_sched_alloc_tags() error handling John Garry 2021-07-27 10:06 ` Ming Lei 2021-07-27 10:30 ` John Garry 2021-07-27 10:43 ` Ming Lei 2021-07-27 22:44 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).