Commit f4e3f59e authored by Changpeng Liu's avatar Changpeng Liu Committed by Tomasz Zawadzki
Browse files

nvme: fix potential memory leak when there is controller scan failure 



The nvme_transport_ctrlr_scan() may return failure while there are
multiple controllers, so the probe context's init_ctrlrs list may
not null for this case, so when free the probe context, let's ensure
there is no controller in the init_ctrlrs list.  Also added a UT to
cover this case.

Fix issue #1095.

Change-Id: I4d9a10ad73cf00bbe159edd1f5b919797333feb6
Signed-off-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/476969


Community-CI: Broadcom SPDK FC-NVMe CI <spdk-ci.pdl@broadcom.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent b9af5dd8

lib/nvme/nvme.c

+5 −1
Original line number Diff line number Diff line
@@ -547,7 +547,7 @@ spdk_nvme_probe_internal(struct spdk_nvme_probe_ctx *probe_ctx, 
			 bool direct_connect)

{

	int rc;

	struct spdk_nvme_ctrlr *ctrlr;

	struct spdk_nvme_ctrlr *ctrlr, *ctrlr_tmp;



	if (!spdk_nvme_transport_available(probe_ctx->trid.trtype)) {

		SPDK_ERRLOG("NVMe trtype %u not available\n", probe_ctx->trid.trtype);
@@ -559,6 +559,10 @@ spdk_nvme_probe_internal(struct spdk_nvme_probe_ctx *probe_ctx, 
	rc = nvme_transport_ctrlr_scan(probe_ctx, direct_connect);

	if (rc != 0) {

		SPDK_ERRLOG("NVMe ctrlr scan failed\n");

		TAILQ_FOREACH_SAFE(ctrlr, &probe_ctx->init_ctrlrs, tailq, ctrlr_tmp) {

			TAILQ_REMOVE(&probe_ctx->init_ctrlrs, ctrlr, tailq);

			nvme_transport_ctrlr_destruct(ctrlr);

		}

		nvme_robust_mutex_unlock(&g_spdk_nvme_driver->lock);

		return -1;

	}

test/unit/lib/nvme/nvme.c/nvme_ut.c

+74 −0
Original line number Diff line number Diff line
@@ -83,6 +83,45 @@ memset_trid(struct spdk_nvme_transport_id *trid1, struct spdk_nvme_transport_id 
}



static bool ut_check_trtype = false;

static bool ut_test_probe_internal = false;



static int

ut_nvme_pcie_ctrlr_scan(struct spdk_nvme_probe_ctx *probe_ctx,

			bool direct_connect)

{

	struct spdk_nvme_ctrlr *ctrlr;

	struct spdk_nvme_qpair qpair = {};

	int rc;



	if (probe_ctx->trid.trtype != SPDK_NVME_TRANSPORT_PCIE) {

		return -1;

	}



	ctrlr = calloc(1, sizeof(*ctrlr));

	CU_ASSERT(ctrlr != NULL);

	ctrlr->adminq = &qpair;



	/* happy path with first controller */

	MOCK_SET(nvme_transport_ctrlr_construct, ctrlr);

	rc = nvme_ctrlr_probe(&probe_ctx->trid, probe_ctx, NULL);

	CU_ASSERT(rc == 0);



	/* failed with the second controller */

	MOCK_SET(nvme_transport_ctrlr_construct, NULL);

	rc = nvme_ctrlr_probe(&probe_ctx->trid, probe_ctx, NULL);

	CU_ASSERT(rc != 0);

	MOCK_CLEAR_P(nvme_transport_ctrlr_construct);



	return -1;

}



int

nvme_transport_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr)

{

	free(ctrlr);

	return 0;

}



int

nvme_transport_ctrlr_scan(struct spdk_nvme_probe_ctx *probe_ctx,

			  bool direct_connect)
@@ -93,6 +132,10 @@ nvme_transport_ctrlr_scan(struct spdk_nvme_probe_ctx *probe_ctx, 
		CU_ASSERT(probe_ctx->trid.trtype == SPDK_NVME_TRANSPORT_PCIE);

	}



	if (ut_test_probe_internal) {

		return ut_nvme_pcie_ctrlr_scan(probe_ctx, direct_connect);

	}



	if (direct_connect == true && probe_ctx->probe_cb) {

		nvme_robust_mutex_unlock(&g_spdk_nvme_driver->lock);

		ctrlr = spdk_nvme_get_ctrlr_by_trid(&probe_ctx->trid);
@@ -1198,6 +1241,35 @@ test_nvme_wait_for_completion(void) 
	CU_ASSERT(rc == 0);

}



static void

test_nvme_ctrlr_probe_internal(void)

{

	struct spdk_nvme_probe_ctx *probe_ctx;

	struct spdk_nvme_transport_id trid = {};

	struct nvme_driver dummy;

	int rc;



	probe_ctx = calloc(1, sizeof(*probe_ctx));

	CU_ASSERT(probe_ctx != NULL);



	MOCK_SET(spdk_process_is_primary, true);

	MOCK_SET(spdk_memzone_reserve, (void *)&dummy);

	g_spdk_nvme_driver = NULL;

	rc = nvme_driver_init();

	CU_ASSERT(rc == 0);



	ut_test_probe_internal = true;

	MOCK_SET(dummy_probe_cb, true);

	trid.trtype = SPDK_NVME_TRANSPORT_PCIE;

	spdk_nvme_probe_ctx_init(probe_ctx, &trid, NULL, dummy_probe_cb, NULL, NULL);

	rc = spdk_nvme_probe_internal(probe_ctx, false);

	CU_ASSERT(rc < 0);

	CU_ASSERT(TAILQ_EMPTY(&probe_ctx->init_ctrlrs));



	free(probe_ctx);

	ut_test_probe_internal = false;

}



int main(int argc, char **argv)

{

	CU_pSuite	suite = NULL;
@@ -1232,6 +1304,8 @@ int main(int argc, char **argv) 
			    test_spdk_nvme_probe) == NULL ||

		CU_add_test(suite, "test_spdk_nvme_connect",

			    test_spdk_nvme_connect) == NULL ||

		CU_add_test(suite, "test_nvme_pci_probe_internal",

			    test_nvme_ctrlr_probe_internal) == NULL ||

		CU_add_test(suite, "test_nvme_init_controllers",

			    test_nvme_init_controllers) == NULL ||

		CU_add_test(suite, "test_nvme_driver_init",