From: "yukuai (C)" <yukuai3@huawei.com>
To: Ming Lei <ming.lei@redhat.com>, Jens Axboe <axboe@kernel.dk>
Cc: <linux-block@vger.kernel.org>,
Bart Van Assche <bvanassche@acm.org>,
Hannes Reinecke <hare@suse.de>,
John Garry <john.garry@huawei.com>,
"Christoph Hellwig" <hch@lst.de>,
David Jeffery <djeffery@redhat.com>,
"Shinichiro Kawasaki" <shinichiro.kawasaki@wdc.com>
Subject: Re: [PATCH V7 0/4] blk-mq: fix request UAF related with iterating over tagset requests
Date: Fri, 6 Aug 2021 11:40:25 +0800 [thread overview]
Message-ID: <678fd2c5-587d-6abe-4348-067210d4adae@huawei.com> (raw)
In-Reply-To: <20210511152236.763464-1-ming.lei@redhat.com>
On 2021/05/11 23:22, Ming Lei wrote:
> Hi Jens,
>
> This patchset fixes the request UAF issue by one simple approach,
> without clearing ->rqs[] in fast path, please consider it for 5.13.
>
> 1) grab request's ref before calling ->fn in blk_mq_tagset_busy_iter,
> and release it after calling ->fn, so ->fn won't be called for one
> request if its queue is frozen, done in 2st patch
>
> 2) clearing any stale request referred in ->rqs[] before freeing the
> request pool, one per-tags spinlock is added for protecting
> grabbing request ref vs. clearing ->rqs[tag], so UAF by refcount_inc_not_zero
> in bt_tags_iter() is avoided, done in 3rd patch.
>
> V7:
> - fix one null-ptr-deref during updating nr_hw_queues, because
> blk_mq_clear_flush_rq_mapping() may touch non-mapped hw queue's
> tags, only patch 4/4 is modified, reported and verified by
> Shinichiro Kawasaki
> - run blktests and not see regression
>
> V6:
> - hold spin lock when reading rq via ->rq[tag_bit], the issue is
> added in V5
> - make blk_mq_find_and_get_req() more clean, as suggested by Bart
> - not hold tags->lock when clearing ->rqs[] for avoiding to disable
> interrupt a bit long, as suggested by Bart
> - code style change, as suggested by Christoph
>
> V5:
> - cover bt_iter() by same approach taken in bt_tags_iter(), and pass
> John's invasive test
> - add tested-by/reviewed-by tag
>
> V4:
> - remove hctx->fq-flush_rq from tags->rqs[] before freeing hw queue,
> patch 4/4 is added, which is based on David's patch.
>
> V3:
> - drop patches for completing requests started in iterator ->fn,
> because blk-mq guarantees that valid request is passed to ->fn,
> and it is driver's responsibility for avoiding double completion.
> And drivers works well for not completing rq twice.
> - add one patch for avoiding double accounting of flush rq
>
> V2:
> - take Bart's suggestion to not add blk-mq helper for completing
> requests when it is being iterated
> - don't grab rq->ref if the iterator is over static rqs because
> the use case do require to iterate over all requests no matter if
> the request is initialized or not
>
>
> Ming Lei (4):
> block: avoid double io accounting for flush request
> blk-mq: grab rq->refcount before calling ->fn in
> blk_mq_tagset_busy_iter
> blk-mq: clear stale request in tags->rq[] before freeing one request
> pool
> blk-mq: clearing flush request reference in tags->rqs[]
>
> block/blk-flush.c | 3 +-
> block/blk-mq-tag.c | 49 ++++++++++++++++++------
> block/blk-mq-tag.h | 6 +++
> block/blk-mq.c | 95 ++++++++++++++++++++++++++++++++++++++++------
> block/blk-mq.h | 1 +
> 5 files changed, 130 insertions(+), 24 deletions(-)
>
Hi, ming
I see this patchset was applied to fix the problem while iterating over
tagset, however blk_mq_tag_to_rq() is still untouched, and this
interface might still return freed request.
Any reason why this interface didn't use the same solution ?
(hold tags->lock and return null if ref is 0)
Thanks
Kuai
next prev parent reply other threads:[~2021-08-06 3:40 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-11 15:22 [PATCH V7 0/4] blk-mq: fix request UAF related with iterating over tagset requests Ming Lei
2021-05-11 15:22 ` [PATCH V7 1/4] block: avoid double io accounting for flush request Ming Lei
2021-05-11 15:22 ` [PATCH V7 2/4] blk-mq: grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter Ming Lei
2021-05-11 15:22 ` [PATCH V7 3/4] blk-mq: clear stale request in tags->rq[] before freeing one request pool Ming Lei
2021-05-11 15:22 ` [PATCH V7 4/4] blk-mq: clearing flush request reference in tags->rqs[] Ming Lei
2021-05-14 0:43 ` [PATCH V7 0/4] blk-mq: fix request UAF related with iterating over tagset requests Ming Lei
2021-05-14 15:34 ` Jens Axboe
2021-08-06 3:40 ` yukuai (C) [this message]
2021-08-06 4:12 ` Ming Lei
2021-08-06 7:50 ` yukuai (C)
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=678fd2c5-587d-6abe-4348-067210d4adae@huawei.com \
--to=yukuai3@huawei.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=linux-block@vger.kernel.org \
--cc=ming.lei@redhat.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).