From a7d61bef5ad71ba47e55b2bcd8a7b1d4c9b40b39 Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Fri, 14 Jan 2022 13:14:54 +0100 Subject: [PATCH] nvme: guard admin qpair error injection queue Admin commands can be sent and polled from any thread, which also means that the error injection queue on the admin qpair can be accessed from multiple threads. Therefore, any modifications to that queue should be done under the ctrlr lock. Signed-off-by: Konrad Sztyber Change-Id: Ib1ed194405cb5b93f65a007b9749fd4433dc367d Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11099 Tested-by: SPDK CI Jenkins Reviewed-by: Michael Haeuptle Reviewed-by: Aleksey Marchuk Reviewed-by: Jim Harris Community-CI: Broadcom CI Community-CI: Mellanox Build Bot --- lib/nvme/nvme_qpair.c | 18 ++++++++++++++---- .../unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c | 7 +++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/nvme/nvme_qpair.c b/lib/nvme/nvme_qpair.c index eff2308c2..0dc4de741 100644 --- a/lib/nvme/nvme_qpair.c +++ b/lib/nvme/nvme_qpair.c @@ -1076,9 +1076,11 @@ spdk_nvme_qpair_add_cmd_error_injection(struct spdk_nvme_ctrlr *ctrlr, uint8_t sct, uint8_t sc) { struct nvme_error_cmd *entry, *cmd = NULL; + int rc = 0; if (qpair == NULL) { qpair = ctrlr->adminq; + nvme_robust_mutex_lock(&ctrlr->ctrlr_lock); } TAILQ_FOREACH(entry, &qpair->err_cmd_head, link) { @@ -1091,7 +1093,8 @@ spdk_nvme_qpair_add_cmd_error_injection(struct spdk_nvme_ctrlr *ctrlr, if (cmd == NULL) { cmd = spdk_zmalloc(sizeof(*cmd), 64, NULL, SPDK_ENV_LCORE_ID_ANY, SPDK_MALLOC_DMA); if (!cmd) { - return -ENOMEM; + rc = -ENOMEM; + goto out; } TAILQ_INSERT_TAIL(&qpair->err_cmd_head, cmd, link); } @@ -1102,8 +1105,12 @@ spdk_nvme_qpair_add_cmd_error_injection(struct spdk_nvme_ctrlr *ctrlr, cmd->opc = opc; cmd->status.sct = sct; cmd->status.sc = sc; +out: + if (nvme_qpair_is_admin_queue(qpair)) { + nvme_robust_mutex_unlock(&ctrlr->ctrlr_lock); + } - return 0; + return rc; } void @@ -1115,17 +1122,20 @@ spdk_nvme_qpair_remove_cmd_error_injection(struct spdk_nvme_ctrlr *ctrlr, if (qpair == NULL) { qpair = ctrlr->adminq; + nvme_robust_mutex_lock(&ctrlr->ctrlr_lock); } TAILQ_FOREACH_SAFE(cmd, &qpair->err_cmd_head, link, entry) { if (cmd->opc == opc) { TAILQ_REMOVE(&qpair->err_cmd_head, cmd, link); spdk_free(cmd); - return; + break; } } - return; + if (nvme_qpair_is_admin_queue(qpair)) { + nvme_robust_mutex_unlock(&ctrlr->ctrlr_lock); + } } uint16_t diff --git a/test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c b/test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c index aeea8c8eb..6fb41146d 100644 --- a/test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c +++ b/test/unit/lib/nvme/nvme_qpair.c/nvme_qpair_ut.c @@ -445,11 +445,17 @@ test_nvme_qpair_add_cmd_error_injection(void) { struct spdk_nvme_qpair qpair = {}; struct spdk_nvme_ctrlr ctrlr = {}; + pthread_mutexattr_t attr; int rc; prepare_submit_request_test(&qpair, &ctrlr); ctrlr.adminq = &qpair; + SPDK_CU_ASSERT_FATAL(pthread_mutexattr_init(&attr) == 0); + SPDK_CU_ASSERT_FATAL(pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE) == 0); + SPDK_CU_ASSERT_FATAL(pthread_mutex_init(&ctrlr.ctrlr_lock, &attr) == 0); + pthread_mutexattr_destroy(&attr); + /* Admin error injection at submission path */ MOCK_CLEAR(spdk_zmalloc); rc = spdk_nvme_qpair_add_cmd_error_injection(&ctrlr, NULL, @@ -498,6 +504,7 @@ test_nvme_qpair_add_cmd_error_injection(void) CU_ASSERT(TAILQ_EMPTY(&qpair.err_cmd_head)); + pthread_mutex_destroy(&ctrlr.ctrlr_lock); cleanup_submit_request_test(&qpair); }