ivampiresp/Spdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

nvme: Init the status object when tracking the req completion

Browse Source 
Currently nvme_completion_poll_status object is allocated using
malloc, so it may cotnain some garbage. In some scenarious
nvme_completion_poll_cb can be triggered before we enter
spdk_nvme_wait_for_completion_*. In that case status object
will be freed by nvme_completion_poll_cb if it contains a
garbage in `timed_out` field. Later spdk_nvme_wait_for_completion
will work with already freed memory.
Fix - allocate nvme_completion_poll_status object using
calloc and explicitly zerofy it before usage

Fixes #1292

Change-Id: Iac39653a6cd102471de16e65814f0760bbeda7d9
Signed-off-by: Alexey Marchuk <alexeymar@mellanox.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/1373
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: <dongx.yi@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
This commit is contained in:
Alexey Marchuk 2020-03-19 12:55:55 +03:00 committed by Tomasz Zawadzki
parent b9a187977d
commit 24d61956ab
7 changed files with 41 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
lib/nvme/nvme.c
View File

@ -106,7 +106,8 @@ nvme_completion_poll_cb(void *arg, const struct spdk_nvme_cpl *cpl)
* Poll qpair for completions until a command completes.
*
* \param qpair queue to poll
* \param status completion status
* \param status completion status. The user must fill this structure with zeroes before calling
* this function
* \param robust_mutex optional robust mutex to lock while polling qpair
*
* \return 0 if command completed without error,
@ -122,7 +123,6 @@ spdk_nvme_wait_for_completion_robust_lock(
struct nvme_completion_poll_status *status,
pthread_mutex_t *robust_mutex)
{
memset(status, 0, sizeof(*status));
int rc;
while (status->done == false) {
@ -160,7 +160,8 @@ spdk_nvme_wait_for_completion(struct spdk_nvme_qpair *qpair,
* Poll qpair for completions until a command completes.
*
* \param qpair queue to poll
* \param status completion status
* \param status completion status. The user must fill this structure with zeroes before calling
* this function
* \param timeout_in_secs optional timeout
*
* \return 0 if command completed without error,
@ -178,7 +179,6 @@ spdk_nvme_wait_for_completion_timeout(struct spdk_nvme_qpair *qpair,
uint64_t timeout_tsc = 0;
int rc = 0;
memset(status, 0, sizeof(*status));
if (timeout_in_secs) {
timeout_tsc = spdk_get_ticks() + timeout_in_secs * spdk_get_ticks_hz();
}

25
lib/nvme/nvme_ctrlr.c
View File

@ -593,7 +593,7 @@ static int nvme_ctrlr_set_intel_support_log_pages(struct spdk_nvme_ctrlr *ctrlr)
return -ENXIO;
}
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
spdk_free(log_page_directory);
@ -673,7 +673,7 @@ nvme_ctrlr_set_arbitration_feature(struct spdk_nvme_ctrlr *ctrlr)
return;
}
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return;
@ -1362,7 +1362,7 @@ nvme_ctrlr_identify_active_ns(struct spdk_nvme_ctrlr *ctrlr)
return -ENOMEM;
}
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
spdk_free(new_ns_list);
@ -1375,6 +1375,7 @@ nvme_ctrlr_identify_active_ns(struct spdk_nvme_ctrlr *ctrlr)
* there are no more active namespaces
*/
for (i = 0; i < num_pages; i++) {
memset(status, 0, sizeof(*status));
rc = nvme_ctrlr_cmd_identify(ctrlr, SPDK_NVME_IDENTIFY_ACTIVE_NS_LIST, 0, next_nsid,
&new_ns_list[1024 * i], sizeof(struct spdk_nvme_ns_list),
nvme_completion_poll_cb, status);
@ -2961,7 +2962,7 @@ spdk_nvme_ctrlr_attach_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid,
int res;
struct spdk_nvme_ns *ns;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -2999,7 +3000,7 @@ spdk_nvme_ctrlr_detach_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid,
int res;
struct spdk_nvme_ns *ns;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -3040,7 +3041,7 @@ spdk_nvme_ctrlr_create_ns(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_ns_dat
uint32_t nsid;
struct spdk_nvme_ns *ns;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return 0;
@ -3079,7 +3080,7 @@ spdk_nvme_ctrlr_delete_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid)
int res;
struct spdk_nvme_ns *ns;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -3117,7 +3118,7 @@ spdk_nvme_ctrlr_format(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid,
struct nvme_completion_poll_status *status;
int res;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -3171,7 +3172,7 @@ spdk_nvme_ctrlr_update_firmware(struct spdk_nvme_ctrlr *ctrlr, void *payload, ui
return -1;
}
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -3185,6 +3186,7 @@ spdk_nvme_ctrlr_update_firmware(struct spdk_nvme_ctrlr *ctrlr, void *payload, ui
while (size_remaining > 0) {
transfer = spdk_min(size_remaining, ctrlr->min_page_size);
memset(status, 0, sizeof(*status));
res = nvme_ctrlr_cmd_fw_image_download(ctrlr, transfer, offset, p,
nvme_completion_poll_cb,
status);
@ -3210,6 +3212,7 @@ spdk_nvme_ctrlr_update_firmware(struct spdk_nvme_ctrlr *ctrlr, void *payload, ui
fw_commit.fs = slot;
fw_commit.ca = commit_action;
memset(status, 0, sizeof(*status));
res = nvme_ctrlr_cmd_fw_commit(ctrlr, &fw_commit, nvme_completion_poll_cb,
status);
if (res) {
@ -3283,7 +3286,7 @@ spdk_nvme_ctrlr_security_receive(struct spdk_nvme_ctrlr *ctrlr, uint8_t secp,
struct nvme_completion_poll_status *status;
int res;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -3314,7 +3317,7 @@ spdk_nvme_ctrlr_security_send(struct spdk_nvme_ctrlr *ctrlr, uint8_t secp,
struct nvme_completion_poll_status *status;
int res;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;

10
lib/nvme/nvme_fabric.c
View File

@ -50,7 +50,7 @@ nvme_fabric_prop_set_cmd(struct spdk_nvme_ctrlr *ctrlr,
assert(size == SPDK_NVMF_PROP_SIZE_4 || size == SPDK_NVMF_PROP_SIZE_8);
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -93,7 +93,7 @@ nvme_fabric_prop_get_cmd(struct spdk_nvme_ctrlr *ctrlr,
assert(size == SPDK_NVMF_PROP_SIZE_4 || size == SPDK_NVMF_PROP_SIZE_8);
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -231,7 +231,7 @@ nvme_fabric_get_discovery_log_page(struct spdk_nvme_ctrlr *ctrlr,
struct nvme_completion_poll_status *status;
int rc;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -294,7 +294,7 @@ nvme_fabric_ctrlr_scan(struct spdk_nvme_probe_ctx *probe_ctx,
return -1;
}
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
nvme_ctrlr_destruct(discovery_ctrlr);
@ -413,7 +413,7 @@ nvme_fabric_qpair_connect(struct spdk_nvme_qpair *qpair, uint32_t num_entries)
return -ENOMEM;
}
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
spdk_free(nvmf_data);

4
lib/nvme/nvme_ns.c
View File

@ -117,7 +117,7 @@ nvme_ctrlr_identify_ns(struct spdk_nvme_ns *ns)
struct spdk_nvme_ns_data *nsdata;
int rc;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -163,7 +163,7 @@ nvme_ctrlr_identify_id_desc(struct spdk_nvme_ns *ns)
return 0;
}
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;

9
lib/nvme/nvme_pcie.c
View File

@ -1567,7 +1567,7 @@ _nvme_pcie_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme
struct nvme_completion_poll_status *status;
int rc;
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -1587,6 +1587,7 @@ _nvme_pcie_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme
return -1;
}
memset(status, 0, sizeof(*status));
rc = nvme_pcie_ctrlr_cmd_create_io_sq(qpair->ctrlr, qpair, nvme_completion_poll_cb, status);
if (rc != 0) {
free(status);
@ -1598,13 +1599,14 @@ _nvme_pcie_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme
if (status->timed_out) {
/* Request is still queued, the memory will be freed in a completion callback.
allocate a new request */
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
}
}
memset(status, 0, sizeof(*status));
/* Attempt to delete the completion queue */
rc = nvme_pcie_ctrlr_cmd_delete_io_cq(qpair->ctrlr, qpair, nvme_completion_poll_cb, status);
if (rc != 0) {
@ -1704,7 +1706,7 @@ nvme_pcie_ctrlr_delete_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_
goto free;
}
status = malloc(sizeof(*status));
status = calloc(1, sizeof(*status));
if (!status) {
SPDK_ERRLOG("Failed to allocate status tracker\n");
return -ENOMEM;
@ -1724,6 +1726,7 @@ nvme_pcie_ctrlr_delete_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_
return -1;
}
memset(status, 0, sizeof(*status));
/* Delete the completion queue */
rc = nvme_pcie_ctrlr_cmd_delete_io_cq(ctrlr, qpair, nvme_completion_poll_cb, status);
if (rc != 0) {

10
test/unit/lib/nvme/nvme.c/nvme_ut.c
View File

@ -1229,18 +1229,18 @@ test_nvme_wait_for_completion(void)
int rc = 0;
memset(&qpair, 0, sizeof(qpair));
memset(&g_status, 0, sizeof(g_status));
/* completion timeout */
memset(&g_status, 0, sizeof(g_status));
completion_delay = 2;
timeout_in_secs = 1;
g_status.done = true;
rc = spdk_nvme_wait_for_completion_timeout(&qpair, &g_status, timeout_in_secs);
CU_ASSERT(g_status.timed_out == true);
CU_ASSERT(g_status.done == false);
CU_ASSERT(rc == -ECANCELED);
/* spdk_nvme_qpair_process_completions returns error */
memset(&g_status, 0, sizeof(g_status));
g_process_comp_result = -1;
completion_delay = 1;
timeout_in_secs = 2;
@ -1254,6 +1254,7 @@ test_nvme_wait_for_completion(void)
g_process_comp_result = 0;
/* complete in time */
memset(&g_status, 0, sizeof(g_status));
completion_delay = 1;
timeout_in_secs = 2;
rc = spdk_nvme_wait_for_completion_timeout(&qpair, &g_status, timeout_in_secs);
@ -1263,6 +1264,7 @@ test_nvme_wait_for_completion(void)
/* spdk_nvme_wait_for_completion */
/* spdk_nvme_qpair_process_completions returns error */
memset(&g_status, 0, sizeof(g_status));
g_process_comp_result = -1;
rc = spdk_nvme_wait_for_completion(&qpair, &g_status);
CU_ASSERT(rc == -ECANCELED);
@ -1271,9 +1273,9 @@ test_nvme_wait_for_completion(void)
CU_ASSERT(g_status.cpl.status.sct == SPDK_NVME_SCT_GENERIC);
CU_ASSERT(g_status.cpl.status.sc == SPDK_NVME_SC_ABORTED_SQ_DELETION);
g_process_comp_result = 0;
/* successful completion */
memset(&g_status, 0, sizeof(g_status));
g_process_comp_result = 0;
rc = spdk_nvme_wait_for_completion(&qpair, &g_status);
CU_ASSERT(rc == 0);
CU_ASSERT(g_status.timed_out == false);

5
test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c
View File

@ -275,6 +275,10 @@ void
nvme_completion_poll_cb(void *arg, const struct spdk_nvme_cpl *cpl)
{
struct nvme_completion_poll_status *status = arg;
/* This should not happen it test env since this callback is always called
* before wait_for_completion_* while this field can only be set to true in
* wait_for_completion_* functions */
CU_ASSERT(status->timed_out == false);
status->cpl = *cpl;
status->done = true;
@ -288,7 +292,6 @@ spdk_nvme_wait_for_completion_robust_lock(
struct nvme_completion_poll_status *status,
pthread_mutex_t *robust_mutex)
{
memset(status, 0, sizeof(*status));
if (spdk_nvme_qpair_process_completions(qpair, 0) < 0) {
g_failed_status = status;
status->timed_out = true;