Commit 97385af1 authored by Alexey Marchuk's avatar Alexey Marchuk Committed by Tomasz Zawadzki
Browse files

nvmf: Fix double controller destruction when subsys is deleted 



When a subsystem is being deleted, we disconnect all qpairs
and when the last qpair for some controller is disconnected,
we start controller desctruction process. This process requires
to send a message to subsystem's thread to remove the controller
from the list in the subsystem and after that send a message to
controller's thread to release resources.
The problem is that the subsystem also destroys all attached
controllers. This order is unpredictable and we may get
heap-use-after-free or double free.

To fix this problem we can rely on the fact that the subsystem
can only be destroyed in incative state, that means that all
qpairs linked to the subsystem are already disconnected and
all controllers are already destroyed or in the process of
destruction.

spdk_nvmf_subsystem_destroy API is now can be asyncrhonous,
it accepts a callback with cb argument.

Change-Id: Ic72d69200bc8302dae2f8cd8ca44bc640c6a8116
Signed-off-by: default avatarAlexey Marchuk <alexeymar@mellanox.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6660


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent 971f07b9

CHANGELOG.md

+2 −0
Original line number Diff line number Diff line
@@ -15,6 +15,8 @@ An new parameter `anagrpid` was added to the RPC `nvmf_subsystem_add_ns`. 


An new parameter `anagrpid` was added to the RPC `nvmf_subsystem_listener_set_ana_state`.



`spdk_nvmf_subsystem_destroy` is now can be asynchronous, it accepts a callback and callback argument.



### bdev



New API `spdk_bdev_get_memory_domains` has been added, it allows to get SPDK memory domains used by bdev.

include/spdk/nvmf.h

+17 −2
Original line number Diff line number Diff line
@@ -3,6 +3,7 @@ 
 *

 *   Copyright (c) Intel Corporation. All rights reserved.

 *   Copyright (c) 2018-2021 Mellanox Technologies LTD. All rights reserved.

 *   Copyright (c) 2021 NVIDIA CORPORATION & AFFILIATES. All rights reserved.

 *

 *   Redistribution and use in source and binary forms, with or without

 *   modification, are permitted provided that the following conditions
@@ -357,13 +358,27 @@ struct spdk_nvmf_subsystem *spdk_nvmf_subsystem_create(struct spdk_nvmf_tgt *tgt 
		enum spdk_nvmf_subtype type,

		uint32_t num_ns);



typedef void (*nvmf_subsystem_destroy_cb)(void *cb_arg);



/**

 * Destroy an NVMe-oF subsystem. A subsystem may only be destroyed when in

 * the Inactive state. See spdk_nvmf_subsystem_stop().

 * the Inactive state. See spdk_nvmf_subsystem_stop(). A subsystem may be

 * destroyed asynchronously, in that case \b cpl_cb will be called

 *

 * \param subsystem The NVMe-oF subsystem to destroy.

 * \param cpl_cb Optional callback to be called if the subsystem is destroyed asynchronously, only called if

 * return value is -EINPROGRESS

 * \param cpl_cb_arg Optional user context to be passed to \b cpl_cb

 *

 * \retval 0 if sybsystem is destroyed, \b cpl_cb is not called is that case

 * \retval -EINVAl if \b subsystem is a NULL pointer

 * \retval -EAGAIN if \b subsystem is not in INACTIVE state

 * \retval -EALREADY if subsystem destruction is already started

 * \retval -EINPROGRESS if subsystem is destroyed asyncronously, cpl_cb will be called in that case

 */

void spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem);

int

spdk_nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem, nvmf_subsystem_destroy_cb cpl_cb,

			    void *cpl_cb_arg);



/**

 * Function to be called once the subsystem has changed state.

lib/nvmf/ctrlr.c

+14 −1
Original line number Diff line number Diff line
@@ -3,6 +3,7 @@ 
 *

 *   Copyright (c) Intel Corporation. All rights reserved.

 *   Copyright (c) 2019, 2020 Mellanox Technologies LTD. All rights reserved.

 *   Copyright (c) 2021 NVIDIA CORPORATION & AFFILIATES. All rights reserved.

 *

 *   Redistribution and use in source and binary forms, with or without

 *   modification, are permitted provided that the following conditions
@@ -176,6 +177,11 @@ nvmf_ctrlr_keep_alive_poll(void *ctx) 
	uint64_t now = spdk_get_ticks();

	struct spdk_nvmf_ctrlr *ctrlr = ctx;



	if (ctrlr->in_destruct) {

		nvmf_ctrlr_stop_keep_alive_timer(ctrlr);

		return SPDK_POLLER_IDLE;

	}



	SPDK_DEBUGLOG(nvmf, "Polling ctrlr keep alive timeout\n");



	/* If the Keep alive feature is in use and the timer expires */
