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
