* [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq_mapping()
@ 2022-10-11 12:10 Yu Kuai
2022-10-11 13:24 ` John Garry
0 siblings, 1 reply; 3+ messages in thread
From: Yu Kuai @ 2022-10-11 12:10 UTC (permalink / raw)
To: axboe, ming.lei, hare, john.garry
Cc: linux-block, linux-kernel, yukuai3, yukuai1, yi.zhang
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
// failed due to oom
alloc_pages_node
// set->stags[hctx_idx] is still NULL
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>
---
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) {
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq_mapping() 2022-10-11 12:10 [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq_mapping() Yu Kuai @ 2022-10-11 13:24 ` John Garry 2022-10-11 13:27 ` Yu Kuai 0 siblings, 1 reply; 3+ messages in thread From: John Garry @ 2022-10-11 13:24 UTC (permalink / raw) To: Yu Kuai, axboe, ming.lei, hare Cc: linux-block, linux-kernel, yukuai3, yi.zhang 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> > --- > 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) { ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq_mapping() 2022-10-11 13:24 ` John Garry @ 2022-10-11 13:27 ` Yu Kuai 0 siblings, 0 replies; 3+ messages in thread From: Yu Kuai @ 2022-10-11 13:27 UTC (permalink / raw) To: John Garry, Yu Kuai, axboe, ming.lei, hare Cc: linux-block, linux-kernel, yi.zhang, yukuai (C) 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) { > > . > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-11 13:27 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-11 12:10 [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq_mapping() Yu Kuai 2022-10-11 13:24 ` John Garry 2022-10-11 13:27 ` Yu Kuai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox