On Thu, Sep 16, 2021 at 10:18:10PM +0800, Yu Kuai wrote: > There is a problem that nbd_handle_reply() might access freed request: > > 1) At first, a normal io is submitted and completed with scheduler: > > internel_tag = blk_mq_get_tag -> get tag from sched_tags > blk_mq_rq_ctx_init > sched_tags->rq[internel_tag] = sched_tag->static_rq[internel_tag] > ... > blk_mq_get_driver_tag > __blk_mq_get_driver_tag -> get tag from tags > tags->rq[tag] = sched_tag->static_rq[internel_tag] > > So, both tags->rq[tag] and sched_tags->rq[internel_tag] are pointing > to the request: sched_tags->static_rq[internal_tag]. Even if the > io is finished. > > 2) nbd server send a reply with random tag directly: > > recv_work > nbd_handle_reply > blk_mq_tag_to_rq(tags, tag) > rq = tags->rq[tag] > > 3) if the sched_tags->static_rq is freed: > > blk_mq_sched_free_requests > blk_mq_free_rqs(q->tag_set, hctx->sched_tags, i) > -> step 2) access rq before clearing rq mapping > blk_mq_clear_rq_mapping(set, tags, hctx_idx); > __free_pages() -> rq is freed here > > 4) Then, nbd continue to use the freed request in nbd_handle_reply > > Fix the problem by get 'q_usage_counter' before blk_mq_tag_to_rq(), > thus request is ensured not to be freed because 'q_usage_counter' is > not zero. > > Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx> > --- > Changes in v9: > - move percpu_ref_put() behind. Looks fine: Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx> -- Ming
