From a52fc70d5188d0fee2c298e58d23a5f07978af32 Mon Sep 17 00:00:00 2001 From: Seth Howell Date: Thu, 29 Nov 2018 14:11:31 -0700 Subject: [PATCH] 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 1d9be84bfdf8 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: Seth Howell Reviewed-on: https://review.gerrithub.io/435544 Tested-by: SPDK CI Jenkins Chandler-Test-Pool: SPDK Automated Test System Reviewed-by: Ben Walker Reviewed-by: Shuhei Matsumoto --- lib/nvmf/ctrlr.c | 2 +- lib/nvmf/ctrlr_discovery.c | 39 ++++++++++++------- lib/nvmf/nvmf_internal.h | 5 +-- test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c | 2 +- .../ctrlr_discovery.c/ctrlr_discovery_ut.c | 15 ++++--- 5 files changed, 39 insertions(+), 24 deletions(-) diff --git a/lib/nvmf/ctrlr.c b/lib/nvmf/ctrlr.c index f7e56f2cc..a71eaaa8a 100644 --- a/lib/nvmf/ctrlr.c +++ b/lib/nvmf/ctrlr.c @@ -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; diff --git a/lib/nvmf/ctrlr_discovery.c b/lib/nvmf/ctrlr_discovery.c index 6d1aed6d8..c5e325c2a 100644 --- a/lib/nvmf/ctrlr_discovery.c +++ b/lib/nvmf/ctrlr_discovery.c @@ -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); - /* Zero out the rest of the buffer */ - if (zero_len) { - memset((char *)buffer + copy_len, 0, zero_len); - } + memcpy(tmp->iov_base, (char *)tgt->discovery_log_page + offset, copy_len); - /* We should have copied or zeroed every byte of the output buffer. */ - assert(copy_len + zero_len == length); + 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 *)tmp->iov_base + copy_len, 0, zero_len); + } + + for (++tmp; tmp < iov + iovcnt; tmp++) { + memset((char *)tmp->iov_base, 0, tmp->iov_len); + } + } } diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index 89de3dd2b..7d24f7d3a 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/lib/nvmf/nvmf_internal.h @@ -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); diff --git a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c index c49ae2a8e..e76ee416a 100644 --- a/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c +++ b/test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c @@ -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, diff --git a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c index 8e6653861..8e3de340b 100644 --- a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c +++ b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c @@ -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);