Commit a52fc70d authored by Seth Howell's avatar Seth Howell Committed by Jim Harris
Browse files

nvmf: Discover commands use the nvmf_req->iov struct 



Discover commands previously blindly used the nvmf_req->data structure.
This only works if the entire command fits in a single contiguous
buffer. commit 1d9be84b changed the default buffer size such that
this would become a problem for as few as 8 subsystems.

Fixes github issue 525

This change may also help prevent data corruption as we were copying up
to nvmf_req->length data into the buffer. For requests with multiple
data buffers this can cause us to copy off the end of that buffer.

Change-Id: I788259da988b2458f57ee2795e1c5d3ced8803dd
Signed-off-by: default avatarSeth Howell <seth.howell@intel.com>
Reviewed-on: https://review.gerrithub.io/435544


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
parent 766832f3

lib/nvmf/ctrlr.c

+1 −1
Original line number Diff line number Diff line
@@ -1111,7 +1111,7 @@ spdk_nvmf_ctrlr_get_log_page(struct spdk_nvmf_request *req) 
	if (subsystem->subtype == SPDK_NVMF_SUBTYPE_DISCOVERY) {

		switch (lid) {

		case SPDK_NVME_LOG_DISCOVERY:

			spdk_nvmf_get_discovery_log_page(subsystem->tgt, req->data, offset, len);

			spdk_nvmf_get_discovery_log_page(subsystem->tgt, req->iov, req->iovcnt, offset, len);

			return SPDK_NVMF_REQUEST_EXEC_STATUS_COMPLETE;

		default:

			goto invalid_log_page;

lib/nvmf/ctrlr_discovery.c

+25 −14
Original line number Diff line number Diff line
@@ -116,11 +116,12 @@ nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt) 
}



void

spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, void *buffer,

				 uint64_t offset, uint32_t length)

spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, struct iovec *iov,

				 uint32_t iovcnt, uint64_t offset, uint32_t length)

{

	size_t copy_len = 0;

	size_t zero_len = length;

	size_t zero_len = 0;

	struct iovec *tmp;



	if (tgt->discovery_log_page == NULL ||

	    tgt->discovery_log_page->genctr != tgt->discovery_genctr) {
@@ -128,17 +129,27 @@ spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, void *buffer, 
	}



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

	if (tgt->discovery_log_page && offset < tgt->discovery_log_page_size) {

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

		zero_len -= copy_len;

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

	}

	if (tgt->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);



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



	/* Zero out the rest of the buffer */

			offset += copy_len;

			length -= copy_len;

			zero_len = tmp->iov_len - copy_len;

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

				break;

			}

		}

		/* Zero out the rest of the payload */

		if (zero_len) {

		memset((char *)buffer + copy_len, 0, zero_len);

			memset((char *)tmp->iov_base + copy_len, 0, zero_len);

		}



	/* We should have copied or zeroed every byte of the output buffer. */

	assert(copy_len + zero_len == length);

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

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

		}

	}

}

lib/nvmf/nvmf_internal.h

+2 −3
Original line number Diff line number Diff line
@@ -275,9 +275,8 @@ void spdk_nvmf_request_exec(struct spdk_nvmf_request *req); 
int spdk_nvmf_request_free(struct spdk_nvmf_request *req);

int spdk_nvmf_request_complete(struct spdk_nvmf_request *req);



void spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt,

				      void *buffer, uint64_t offset,

				      uint32_t length);

void spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, struct iovec *iov,

				      uint32_t iovcnt, uint64_t offset, uint32_t length);



void spdk_nvmf_ctrlr_destruct(struct spdk_nvmf_ctrlr *ctrlr);

int spdk_nvmf_ctrlr_process_fabrics_cmd(struct spdk_nvmf_request *req);

test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c

+1 −1
Original line number Diff line number Diff line
@@ -116,7 +116,7 @@ DEFINE_STUB(spdk_nvmf_ctrlr_write_zeroes_supported, 
	    false);



