From: "yukuai (C)" <yukuai3@huawei.com>
To: Ming Lei <ming.lei@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>, <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 15:50:31 +0800 [thread overview]
Message-ID: <3809c299-4f40-3614-c233-184d8a39e286@huawei.com> (raw)
In-Reply-To: <YQy2x5RzwveW0ROR@T590>
On 2021/08/06 12:12, Ming Lei wrote:
> On Fri, Aug 06, 2021 at 11:40:25AM +0800, yukuai (C) wrote:
>> 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)
>
> It is driver's responsibility to cover race between normal completion
> and timeout/error handing, so driver has the knowledge if one tag is valid
> or not. In short, it is driver's responsibility to guarantee that valid 'tag'
> is passed to blk_mq_tag_to_rq().
>
Thanks for the explanation.
Best regards,
Kuai
prev parent reply other threads:[~2021-08-06 7:50 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)
2021-08-06 4:12 ` Ming Lei
2021-08-06 7:50 ` yukuai (C) [this message]
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=3809c299-4f40-3614-c233-184d8a39e286@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).