From: Ming Lei <ming.lei@redhat.com> To: linux-nvme@lists.infradead.org, linux-scsi@vger.kernel.org, Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org, "Martin K . Petersen" <martin.petersen@oracle.com>, Christoph Hellwig <hch@lst.de> Cc: Bart Van Assche <bvanassche@acm.org>, Khazhy Kumykov <khazhy@google.com>, Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>, Hannes Reinecke <hare@suse.de>, John Garry <john.garry@huawei.com>, David Jeffery <djeffery@redhat.com>, Ming Lei <ming.lei@redhat.com> Subject: [PATCH 7/8] blk-mq: grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter Date: Sun, 25 Apr 2021 16:57:52 +0800 [thread overview] Message-ID: <20210425085753.2617424-8-ming.lei@redhat.com> (raw) In-Reply-To: <20210425085753.2617424-1-ming.lei@redhat.com> Grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter(), and this way will prevent the request from being re-used when ->fn is running. The approach is same as what we do during handling timeout. Fix request UAF related with completion race or queue releasing: - If one rq is referred before rq->q is frozen, then queue won't be frozen before the request is released during iteration. - If one rq is referred after rq->q is frozen, refcount_inc_not_zero() will return false, and we won't iterate over this request. However, still one request UAF not covered: refcount_inc_not_zero() may read one freed request, and it will be handled in next patch. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk-mq-tag.c | 14 +++++++++++--- block/blk-mq.c | 14 +++++++++----- block/blk-mq.h | 1 + 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index 2a37731e8244..489d2db89856 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -264,6 +264,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data) struct blk_mq_tags *tags = iter_data->tags; bool reserved = iter_data->flags & BT_TAG_ITER_RESERVED; struct request *rq; + bool ret; if (!reserved) bitnr += tags->nr_reserved_tags; @@ -276,12 +277,15 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data) rq = tags->static_rqs[bitnr]; else rq = tags->rqs[bitnr]; - if (!rq) + if (!rq || !refcount_inc_not_zero(&rq->ref)) return true; if ((iter_data->flags & BT_TAG_ITER_STARTED) && !blk_mq_request_started(rq)) - return true; - return iter_data->fn(rq, iter_data->data, reserved); + ret = true; + else + ret = iter_data->fn(rq, iter_data->data, reserved); + blk_mq_put_rq_ref(rq); + return ret; } /** @@ -348,6 +352,10 @@ void blk_mq_all_tag_iter(struct blk_mq_tags *tags, busy_tag_iter_fn *fn, * indicates whether or not @rq is a reserved request. Return * true to continue iterating tags, false to stop. * @priv: Will be passed as second argument to @fn. + * + * We grab one request reference before calling @fn and release it after + * @fn returns. So far we don't support to pass the request reference to + * one new conetxt in @fn. */ void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset, busy_tag_iter_fn *fn, void *priv) diff --git a/block/blk-mq.c b/block/blk-mq.c index e3d1067b10c3..9a4d520740a1 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -925,6 +925,14 @@ static bool blk_mq_req_expired(struct request *rq, unsigned long *next) return false; } +void blk_mq_put_rq_ref(struct request *rq) +{ + if (is_flush_rq(rq, rq->mq_hctx)) + rq->end_io(rq, 0); + else if (refcount_dec_and_test(&rq->ref)) + __blk_mq_free_request(rq); +} + static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, struct request *rq, void *priv, bool reserved) { @@ -958,11 +966,7 @@ static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, if (blk_mq_req_expired(rq, next)) blk_mq_rq_timed_out(rq, reserved); - if (is_flush_rq(rq, hctx)) - rq->end_io(rq, 0); - else if (refcount_dec_and_test(&rq->ref)) - __blk_mq_free_request(rq); - + blk_mq_put_rq_ref(rq); return true; } diff --git a/block/blk-mq.h b/block/blk-mq.h index 3616453ca28c..143afe42c63a 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -47,6 +47,7 @@ void blk_mq_add_to_requeue_list(struct request *rq, bool at_head, void blk_mq_flush_busy_ctxs(struct blk_mq_hw_ctx *hctx, struct list_head *list); struct request *blk_mq_dequeue_from_ctx(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *start); +void blk_mq_put_rq_ref(struct request *rq); /* * Internal helpers for allocating/freeing the request map -- 2.29.2
WARNING: multiple messages have this Message-ID (diff)
From: Ming Lei <ming.lei@redhat.com> To: linux-nvme@lists.infradead.org, linux-scsi@vger.kernel.org, Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org, "Martin K . Petersen" <martin.petersen@oracle.com>, Christoph Hellwig <hch@lst.de> Cc: Bart Van Assche <bvanassche@acm.org>, Khazhy Kumykov <khazhy@google.com>, Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>, Hannes Reinecke <hare@suse.de>, John Garry <john.garry@huawei.com>, David Jeffery <djeffery@redhat.com>, Ming Lei <ming.lei@redhat.com> Subject: [PATCH 7/8] blk-mq: grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter Date: Sun, 25 Apr 2021 16:57:52 +0800 [thread overview] Message-ID: <20210425085753.2617424-8-ming.lei@redhat.com> (raw) In-Reply-To: <20210425085753.2617424-1-ming.lei@redhat.com> Grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter(), and this way will prevent the request from being re-used when ->fn is running. The approach is same as what we do during handling timeout. Fix request UAF related with completion race or queue releasing: - If one rq is referred before rq->q is frozen, then queue won't be frozen before the request is released during iteration. - If one rq is referred after rq->q is frozen, refcount_inc_not_zero() will return false, and we won't iterate over this request. However, still one request UAF not covered: refcount_inc_not_zero() may read one freed request, and it will be handled in next patch. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk-mq-tag.c | 14 +++++++++++--- block/blk-mq.c | 14 +++++++++----- block/blk-mq.h | 1 + 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index 2a37731e8244..489d2db89856 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -264,6 +264,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data) struct blk_mq_tags *tags = iter_data->tags; bool reserved = iter_data->flags & BT_TAG_ITER_RESERVED; struct request *rq; + bool ret; if (!reserved) bitnr += tags->nr_reserved_tags; @@ -276,12 +277,15 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data) rq = tags->static_rqs[bitnr]; else rq = tags->rqs[bitnr]; - if (!rq) + if (!rq || !refcount_inc_not_zero(&rq->ref)) return true; if ((iter_data->flags & BT_TAG_ITER_STARTED) && !blk_mq_request_started(rq)) - return true; - return iter_data->fn(rq, iter_data->data, reserved); + ret = true; + else + ret = iter_data->fn(rq, iter_data->data, reserved); + blk_mq_put_rq_ref(rq); + return ret; } /** @@ -348,6 +352,10 @@ void blk_mq_all_tag_iter(struct blk_mq_tags *tags, busy_tag_iter_fn *fn, * indicates whether or not @rq is a reserved request. Return * true to continue iterating tags, false to stop. * @priv: Will be passed as second argument to @fn. + * + * We grab one request reference before calling @fn and release it after + * @fn returns. So far we don't support to pass the request reference to + * one new conetxt in @fn. */ void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset, busy_tag_iter_fn *fn, void *priv) diff --git a/block/blk-mq.c b/block/blk-mq.c index e3d1067b10c3..9a4d520740a1 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -925,6 +925,14 @@ static bool blk_mq_req_expired(struct request *rq, unsigned long *next) return false; } +void blk_mq_put_rq_ref(struct request *rq) +{ + if (is_flush_rq(rq, rq->mq_hctx)) + rq->end_io(rq, 0); + else if (refcount_dec_and_test(&rq->ref)) + __blk_mq_free_request(rq); +} + static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, struct request *rq, void *priv, bool reserved) { @@ -958,11 +966,7 @@ static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, if (blk_mq_req_expired(rq, next)) blk_mq_rq_timed_out(rq, reserved); - if (is_flush_rq(rq, hctx)) - rq->end_io(rq, 0); - else if (refcount_dec_and_test(&rq->ref)) - __blk_mq_free_request(rq); - + blk_mq_put_rq_ref(rq); return true; } diff --git a/block/blk-mq.h b/block/blk-mq.h index 3616453ca28c..143afe42c63a 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -47,6 +47,7 @@ void blk_mq_add_to_requeue_list(struct request *rq, bool at_head, void blk_mq_flush_busy_ctxs(struct blk_mq_hw_ctx *hctx, struct list_head *list); struct request *blk_mq_dequeue_from_ctx(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *start); +void blk_mq_put_rq_ref(struct request *rq); /* * Internal helpers for allocating/freeing the request map -- 2.29.2 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2021-04-25 8:58 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-25 8:57 [PATCH 0/8] blk-mq: fix request UAF related with iterating over tagset requests Ming Lei 2021-04-25 8:57 ` Ming Lei 2021-04-25 8:57 ` [PATCH 1/8] Revert "blk-mq: Fix races between blk_mq_update_nr_hw_queues() and iterating over tags" Ming Lei 2021-04-25 8:57 ` Ming Lei 2021-04-25 8:57 ` [PATCH 2/8] Revert "blk-mq: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list" Ming Lei 2021-04-25 8:57 ` Ming Lei 2021-04-25 8:57 ` [PATCH 3/8] Revert "blk-mq: Fix races between iterating over requests and freeing requests" Ming Lei 2021-04-25 8:57 ` Ming Lei 2021-04-25 8:57 ` [PATCH 4/8] Revert "blk-mq: Introduce atomic variants of blk_mq_(all_tag|tagset_busy)_iter" Ming Lei 2021-04-25 8:57 ` Ming Lei 2021-04-25 8:57 ` [PATCH 5/8] blk-mq: blk_mq_complete_request_locally Ming Lei 2021-04-25 8:57 ` Ming Lei 2021-04-25 8:57 ` [PATCH 6/8] block: drivers: complete request locally from blk_mq_tagset_busy_iter Ming Lei 2021-04-25 8:57 ` Ming Lei 2021-04-26 3:02 ` Bart Van Assche 2021-04-26 3:02 ` Bart Van Assche 2021-04-26 6:24 ` Ming Lei 2021-04-26 6:24 ` Ming Lei 2021-04-27 8:54 ` Ming Lei 2021-04-27 8:54 ` Ming Lei 2021-04-25 8:57 ` Ming Lei [this message] 2021-04-25 8:57 ` [PATCH 7/8] blk-mq: grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter Ming Lei 2021-04-25 18:55 ` Bart Van Assche 2021-04-25 18:55 ` Bart Van Assche 2021-04-26 0:41 ` Ming Lei 2021-04-26 0:41 ` Ming Lei 2021-04-25 8:57 ` [PATCH 8/8] blk-mq: clear stale request in tags->rq[] before freeing one request pool Ming Lei 2021-04-25 8:57 ` Ming Lei 2021-04-25 20:42 ` Bart Van Assche 2021-04-25 20:42 ` Bart Van Assche 2021-04-26 0:49 ` Ming Lei 2021-04-26 0:49 ` Ming Lei 2021-04-26 1:50 ` Bart Van Assche 2021-04-26 1:50 ` Bart Van Assche 2021-04-26 2:07 ` Ming Lei 2021-04-26 2:07 ` Ming Lei 2021-04-25 9:27 ` [PATCH 0/8] blk-mq: fix request UAF related with iterating over tagset requests Ming Lei 2021-04-25 9:27 ` Ming Lei 2021-04-25 20:53 ` Bart Van Assche 2021-04-25 20:53 ` Bart Van Assche 2021-04-26 1:19 ` Ming Lei 2021-04-26 1:19 ` Ming Lei 2021-04-26 1:57 ` Bart Van Assche 2021-04-26 1:57 ` Bart Van Assche 2021-04-25 16:17 ` Jens Axboe 2021-04-25 16:17 ` Jens Axboe 2021-04-25 18:39 ` Bart Van Assche 2021-04-25 18:39 ` Bart Van Assche 2021-04-25 20:18 ` Jens Axboe 2021-04-25 20:18 ` Jens Axboe
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210425085753.2617424-8-ming.lei@redhat.com \ --to=ming.lei@redhat.com \ --cc=axboe@kernel.dk \ --cc=bvanassche@acm.org \ --cc=djeffery@redhat.com \ --cc=hare@suse.de \ --cc=hch@lst.de \ --cc=john.garry@huawei.com \ --cc=khazhy@google.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=linux-scsi@vger.kernel.org \ --cc=martin.petersen@oracle.com \ --cc=shinichiro.kawasaki@wdc.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.