Commit 1c81d1af authored by Konrad Sztyber's avatar Konrad Sztyber Committed by Tomasz Zawadzki
Browse files

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: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Change-Id: Iace65a46eebc5972cc4ef0f1c1822ffc5d3e559e
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9237


Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarZiye Yang <ziye.yang@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent fb4398ef

lib/nvmf/ctrlr.c

+36 −29
Original line number Diff line number Diff line
@@ -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(&copy_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",

lib/nvmf/subsystem.c

+7 −0
Original line number Diff line number Diff line
@@ -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);