DEFINE_STUB_V(spdk_nvmf_get_discovery_log_page,

	      (struct spdk_nvmf_tgt *tgt, void *buffer, uint64_t offset, uint32_t length));

	      (struct spdk_nvmf_tgt *tgt, struct iovec *iov, uint32_t iovcnt, uint64_t offset, uint32_t length));



DEFINE_STUB(spdk_nvmf_request_complete,

	    int,

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

+10 −5
Original line number Diff line number Diff line
@@ -217,10 +217,14 @@ test_discovery_log(void) 
	struct spdk_nvmf_tgt tgt = {};

	struct spdk_nvmf_subsystem *subsystem;

	uint8_t buffer[8192];

	struct iovec iov;

	struct spdk_nvmf_discovery_log_page *disc_log;

	struct spdk_nvmf_discovery_log_page_entry *entry;

	struct spdk_nvme_transport_id trid = {};



	iov.iov_base = buffer;

	iov.iov_len = 8192;



	tgt.max_subsystems = 1024;

	tgt.subsystems = calloc(tgt.max_subsystems, sizeof(struct spdk_nvmf_subsystem *));

	SPDK_CU_ASSERT_FATAL(tgt.subsystems != NULL);
@@ -239,20 +243,20 @@ test_discovery_log(void) 
	/* Get only genctr (first field in the header) */

	memset(buffer, 0xCC, sizeof(buffer));

	disc_log = (struct spdk_nvmf_discovery_log_page *)buffer;

	spdk_nvmf_get_discovery_log_page(&tgt, buffer, 0, sizeof(disc_log->genctr));

	spdk_nvmf_get_discovery_log_page(&tgt, &iov, 1, 0, sizeof(disc_log->genctr));

	CU_ASSERT(disc_log->genctr == 1); /* one added subsystem */



	/* Get only the header, no entries */

	memset(buffer, 0xCC, sizeof(buffer));

	disc_log = (struct spdk_nvmf_discovery_log_page *)buffer;

	spdk_nvmf_get_discovery_log_page(&tgt, buffer, 0, sizeof(*disc_log));

	spdk_nvmf_get_discovery_log_page(&tgt, &iov, 1, 0, sizeof(*disc_log));

	CU_ASSERT(disc_log->genctr == 1);

	CU_ASSERT(disc_log->numrec == 1);



	/* Offset 0, exact size match */

	memset(buffer, 0xCC, sizeof(buffer));

	disc_log = (struct spdk_nvmf_discovery_log_page *)buffer;

	spdk_nvmf_get_discovery_log_page(&tgt, buffer, 0,

	spdk_nvmf_get_discovery_log_page(&tgt, &iov, 1, 0,

					 sizeof(*disc_log) + sizeof(disc_log->entries[0]));

	CU_ASSERT(disc_log->genctr != 0);

	CU_ASSERT(disc_log->numrec == 1);
@@ -261,7 +265,7 @@ test_discovery_log(void) 
	/* Offset 0, oversize buffer */

	memset(buffer, 0xCC, sizeof(buffer));

	disc_log = (struct spdk_nvmf_discovery_log_page *)buffer;

	spdk_nvmf_get_discovery_log_page(&tgt, buffer, 0, sizeof(buffer));

	spdk_nvmf_get_discovery_log_page(&tgt, &iov, 1, 0, sizeof(buffer));

	CU_ASSERT(disc_log->genctr != 0);

	CU_ASSERT(disc_log->numrec == 1);

	CU_ASSERT(disc_log->entries[0].trtype == 42);
@@ -271,7 +275,8 @@ test_discovery_log(void) 
	/* Get just the first entry, no header */

	memset(buffer, 0xCC, sizeof(buffer));

	entry = (struct spdk_nvmf_discovery_log_page_entry *)buffer;

	spdk_nvmf_get_discovery_log_page(&tgt, buffer,

	spdk_nvmf_get_discovery_log_page(&tgt, &iov,

					 1,

					 offsetof(struct spdk_nvmf_discovery_log_page, entries[0]),

					 sizeof(*entry));

	CU_ASSERT(entry->trtype == 42);