@@ -465,6 +471,9 @@ _nvmf_ctrlr_destruct(void *ctx) 
	struct spdk_nvmf_reservation_log *log, *log_tmp;

	struct spdk_nvmf_async_event_completion *event, *event_tmp;



	assert(spdk_get_thread() == ctrlr->thread);

	assert(ctrlr->in_destruct);



	if (ctrlr->disconnect_in_progress) {

		SPDK_ERRLOG("freeing ctrlr with disconnect in progress\n");

		spdk_thread_send_msg(ctrlr->thread, _nvmf_ctrlr_destruct, ctrlr);
@@ -904,6 +913,11 @@ nvmf_ctrlr_association_remove(void *ctx) 
	struct spdk_nvmf_ctrlr *ctrlr = ctx;

	int rc;



	nvmf_ctrlr_stop_association_timer(ctrlr);



	if (ctrlr->in_destruct) {

		return SPDK_POLLER_IDLE;

	}

	SPDK_DEBUGLOG(nvmf, "Disconnecting host from subsystem %s due to association timeout.\n",

		      ctrlr->subsys->subnqn);
@@ -913,7 +927,6 @@ nvmf_ctrlr_association_remove(void *ctx) 
		assert(false);

	}



	nvmf_ctrlr_stop_association_timer(ctrlr);

	return SPDK_POLLER_BUSY;

}

lib/nvmf/nvmf.c

+15 −1
Original line number Diff line number Diff line
@@ -3,6 +3,7 @@ 
 *

 *   Copyright (c) Intel Corporation. All rights reserved.

 *   Copyright (c) 2018-2019, 2021 Mellanox Technologies LTD. All rights reserved.

 *   Copyright (c) 2021 NVIDIA CORPORATION & AFFILIATES. All rights reserved.

 *

 *   Redistribution and use in source and binary forms, with or without

 *   modification, are permitted provided that the following conditions
@@ -357,12 +358,24 @@ nvmf_tgt_destroy_cb(void *io_device) 
{

	struct spdk_nvmf_tgt *tgt = io_device;

	uint32_t i;

	int rc;



	if (tgt->subsystems) {

		for (i = 0; i < tgt->max_subsystems; i++) {

			if (tgt->subsystems[i]) {

				nvmf_subsystem_remove_all_listeners(tgt->subsystems[i], true);

				spdk_nvmf_subsystem_destroy(tgt->subsystems[i]);



				rc = spdk_nvmf_subsystem_destroy(tgt->subsystems[i], nvmf_tgt_destroy_cb, tgt);

				if (rc) {

					if (rc == -EINPROGRESS) {

						/* If rc is -EINPROGRESS, nvmf_tgt_destroy_cb will be called again when subsystem #i

						 * is destroyed, nvmf_tgt_destroy_cb will continue to destroy other subsystems if any */

						return;

					} else {

						SPDK_ERRLOG("Failed to destroy subsystem, id %u, rc %d\n", tgt->subsystems[i]->id, rc);

						assert(0);

					}

				}

			}

		}

		free(tgt->subsystems);
@@ -940,6 +953,7 @@ _nvmf_ctrlr_free_from_qpair(void *ctx) 
	spdk_bit_array_clear(ctrlr->qpair_mask, qpair_ctx->qid);

	count = spdk_bit_array_count_set(ctrlr->qpair_mask);

	if (count == 0) {

		assert(!ctrlr->in_destruct);

		ctrlr->in_destruct = true;

		spdk_thread_send_msg(ctrlr->subsys->thread, _nvmf_ctrlr_destruct, ctrlr);

	}

lib/nvmf/nvmf_internal.h

+7 −0
Original line number Diff line number Diff line
@@ -3,6 +3,7 @@ 
 *

 *   Copyright (c) Intel Corporation. All rights reserved.

 *   Copyright (c) 2019 Mellanox Technologies LTD. All rights reserved.

 *   Copyright (c) 2021 NVIDIA CORPORATION & AFFILIATES. All rights reserved.

 *

 *   Redistribution and use in source and binary forms, with or without

 *   modification, are permitted provided that the following conditions
@@ -292,6 +293,9 @@ struct spdk_nvmf_subsystem { 
	/* boolean for state change synchronization */

	bool						changing_state;



	bool						destroying;

	bool						async_destroy;



	struct spdk_nvmf_tgt				*tgt;



	/* Array of pointers to namespaces of size max_nsid indexed by nsid - 1 */
@@ -314,6 +318,9 @@ struct spdk_nvmf_subsystem { 


	TAILQ_ENTRY(spdk_nvmf_subsystem)		entries;



	nvmf_subsystem_destroy_cb			async_destroy_cb;

	void						*async_destroy_cb_arg;



	char						sn[SPDK_NVME_CTRLR_SN_LEN + 1];

	char						mn[SPDK_NVME_CTRLR_MN_LEN + 1];

	char						subnqn[SPDK_NVMF_NQN_MAX_LEN + 1];