blk-mq: fix kernel panic during iterating over flush request
PATCH 1/1 blk-mq: fix kernel panic during iterating over flush request
For fixing use-after-free during iterating over requests, we grabbed request's refcount before calling ->fn in commit 2e315dc07df0 ("blk-mq:
grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter"). Turns out this way may cause kernel panic when iterating over one flush request:
1) old flush request's tag is just released, and this tag is reused by one new request, but ->rqs[] isn't updated yet
2) the flush request can be re-used for submitting one new flush command, so blk_rq_init() is called at the same time
3) meantime blk_mq_queue_tag_busy_iter() is called, and old flush request is retrieved from ->rqs[tag]; when blk_mq_put_rq_ref() is called, flush_rq->end_io may not be updated yet, so NULL pointer dereference is triggered in blk_mq_put_rq_ref().
Fix the issue by calling refcount_set(&flush_rq->ref, 1) after flush_rq->end_io is set. So far the only other caller of blk_rq_init() is scsi_ioctl_reset() in which the request doesn't enter block IO stack and the request reference count isn't used, so the change is safe.
blk_mq_tagset_busy_iter")
--- block/blk-core.c | 1 - block/blk-flush.c | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
block/blk-core.c +0 -1
@@ -121,7 +121,6 @@ void blk_rq_init(struct request_queue *q, struct request *rq) rq->internal_tag =BLK_MQ_NO_TAG ; rq->start_time_ns =ktime_get_ns (); rq->part =NULL ; -refcount_set (&rq->ref,1 );blk_crypto_rq_set_defaults (rq); }EXPORT_SYMBOL (blk_rq_init);
block/blk-flush.c +8 -0
@@ -329,6 +329,14 @@ static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq, flush_rq->rq_flags |=RQF_FLUSH_SEQ ; flush_rq->rq_disk = first_rq->rq_disk; flush_rq->end_io = flush_end_io; +/* +* Order WRITE ->end_io and WRITE rq->ref, and its pair is the one +* implied in refcount_inc_not_zero() called from +* blk_mq_find_and_get_req(), which orders WRITE/READ flush_rq->ref +* and READ flush_rq->end_io +*/ +smp_wmb (); +refcount_set (&flush_rq->ref,1 );blk_flush_queue_rq (flush_rq, false); }
Signature · 1 line
2.31.1
Looks good,
Quoted 22 lines · Ming Lei
For fixing use-after-free during iterating over requests, we grabbed request's refcount before calling ->fn in commit
2e315dc07df0("blk-mq:
grabrq->refcountbefore calling ->fn inblk_mq_tagset_busy_iter"). Turns out this way may cause kernel panic when iterating over one flush request:1) old flush request's tag is just released, and this tag is reused by one new request, but ->rqs[] isn't updated yet
2) the flush request can be re-used for submitting one new flush command, so
blk_rq_init()is called at the same time3) meantime
blk_mq_queue_tag_busy_iter()is called, and old flush request is retrieved from ->rqs[tag]; whenblk_mq_put_rq_ref()is called,flush_rq->end_iomay not be updated yet, so NULL pointer dereference is triggered inblk_mq_put_rq_ref().Fix the issue by calling
refcount_set(&flush_rq->ref, 1)afterflush_rq->end_iois set. So far the only other caller ofblk_rq_init()isscsi_ioctl_reset()in which the request doesn't enter block IO stack and the request reference count isn't used, so the change is safe.
Quoted 5 lines
Fixes 2e315dc07df0 ("blk-mq: grab rq->refcount before calling ->fn in
blk_mq_tagset_busy_iter")Reported-by "Blank-Burian, Markus, Dr." <[email protected]>Signed-off-by Ming Lei <[email protected]>
it looks ok:
Quoted 33 lines
--- block/blk-core.c | 1 - block/blk-flush.c | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-)block/blk-core.c +0
-1@@ -121,7 +121,6 @@ void blk_rq_init(struct request_queue *q, struct request *rq) rq->internal_tag =BLK_MQ_NO_TAG ; rq->start_time_ns =ktime_get_ns (); rq->part =NULL ; -refcount_set (&rq->ref,1 );blk_crypto_rq_set_defaults (rq); }EXPORT_SYMBOL (blk_rq_init);block/blk-flush.c +8
-0@@ -329,6 +329,14 @@ static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq, flush_rq->rq_flags |=RQF_FLUSH_SEQ ; flush_rq->rq_disk = first_rq->rq_disk; flush_rq->end_io = flush_end_io; +/* +* Order WRITE ->end_io and WRITE rq->ref, and its pair is the one +* implied in refcount_inc_not_zero() called from +* blk_mq_find_and_get_req(), which orders WRITE/READ flush_rq->ref +* and READ flush_rq->end_io +*/ +smp_wmb (); +refcount_set (&flush_rq->ref,1 );
hi,
Quoted 49 lines
For fixing use-after-free during iterating over requests, we grabbed request's refcount before calling ->fn in commit
2e315dc07df0("blk-mq:
grabrq->refcountbefore calling ->fn inblk_mq_tagset_busy_iter"). Turns out this way may cause kernel panic when iterating over one flush request:1) old flush request's tag is just released, and this tag is reused by one new request, but ->rqs[] isn't updated yet
2) the flush request can be re-used for submitting one new flush command, so
blk_rq_init()is called at the same time3) meantime
blk_mq_queue_tag_busy_iter()is called, and old flush request is retrieved from ->rqs[tag]; whenblk_mq_put_rq_ref()is called,flush_rq->end_iomay not be updated yet, so NULL pointer dereference is triggered inblk_mq_put_rq_ref().Fix the issue by calling
refcount_set(&flush_rq->ref, 1)afterflush_rq->end_iois set. So far the only other caller ofblk_rq_init()isscsi_ioctl_reset()in which the request doesn't enter block IO stack and the request reference count isn't used, so the change is safe.Fixes 2e315dc07df0 ("blk-mq: grab rq->refcount before calling ->fn in
blk_mq_tagset_busy_iter")Reported-by "Blank-Burian, Markus, Dr." <[email protected]>Signed-off-by Ming Lei <[email protected]>--- block/blk-core.c | 1 - block/blk-flush.c | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-)block/blk-core.c +0
-1@@ -121,7 +121,6 @@ void blk_rq_init(struct request_queue *q, struct request *rq) rq->internal_tag =BLK_MQ_NO_TAG ; rq->start_time_ns =ktime_get_ns (); rq->part =NULL ; -refcount_set (&rq->ref,1 );blk_crypto_rq_set_defaults (rq); }EXPORT_SYMBOL (blk_rq_init);block/blk-flush.c +0
-0
block/blk-flush.c static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq, · line 336
Quoted 8 lines
block/blk-flush.c +5
-0@@ -329,6 +329,14 @@ static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq, flush_rq->rq_flags |=RQF_FLUSH_SEQ ; flush_rq->rq_disk = first_rq->rq_disk; flush_rq->end_io = flush_end_io; +/* +* Order WRITE ->end_io and WRITE rq->ref, and its pair is the one +* implied in refcount_inc_not_zero() called from +* blk_mq_find_and_get_req(), which orders WRITE/READ flush_rq->ref +* and READ flush_rq->end_io
Recently we run into similar panic which is a NULL dereference on rq->mq_hctx in is_flush_rq(), we also
guess there is race bug just what you have fixed.
But I have one question here, for a blk-mq device, before issuing the first flush req, flush_rq->end_io
is NULL, and for following flush reqs on this blk-mq device, flush_rq->end_io won't be NULL. I searched
the codes and don't find any place that sets flush_rq->end_io to be NULL once flush_rq has been completed.
Regards,
Xiaoguang Wang
Quoted 6 lines
block/blk-flush.c +3
-0+*/ +smp_wmb (); +refcount_set (&flush_rq->ref,1 );blk_flush_queue_rq (flush_rq, false); }
Quoted 66 lines · Xiaoguang Wang
hi,
Quoted 49 lines
For fixing use-after-free during iterating over requests, we grabbed request's refcount before calling ->fn in commit
2e315dc07df0("blk-mq:
grabrq->refcountbefore calling ->fn inblk_mq_tagset_busy_iter"). Turns out this way may cause kernel panic when iterating over one flush request:1) old flush request's tag is just released, and this tag is reused by one new request, but ->rqs[] isn't updated yet
2) the flush request can be re-used for submitting one new flush command, so
blk_rq_init()is called at the same time3) meantime
blk_mq_queue_tag_busy_iter()is called, and old flush request is retrieved from ->rqs[tag]; whenblk_mq_put_rq_ref()is called,flush_rq->end_iomay not be updated yet, so NULL pointer dereference is triggered inblk_mq_put_rq_ref().Fix the issue by calling
refcount_set(&flush_rq->ref, 1)afterflush_rq->end_iois set. So far the only other caller ofblk_rq_init()isscsi_ioctl_reset()in which the request doesn't enter block IO stack and the request reference count isn't used, so the change is safe.Fixes 2e315dc07df0 ("blk-mq: grab rq->refcount before calling ->fn in
blk_mq_tagset_busy_iter")Reported-by "Blank-Burian, Markus, Dr." <[email protected]>Signed-off-by Ming Lei <[email protected]>--- block/blk-core.c | 1 - block/blk-flush.c | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-)block/blk-core.c +0
-1@@ -121,7 +121,6 @@ void blk_rq_init(struct request_queue *q, struct request *rq) rq->internal_tag =BLK_MQ_NO_TAG ; rq->start_time_ns =ktime_get_ns (); rq->part =NULL ; -refcount_set (&rq->ref,1 );blk_crypto_rq_set_defaults (rq); }EXPORT_SYMBOL (blk_rq_init);block/blk-flush.c +0
-0block/blk-flush.c static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq, · line 336
Quoted 8 lines
block/blk-flush.c +5
-0@@ -329,6 +329,14 @@ static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq, flush_rq->rq_flags |=RQF_FLUSH_SEQ ; flush_rq->rq_disk = first_rq->rq_disk; flush_rq->end_io = flush_end_io; +/* +* Order WRITE ->end_io and WRITE rq->ref, and its pair is the one +* implied in refcount_inc_not_zero() called from +* blk_mq_find_and_get_req(), which orders WRITE/READ flush_rq->ref +* and READ flush_rq->end_io Recently we run into similar panic which is a NULL dereference on
rq->mq_hctxinis_flush_rq(), we alsoguess there is race bug just what you have fixed.
Quoted 8 lines
But I have one question here, for a blk-mq device, before issuing the first flush req,
flush_rq->end_iois NULL, and for following flush reqs on this blk-mq device,
flush_rq->end_iowon't be NULL. I searchedthe codes and don't find any place that sets
flush_rq->end_ioto be NULL onceflush_rqhas been completed.
blk_kick_flush():
blk_rq_init memset(rq, 0, sizeof(*rq));
Thanks,
Ming
hi,
Quoted 69 lines · Xiaoguang Wang
Quoted 68 lines
hi,
Quoted 49 lines
For fixing use-after-free during iterating over requests, we grabbed request's refcount before calling ->fn in commit
2e315dc07df0("blk-mq:
grabrq->refcountbefore calling ->fn inblk_mq_tagset_busy_iter"). Turns out this way may cause kernel panic when iterating over one flush request:1) old flush request's tag is just released, and this tag is reused by one new request, but ->rqs[] isn't updated yet
2) the flush request can be re-used for submitting one new flush command, so
blk_rq_init()is called at the same time3) meantime
blk_mq_queue_tag_busy_iter()is called, and old flush request is retrieved from ->rqs[tag]; whenblk_mq_put_rq_ref()is called,flush_rq->end_iomay not be updated yet, so NULL pointer dereference is triggered inblk_mq_put_rq_ref().Fix the issue by calling
refcount_set(&flush_rq->ref, 1)afterflush_rq->end_iois set. So far the only other caller ofblk_rq_init()isscsi_ioctl_reset()in which the request doesn't enter block IO stack and the request reference count isn't used, so the change is safe.Fixes 2e315dc07df0 ("blk-mq: grab rq->refcount before calling ->fn in
blk_mq_tagset_busy_iter")Reported-by "Blank-Burian, Markus, Dr." <[email protected]>Signed-off-by Ming Lei <[email protected]>--- block/blk-core.c | 1 - block/blk-flush.c | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-)block/blk-core.c +0
-1@@ -121,7 +121,6 @@ void blk_rq_init(struct request_queue *q, struct request *rq) rq->internal_tag =BLK_MQ_NO_TAG ; rq->start_time_ns =ktime_get_ns (); rq->part =NULL ; -refcount_set (&rq->ref,1 );blk_crypto_rq_set_defaults (rq); }EXPORT_SYMBOL (blk_rq_init);block/blk-flush.c +0
-0block/blk-flush.c static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq, · line 336
Quoted 8 lines
block/blk-flush.c +5
-0@@ -329,6 +329,14 @@ static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq, flush_rq->rq_flags |=RQF_FLUSH_SEQ ; flush_rq->rq_disk = first_rq->rq_disk; flush_rq->end_io = flush_end_io; +/* +* Order WRITE ->end_io and WRITE rq->ref, and its pair is the one +* implied in refcount_inc_not_zero() called from +* blk_mq_find_and_get_req(), which orders WRITE/READ flush_rq->ref +* and READ flush_rq->end_io Recently we run into similar panic which is a NULL dereference on
rq->mq_hctxinis_flush_rq(), we alsoguess there is race bug just what you have fixed.
But I have one question here, for a blk-mq device, before issuing the first flush req,
flush_rq->end_io
Quoted 8 lines
Quoted 5 lines
is NULL, and for following flush reqs on this blk-mq device,
flush_rq->end_iowon't be NULL. I searchedthe codes and don't find any place that sets
flush_rq->end_ioto be NULL onceflush_rqhas been completed.
blk_kick_flush():blk_rq_init memset(rq, 0, sizeof(*rq));
Oh, thanks.
I searched codes for quite a while...
Regards,
Xiaoguang Wang
Quoted 4 lines
Thanks,
Ming
Quoted 22 lines · Ming Lei
For fixing use-after-free during iterating over requests, we grabbed request's refcount before calling ->fn in commit
2e315dc07df0("blk-mq:
grabrq->refcountbefore calling ->fn inblk_mq_tagset_busy_iter"). Turns out this way may cause kernel panic when iterating over one flush request:1) old flush request's tag is just released, and this tag is reused by one new request, but ->rqs[] isn't updated yet
2) the flush request can be re-used for submitting one new flush command, so
blk_rq_init()is called at the same time3) meantime
blk_mq_queue_tag_busy_iter()is called, and old flush request is retrieved from ->rqs[tag]; whenblk_mq_put_rq_ref()is called,flush_rq->end_iomay not be updated yet, so NULL pointer dereference is triggered inblk_mq_put_rq_ref().Fix the issue by calling
refcount_set(&flush_rq->ref, 1)afterflush_rq->end_iois set. So far the only other caller ofblk_rq_init()isscsi_ioctl_reset()in which the request doesn't enter block IO stack and the request reference count isn't used, so the change is safe.
Quoted 5 lines
Fixes 2e315dc07df0 ("blk-mq: grab rq->refcount before calling ->fn in
blk_mq_tagset_busy_iter")Reported-by "Blank-Burian, Markus, Dr." <[email protected]>Signed-off-by Ming Lei <[email protected]>
Hello Jens,
Ping...
Thanks,
Ming
Quoted 17 lines · Ming Lei
For fixing use-after-free during iterating over requests, we grabbed request's refcount before calling ->fn in commit
2e315dc07df0("blk-mq:
grabrq->refcountbefore calling ->fn inblk_mq_tagset_busy_iter"). Turns out this way may cause kernel panic when iterating over one flush request:1) old flush request's tag is just released, and this tag is reused by one new request, but ->rqs[] isn't updated yet
2) the flush request can be re-used for submitting one new flush command, so
blk_rq_init()is called at the same time3) meantime
blk_mq_queue_tag_busy_iter()is called, and old flush request is retrieved from ->rqs[tag]; whenblk_mq_put_rq_ref()is called,flush_rq->end_iomay not be updated yet, so NULL pointer dereference is triggered inblk_mq_put_rq_ref().
Quoted 4 lines
Fix the issue by calling
refcount_set(&flush_rq->ref, 1)afterflush_rq->end_iois set. So far the only other caller ofblk_rq_init()isscsi_ioctl_reset()in which the request doesn't enter block IO stack and the request reference count isn't used, so the change is safe.
Applied, thanks.
Signature · 1 line
Jens Axboe