From b503ef4fa0220719441e31841452b82182599ca4 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Sun, 20 Jun 2021 23:18:44 +0900 Subject: [PATCH] nvmf: Fix heap-use-after-free when poll_group_remove() is called after ctrlr is freed When a qpair is destroyed and the qpair is the last, _nvmf_ctrlr_free_from_qpair() (in lib/nvmf/nvmf.c) sends two messages, one is for _nvmf_ctrlr_destruct() and another is for _nvmf_transport_qpair_fini(). We do not know which of two completes earlier. _nvmf_ctrlr_destruct() frees the qpair->ctrlr in the end. On the other hand, _nvmf_ctrlr_free_from_qpair() calls spdk_nvmf_poll_group_remove() in the end, and spdk_nvmf_poll_group_remove() accesses the qpair->ctrlr to free queued requests to the qpair. Before one recent change, spdk_nvmf_poll_group_remove() had been called before _nvmf_ctrlr_free_from_qpair() was called. Hence extrace the operation to free queued requests from spdk_nvmf_poll_group_remove() and inline it into _nvmf_qpair_destroy(). Fixes one showstopper error to investigate the issue reported in #1819. Signed-off-by: Shuhei Matsumoto Change-Id: I29c43ff7b289fc77a5de9c33e0266301c412e208 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8438 Tested-by: SPDK CI Jenkins Reviewed-by: Reviewed-by: Aleksey Marchuk Reviewed-by: Jim Harris Community-CI: Mellanox Build Bot --- lib/nvmf/nvmf.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/lib/nvmf/nvmf.c b/lib/nvmf/nvmf.c index b13a3cd38..702be795c 100644 --- a/lib/nvmf/nvmf.c +++ b/lib/nvmf/nvmf.c @@ -978,10 +978,7 @@ _nvmf_ctrlr_free_from_qpair(void *ctx) void spdk_nvmf_poll_group_remove(struct spdk_nvmf_qpair *qpair) { - struct spdk_nvmf_ctrlr *ctrlr = qpair->ctrlr; struct spdk_nvmf_transport_poll_group *tgroup; - struct spdk_nvmf_request *req, *tmp; - struct spdk_nvmf_subsystem_poll_group *sgroup; int rc; SPDK_DTRACE_PROBE2(nvmf_poll_group_remove_qpair, qpair, @@ -1000,18 +997,6 @@ spdk_nvmf_poll_group_remove(struct spdk_nvmf_qpair *qpair) } } - if (ctrlr) { - sgroup = &qpair->group->sgroups[ctrlr->subsys->id]; - TAILQ_FOREACH_SAFE(req, &sgroup->queued, link, tmp) { - if (req->qpair == qpair) { - TAILQ_REMOVE(&sgroup->queued, req, link); - if (nvmf_transport_req_free(req)) { - SPDK_ERRLOG("Transport request free error!\n"); - } - } - } - } - TAILQ_REMOVE(&qpair->group->qpairs, qpair, link); qpair->group = NULL; } @@ -1022,6 +1007,8 @@ _nvmf_qpair_destroy(void *ctx, int status) struct nvmf_qpair_disconnect_ctx *qpair_ctx = ctx; struct spdk_nvmf_qpair *qpair = qpair_ctx->qpair; struct spdk_nvmf_ctrlr *ctrlr = qpair->ctrlr; + struct spdk_nvmf_request *req, *tmp; + struct spdk_nvmf_subsystem_poll_group *sgroup; assert(qpair->state == SPDK_NVMF_QPAIR_DEACTIVATING); qpair_ctx->qid = qpair->qid; @@ -1034,6 +1021,16 @@ _nvmf_qpair_destroy(void *ctx, int status) assert(qpair->group->stat.current_io_qpairs > 0); qpair->group->stat.current_io_qpairs--; } + + sgroup = &qpair->group->sgroups[ctrlr->subsys->id]; + TAILQ_FOREACH_SAFE(req, &sgroup->queued, link, tmp) { + if (req->qpair == qpair) { + TAILQ_REMOVE(&sgroup->queued, req, link); + if (nvmf_transport_req_free(req)) { + SPDK_ERRLOG("Transport request free error!/n"); + } + } + } } if (!ctrlr || !ctrlr->thread) {