Messages in this thread | | | Subject | Re: [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq_mapping() | From | Yu Kuai <> | Date | Tue, 11 Oct 2022 21:27:34 +0800 |
| |
Hi,
在 2022/10/11 21:24, John Garry 写道: > On 11/10/2022 13:10, Yu Kuai wrote: >> From: Yu Kuai <yukuai3@huawei.com> >> >> Our syzkaller report a null pointer dereference, root cause is >> following: >> >> __blk_mq_alloc_map_and_rqs >> set->tags[hctx_idx] = blk_mq_alloc_map_and_rqs >> blk_mq_alloc_map_and_rqs >> blk_mq_alloc_rqso > > blk_mq_alloc_rqs > >> // failed due to oom >> alloc_pages_node >> // set->stags[hctx_idx] is still NULL > > set->tags > >> blk_mq_free_rqs >> drv_tags = set->tags[hctx_idx]; >> // null pointer dereference is triggered >> blk_mq_clear_rq_mapping(drv_tags, ...) >> >> This is because commit 63064be150e4 ("blk-mq: >> Add blk_mq_alloc_map_and_rqs()") merged the two steps: >> >> 1) set->tags[hctx_idx] = blk_mq_alloc_rq_map() >> 2) blk_mq_alloc_rqs(..., set->tags[hctx_idx]) >> >> into one step: >> >> set->tags[hctx_idx] = blk_mq_alloc_map_and_rqs() >> >> Since tags is not initialized yet in this case, fix the problem by >> checking if tags is NULL pointer in blk_mq_clear_rq_mapping(). >> >> Fixes: 63064be150e4 ("blk-mq: Add blk_mq_alloc_map_and_rqs()") >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > We could have something like the following to ensure the drv tags are > assigned before we try to allocate the rqs (and fail), but prob not > worth the churn since it's not nice to pass &set->tags[hctx_idx]: > > --->8--- > > -struct blk_mq_tags *blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, > +bool blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, > unsigned int hctx_idx, > - unsigned int depth) > + unsigned int depth, > + struct blk_mq_tags **tags) > { > - struct blk_mq_tags *tags; > int ret; > > - tags = blk_mq_alloc_rq_map(set, hctx_idx, depth, > set->reserved_tags); > - if (!tags) > - return NULL; > + *tags = blk_mq_alloc_rq_map(set, hctx_idx, depth, > set->reserved_tags); > + if (!*tags) > + return false; > > - ret = blk_mq_alloc_rqs(set, tags, hctx_idx, depth); > + ret = blk_mq_alloc_rqs(set, *tags, hctx_idx, depth); > if (ret) { > blk_mq_free_rq_map(tags); > + *tags = NULL; > - return NULL; > + return false; > } > > - return tags; > + return true; > } > > static bool __blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, > @@ -3632,10 +3632,9 @@ static bool __blk_mq_alloc_map_and_rqs(struct > blk_mq_tag_set *set, > return true; > } > > - set->tags[hctx_idx] = blk_mq_alloc_map_and_rqs(set, hctx_idx, > - set->queue_depth); > - > - return set->tags[hctx_idx]; > + return blk_mq_alloc_map_and_rqs(set, hctx_idx, > + set->queue_depth, > + &set->tags[hctx_idx]); > } > > ---8<---- > > > So, > > Reviewed-by: John Garry <john.garry@huawei.com>
Thanks for the review, I'll send a new verion with the spelling mistakes fixed.
Kuai > >> --- >> block/blk-mq.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 8070b6c10e8d..33292c01875d 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -3112,8 +3112,11 @@ static void blk_mq_clear_rq_mapping(struct >> blk_mq_tags *drv_tags, >> struct page *page; >> unsigned long flags; >> - /* There is no need to clear a driver tags own mapping */ >> - if (drv_tags == tags) >> + /* >> + * There is no need to clear mapping if driver tags is not >> initialized >> + * or the mapping belongs to the driver tags. >> + */ >> + if (!drv_tags || drv_tags == tags) >> return; >> list_for_each_entry(page, &tags->page_list, lru) { > > . >
| |