Commit 20b35d76 authored by Seth Howell's avatar Seth Howell Committed by Changpeng Liu
Browse files

nvmf: don't keep a global discovery log page. 



Keeping a global discovery log page was meant to be a time saving
mechanism, but in the current implementation, it doesn't work properly,
and can cause undesirable behavior and potential crashes. There are two
main problems with keeping a global log page.

1. Admin qpairs can be assigned to any SPDK thread. This means that when
multiple initiators connect to the host and request the discovery log,
they can both be running through the spdk_nvmf_ctrlr_get_log_page
function at the same time. In the event that the discovery generation
counter is incremented while these accesses are occurring, it can cause
one or both of the threads to update the log at the same time. This
results in both logs trying to free the old log page (double free) and
set their log as the new one (possible memory leak).

2. The second problem is that each host is supposed to get a unique
discovery log based on the subsystems to which they have access.
Currently the code relies on whether the discovery log page offset in
the request is equal to 0 to determine if it should load a new discovery
log page or use the cached one. This is inherently faulty because it
relies on initiator provided value to determine what information to
provide from the log page. An initiator could easily send a discovery
request with an offset greater than 0 on purpose to procure most of a
log page provided to another host.

Overall, I think it's safest to not cache the log page at all anymore
and rely on a thread local fresh log page each time.

Reported-by: default avatarCurt Bruns <curt.e.bruns@intel.com>

Change-Id: Ib048e26f139927d888fed7019e0deec346359582
Signed-off-by: default avatarSeth Howell <seth.howell@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466839


Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent c071190e

lib/nvmf/ctrlr_discovery.c

+14 −15
Original line number Diff line number Diff line
@@ -48,8 +48,8 @@ 
#include "spdk/bdev_module.h"

#include "spdk_internal/log.h"



static void

nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn)

static struct spdk_nvmf_discovery_log_page *

nvmf_generate_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn, size_t *log_page_size)

{

	uint64_t numrec = 0;

	struct spdk_nvmf_subsystem *subsystem;
@@ -66,7 +66,7 @@ nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn) 
	disc_log = calloc(1, cur_size);

	if (disc_log == NULL) {

		SPDK_ERRLOG("Discovery log page memory allocation error\n");

		return;

		return NULL;

	}



	for (sid = 0; sid < tgt->max_subsystems; sid++) {
@@ -114,11 +114,9 @@ nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn) 


	disc_log->numrec = numrec;

	disc_log->genctr = tgt->discovery_genctr;

	*log_page_size = cur_size;



	free(tgt->discovery_log_page);



	tgt->discovery_log_page = disc_log;

	tgt->discovery_log_page_size = cur_size;

	return disc_log;

}



void
@@ -128,24 +126,23 @@ spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, const char *hostnqn, 
	size_t copy_len = 0;

	size_t zero_len = 0;

	struct iovec *tmp;

	size_t log_page_size = 0;

	struct spdk_nvmf_discovery_log_page *discovery_log_page;



	if (offset == 0 || tgt->discovery_log_page == NULL ||

	    tgt->discovery_log_page->genctr != tgt->discovery_genctr) {

		nvmf_update_discovery_log(tgt, hostnqn);

	}

	discovery_log_page = nvmf_generate_discovery_log(tgt, hostnqn, &log_page_size);



	/* Copy the valid part of the discovery log page, if any */

	if (tgt->discovery_log_page) {

	if (discovery_log_page) {

		for (tmp = iov; tmp < iov + iovcnt; tmp++) {

			copy_len = spdk_min(tmp->iov_len, length);

			copy_len = spdk_min(tgt->discovery_log_page_size - offset, copy_len);

			copy_len = spdk_min(log_page_size - offset, copy_len);



			memcpy(tmp->iov_base, (char *)tgt->discovery_log_page + offset, copy_len);

			memcpy(tmp->iov_base, (char *)discovery_log_page + offset, copy_len);



			offset += copy_len;

			length -= copy_len;

			zero_len = tmp->iov_len - copy_len;

			if (tgt->discovery_log_page_size <= offset || length == 0) {

			if (log_page_size <= offset || length == 0) {

				break;

			}

		}
@@ -157,5 +154,7 @@ spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, const char *hostnqn, 
		for (++tmp; tmp < iov + iovcnt; tmp++) {

			memset((char *)tmp->iov_base, 0, tmp->iov_len);

		}



		free(discovery_log_page);

	}

}

lib/nvmf/nvmf.c

+0 −6
Original line number Diff line number Diff line
@@ -249,8 +249,6 @@ spdk_nvmf_tgt_create(struct spdk_nvmf_target_opts *opts) 
	}



	tgt->discovery_genctr = 0;

	tgt->discovery_log_page = NULL;

	tgt->discovery_log_page_size = 0;

	TAILQ_INIT(&tgt->transports);



	tgt->subsystems = calloc(tgt->max_subsystems, sizeof(struct spdk_nvmf_subsystem *));
@@ -279,10 +277,6 @@ spdk_nvmf_tgt_destroy_cb(void *io_device) 
	void					*destroy_cb_arg;

	uint32_t i;



	if (tgt->discovery_log_page) {

		free(tgt->discovery_log_page);

	}



	if (tgt->subsystems) {

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

			if (tgt->subsystems[i]) {

lib/nvmf/nvmf_internal.h

+0 −2
Original line number Diff line number Diff line
@@ -86,8 +86,6 @@ struct spdk_nvmf_tgt { 
	/* Array of subsystem pointers of size max_subsystems indexed by sid */

	struct spdk_nvmf_subsystem		**subsystems;



	struct spdk_nvmf_discovery_log_page	*discovery_log_page;

	size_t					discovery_log_page_size;

	TAILQ_HEAD(, spdk_nvmf_transport)	transports;



	spdk_nvmf_tgt_destroy_done_fn		*destroy_cb_fn;

test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c

+0 −1
Original line number Diff line number Diff line
@@ -274,7 +274,6 @@ test_discovery_log(void) 
	subsystem->state = SPDK_NVMF_SUBSYSTEM_INACTIVE;

	spdk_nvmf_subsystem_destroy(subsystem);

	free(tgt.subsystems);

	free(tgt.discovery_log_page);

}



int main(int argc, char **argv)