Commit b503ef4f authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Jim Harris
Browse files

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: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: I29c43ff7b289fc77a5de9c33e0266301c412e208
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8438


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatar <dongx.yi@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Community-CI: Mellanox Build Bot
parent e1bf63af

lib/nvmf/nvmf.c

+12 −15
Original line number Diff line number Diff line
@@ -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) {