From f4e3f59e0e19881dc4cb64b85e5fa3c5f2ec1a5d Mon Sep 17 00:00:00 2001 From: Changpeng Liu Date: Thu, 5 Dec 2019 22:24:20 -0500 Subject: [PATCH] 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: Changpeng Liu Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/476969 Community-CI: Broadcom SPDK FC-NVMe CI Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Jim Harris --- lib/nvme/nvme.c | 6 ++- test/unit/lib/nvme/nvme.c/nvme_ut.c | 74 +++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/lib/nvme/nvme.c b/lib/nvme/nvme.c index 0f4706b0b..50a3ae146 100644 --- a/lib/nvme/nvme.c +++ b/lib/nvme/nvme.c @@ -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; } diff --git a/test/unit/lib/nvme/nvme.c/nvme_ut.c b/test/unit/lib/nvme/nvme.c/nvme_ut.c index ef31a3580..00934afba 100644 --- a/test/unit/lib/nvme/nvme.c/nvme_ut.c +++ b/test/unit/lib/nvme/nvme.c/nvme_ut.c @@ -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",