From 1c81d1afa2a99e6b2bc7cc46c860c232a6bbb843 Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Thu, 19 Aug 2021 15:09:41 +0200 Subject: [PATCH] nvmf: clear ctrlr->listener in remove_listener When removing a listener, there's a small window when the listener is already freed, while the controllers that were created on that listener are still active. It happens, because the listener is removed before disconnecting its qpairs and in turn destroying the controllers. The ctrlr->listener pointer is now cleared when a listener is freed and its value is checked for NULL before each use. Signed-off-by: Konrad Sztyber Change-Id: Iace65a46eebc5972cc4ef0f1c1822ffc5d3e559e Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9237 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Reviewed-by: Shuhei Matsumoto Reviewed-by: Jim Harris Reviewed-by: Ziye Yang Tested-by: SPDK CI Jenkins --- lib/nvmf/ctrlr.c | 65 ++++++++++++++++++++++++-------------------- lib/nvmf/subsystem.c | 7 +++++ 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/lib/nvmf/ctrlr.c b/lib/nvmf/ctrlr.c index 238c63b9c..40c93b8f1 100644 --- a/lib/nvmf/ctrlr.c +++ b/lib/nvmf/ctrlr.c @@ -1948,6 +1948,37 @@ nvmf_ctrlr_mask_aen(struct spdk_nvmf_ctrlr *ctrlr, } } +/* we have to use the typedef in the function declaration to appease astyle. */ +typedef enum spdk_nvme_ana_state spdk_nvme_ana_state_t; + +static inline spdk_nvme_ana_state_t +nvmf_ctrlr_get_ana_state(struct spdk_nvmf_ctrlr *ctrlr, uint32_t anagrpid) +{ + if (spdk_unlikely(ctrlr->listener == NULL)) { + return SPDK_NVME_ANA_INACCESSIBLE_STATE; + } + + assert(anagrpid - 1 < ctrlr->subsys->max_nsid); + return ctrlr->listener->ana_state[anagrpid - 1]; +} + +static spdk_nvme_ana_state_t +nvmf_ctrlr_get_ana_state_from_nsid(struct spdk_nvmf_ctrlr *ctrlr, uint32_t nsid) +{ + struct spdk_nvmf_ns *ns; + + /* We do not have NVM subsystem specific ANA state. Hence if NSID is either + * SPDK_NVMF_GLOBAL_NS_TAG, invalid, or for inactive namespace, return + * the optimized state. + */ + ns = _nvmf_subsystem_get_ns(ctrlr->subsys, nsid); + if (ns == NULL) { + return SPDK_NVME_ANA_OPTIMIZED_STATE; + } + + return nvmf_ctrlr_get_ana_state(ctrlr, ns->anagrpid); +} + static void nvmf_get_ana_log_page(struct spdk_nvmf_ctrlr *ctrlr, struct iovec *iovs, int iovcnt, uint64_t offset, uint32_t length, uint32_t rae) @@ -2003,7 +2034,7 @@ nvmf_get_ana_log_page(struct spdk_nvmf_ctrlr *ctrlr, struct iovec *iovs, int iov ana_desc.ana_group_id = anagrpid; ana_desc.num_of_nsid = ctrlr->subsys->ana_group[anagrpid - 1]; - ana_desc.ana_state = ctrlr->listener->ana_state[anagrpid - 1]; + ana_desc.ana_state = nvmf_ctrlr_get_ana_state(ctrlr, anagrpid); copy_len = spdk_min(sizeof(ana_desc) - offset, length); copied_len = _copy_buf_to_iovs(©_ctx, (const char *)&ana_desc + offset, @@ -2335,7 +2366,7 @@ spdk_nvmf_ctrlr_identify_ns(struct spdk_nvmf_ctrlr *ctrlr, assert(ns->anagrpid - 1 < subsystem->max_nsid); nsdata->anagrpid = ns->anagrpid; - ana_state = ctrlr->listener->ana_state[ns->anagrpid - 1]; + ana_state = nvmf_ctrlr_get_ana_state(ctrlr, ns->anagrpid); if (ana_state == SPDK_NVME_ANA_INACCESSIBLE_STATE || ana_state == SPDK_NVME_ANA_PERSISTENT_LOSS_STATE) { nsdata->nuse = 0; @@ -2785,28 +2816,6 @@ _nvme_ana_state_to_path_status(enum spdk_nvme_ana_state ana_state) } } -/* we have to use the typedef in the function declaration to appease astyle. */ -typedef enum spdk_nvme_ana_state spdk_nvme_ana_state_t; - -static spdk_nvme_ana_state_t -nvmf_ctrlr_get_ana_state(struct spdk_nvmf_ctrlr *ctrlr, uint32_t nsid) -{ - struct spdk_nvmf_ns *ns; - - /* We do not have NVM subsystem specific ANA state. Hence if NSID is either - * SPDK_NVMF_GLOBAL_NS_TAG, invalid, or for inactive namespace, return - * the optimized state. - */ - ns = _nvmf_subsystem_get_ns(ctrlr->subsys, nsid); - if (ns == NULL) { - return SPDK_NVME_ANA_OPTIMIZED_STATE; - } - - assert(ns->anagrpid - 1 < ctrlr->subsys->max_nsid); - - return ctrlr->listener->ana_state[ns->anagrpid]; -} - static int nvmf_ctrlr_get_features(struct spdk_nvmf_request *req) { @@ -2836,7 +2845,7 @@ nvmf_ctrlr_get_features(struct spdk_nvmf_request *req) /* * Process Get Features command for non-discovery controller */ - ana_state = nvmf_ctrlr_get_ana_state(ctrlr, cmd->nsid); + ana_state = nvmf_ctrlr_get_ana_state_from_nsid(ctrlr, cmd->nsid); switch (ana_state) { case SPDK_NVME_ANA_INACCESSIBLE_STATE: case SPDK_NVME_ANA_PERSISTENT_LOSS_STATE: @@ -2932,7 +2941,7 @@ nvmf_ctrlr_set_features(struct spdk_nvmf_request *req) /* * Process Set Features command for non-discovery controller */ - ana_state = nvmf_ctrlr_get_ana_state(ctrlr, cmd->nsid); + ana_state = nvmf_ctrlr_get_ana_state_from_nsid(ctrlr, cmd->nsid); switch (ana_state) { case SPDK_NVME_ANA_INACCESSIBLE_STATE: case SPDK_NVME_ANA_CHANGE_STATE: @@ -3736,9 +3745,7 @@ nvmf_ctrlr_process_io_cmd(struct spdk_nvmf_request *req) return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE; } - assert(ns->anagrpid - 1 < ctrlr->subsys->max_nsid); - - ana_state = ctrlr->listener->ana_state[ns->anagrpid - 1]; + ana_state = nvmf_ctrlr_get_ana_state(ctrlr, ns->anagrpid); if (spdk_unlikely(ana_state != SPDK_NVME_ANA_OPTIMIZED_STATE && ana_state != SPDK_NVME_ANA_NON_OPTIMIZED_STATE)) { SPDK_DEBUGLOG(nvmf, "Fail I/O command due to ANA state %d\n", diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index 5856081b6..894171e88 100644 --- a/lib/nvmf/subsystem.c +++ b/lib/nvmf/subsystem.c @@ -342,6 +342,7 @@ _nvmf_subsystem_remove_listener(struct spdk_nvmf_subsystem *subsystem, bool stop) { struct spdk_nvmf_transport *transport; + struct spdk_nvmf_ctrlr *ctrlr; if (stop) { transport = spdk_nvmf_tgt_get_transport(subsystem->tgt, listener->trid->trstring); @@ -350,6 +351,12 @@ _nvmf_subsystem_remove_listener(struct spdk_nvmf_subsystem *subsystem, } } + TAILQ_FOREACH(ctrlr, &subsystem->ctrlrs, link) { + if (ctrlr->listener == listener) { + ctrlr->listener = NULL; + } + } + TAILQ_REMOVE(&subsystem->listeners, listener, link); free(listener->ana_state); free(listener);