From 27c42e313f3957b744410dddd1c6d47485cedfe3 Mon Sep 17 00:00:00 2001 From: Darek Stojaczyk Date: Fri, 15 Mar 2019 13:24:09 +0100 Subject: [PATCH] nvme: don't rely on phys_addr retrieved from spdk_malloc() The phys_addr param in spdk_*malloc() is about to be deprecated, so use a separate spdk_vtophys() call to retrieve physical addresses. This patch also adds error checks against SPDK_VTOPHYS_ERROR. The error handling paths are already there to account for spdk_*malloc() failures themselves, so reuse them in case of vtophys failures. Change-Id: I377636e66b8c570d013c1bb2021f04bce4e6c0ce Signed-off-by: Darek Stojaczyk Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/416998 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Changpeng Liu --- lib/nvme/nvme.c | 3 +-- lib/nvme/nvme_ctrlr.c | 31 ++++++++++++++++++++++--------- lib/nvme/nvme_pcie.c | 16 ++++++++++++++-- test/common/lib/test_env.c | 2 ++ 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/lib/nvme/nvme.c b/lib/nvme/nvme.c index d8f57178b7..8cf931482b 100644 --- a/lib/nvme/nvme.c +++ b/lib/nvme/nvme.c @@ -202,10 +202,9 @@ nvme_allocate_request_user_copy(struct spdk_nvme_qpair *qpair, { struct nvme_request *req; void *dma_buffer = NULL; - uint64_t phys_addr; if (buffer && payload_size) { - dma_buffer = spdk_zmalloc(payload_size, 4096, &phys_addr, + dma_buffer = spdk_zmalloc(payload_size, 4096, NULL, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_DMA); if (!dma_buffer) { return NULL; diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index fcc95fe20c..35467e1d06 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -394,12 +394,11 @@ nvme_ctrlr_construct_intel_support_log_page_list(struct spdk_nvme_ctrlr *ctrlr, static int nvme_ctrlr_set_intel_support_log_pages(struct spdk_nvme_ctrlr *ctrlr) { int rc = 0; - uint64_t phys_addr = 0; struct nvme_completion_poll_status status; struct spdk_nvme_intel_log_page_directory *log_page_directory; log_page_directory = spdk_zmalloc(sizeof(struct spdk_nvme_intel_log_page_directory), - 64, &phys_addr, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_DMA); + 64, NULL, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_DMA); if (log_page_directory == NULL) { SPDK_ERRLOG("could not allocate log_page_directory\n"); return -ENXIO; @@ -754,7 +753,7 @@ static int nvme_ctrlr_set_doorbell_buffer_config(struct spdk_nvme_ctrlr *ctrlr) { int rc = 0; - uint64_t prp1, prp2; + uint64_t prp1, prp2, len; if (!ctrlr->cdata.oacs.doorbell_buffer_config) { nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_SET_KEEP_ALIVE_TIMEOUT, @@ -770,18 +769,32 @@ nvme_ctrlr_set_doorbell_buffer_config(struct spdk_nvme_ctrlr *ctrlr) /* only 1 page size for doorbell buffer */ ctrlr->shadow_doorbell = spdk_dma_zmalloc(ctrlr->page_size, ctrlr->page_size, - &prp1); + NULL); if (ctrlr->shadow_doorbell == NULL) { rc = -ENOMEM; goto error; } - ctrlr->eventidx = spdk_dma_zmalloc(ctrlr->page_size, ctrlr->page_size, &prp2); + len = ctrlr->page_size; + prp1 = spdk_vtophys(ctrlr->shadow_doorbell, &len); + if (prp1 == SPDK_VTOPHYS_ERROR || len != ctrlr->page_size) { + rc = -EFAULT; + goto error; + } + + ctrlr->eventidx = spdk_dma_zmalloc(ctrlr->page_size, ctrlr->page_size, NULL); if (ctrlr->eventidx == NULL) { rc = -ENOMEM; goto error; } + len = ctrlr->page_size; + prp2 = spdk_vtophys(ctrlr->eventidx, &len); + if (prp2 == SPDK_VTOPHYS_ERROR || len != ctrlr->page_size) { + rc = -EFAULT; + goto error; + } + nvme_ctrlr_set_state(ctrlr, NVME_CTRLR_STATE_WAIT_FOR_DB_BUF_CFG, ctrlr->opts.admin_timeout_ms); @@ -1457,7 +1470,6 @@ nvme_ctrlr_construct_namespaces(struct spdk_nvme_ctrlr *ctrlr) { int rc = 0; uint32_t nn = ctrlr->cdata.nn; - uint64_t phys_addr = 0; /* ctrlr->num_ns may be 0 (startup) or a different number of namespaces (reset), * so check if we need to reallocate. @@ -1470,15 +1482,16 @@ nvme_ctrlr_construct_namespaces(struct spdk_nvme_ctrlr *ctrlr) return 0; } - ctrlr->ns = spdk_zmalloc(nn * sizeof(struct spdk_nvme_ns), 64, - &phys_addr, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_SHARE); + ctrlr->ns = spdk_zmalloc(nn * sizeof(struct spdk_nvme_ns), 64, NULL, + SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_SHARE); if (ctrlr->ns == NULL) { rc = -ENOMEM; goto fail; } ctrlr->nsdata = spdk_zmalloc(nn * sizeof(struct spdk_nvme_ns_data), 64, - &phys_addr, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_SHARE | SPDK_MALLOC_DMA); + NULL, SPDK_ENV_SOCKET_ID_ANY, + SPDK_MALLOC_SHARE | SPDK_MALLOC_DMA); if (ctrlr->nsdata == NULL) { rc = -ENOMEM; goto fail; diff --git a/lib/nvme/nvme_pcie.c b/lib/nvme/nvme_pcie.c index 44232131b3..adb1a7f7ae 100644 --- a/lib/nvme/nvme_pcie.c +++ b/lib/nvme/nvme_pcie.c @@ -1012,22 +1012,34 @@ nvme_pcie_qpair_construct(struct spdk_nvme_qpair *qpair) */ if (pqpair->sq_in_cmb == false) { pqpair->cmd = spdk_zmalloc(pqpair->num_entries * sizeof(struct spdk_nvme_cmd), - page_align, &pqpair->cmd_bus_addr, + page_align, NULL, SPDK_ENV_SOCKET_ID_ANY, flags); if (pqpair->cmd == NULL) { SPDK_ERRLOG("alloc qpair_cmd failed\n"); return -ENOMEM; } + + pqpair->cmd_bus_addr = spdk_vtophys(pqpair->cmd, NULL); + if (pqpair->cmd_bus_addr == SPDK_VTOPHYS_ERROR) { + SPDK_ERRLOG("spdk_vtophys(pqpair->cmd) failed\n"); + return -EFAULT; + } } pqpair->cpl = spdk_zmalloc(pqpair->num_entries * sizeof(struct spdk_nvme_cpl), - page_align, &pqpair->cpl_bus_addr, + page_align, NULL, SPDK_ENV_SOCKET_ID_ANY, flags); if (pqpair->cpl == NULL) { SPDK_ERRLOG("alloc qpair_cpl failed\n"); return -ENOMEM; } + pqpair->cpl_bus_addr = spdk_vtophys(pqpair->cpl, NULL); + if (pqpair->cpl_bus_addr == SPDK_VTOPHYS_ERROR) { + SPDK_ERRLOG("spdk_vtophys(pqpair->cpl) failed\n"); + return -EFAULT; + } + doorbell_base = &pctrlr->regs->doorbell[0].sq_tdbl; pqpair->sq_tdbl = doorbell_base + (2 * qpair->id + 0) * pctrlr->doorbell_stride_u32; pqpair->cq_hdbl = doorbell_base + (2 * qpair->id + 1) * pctrlr->doorbell_stride_u32; diff --git a/test/common/lib/test_env.c b/test/common/lib/test_env.c index 52fb23d150..0b8cc74b19 100644 --- a/test/common/lib/test_env.c +++ b/test/common/lib/test_env.c @@ -153,6 +153,8 @@ spdk_dma_realloc(void *buf, size_t size, size_t align, uint64_t *phys_addr) void spdk_free(void *buf) { + /* fix for false-positives in *certain* static analysis tools. */ + assert((uintptr_t)buf != UINTPTR_MAX); free(buf); }