From 3512714b3f3dbb9f432bdc5dba0d85fc65fd7b22 Mon Sep 17 00:00:00 2001 From: Alexey Marchuk Date: Fri, 15 Jul 2022 13:10:52 +0300 Subject: [PATCH] nvme_fabrics: Lock mutext when prcessing set/get regs That is possible to get/set registers from any thread, during regs processing we are polling admin qpair to get a completion. At the same time, another thread can also poll admin qpair and that can lead to undefined behavior. This patch fixes an issue when bdev_nvme is configured with io_timeout. If remote target becomes unresponsive (e.g. due to link down), IO timeout occurs and bdev_nvme tries to get csts registers in timeout_cb. At the same time another thread can process adminq, so we may have 2 simultaneous adminq polls. If admin qpair is disconnecting at that time (RDMA transport) we may destroy resources twice from different threads. We don't see a problem with set_regs function but it won't be redundant to lock mutex in set_regs as well. Signed-off-by: Alexey Marchuk Change-Id: I7ec3984d25d0249061005533d13b22315b44ddf2 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/13687 Tested-by: SPDK CI Jenkins Reviewed-by: Konrad Sztyber Reviewed-by: Jim Harris --- lib/nvme/nvme_fabric.c | 6 +++--- test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c | 12 ++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/nvme/nvme_fabric.c b/lib/nvme/nvme_fabric.c index 17c52d775..e88757098 100644 --- a/lib/nvme/nvme_fabric.c +++ b/lib/nvme/nvme_fabric.c @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: BSD-3-Clause * Copyright (c) Intel Corporation. All rights reserved. * Copyright (c) 2020 Mellanox Technologies LTD. All rights reserved. - * Copyright (c) 2021 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * Copyright (c) 2021, 2022 NVIDIA CORPORATION & AFFILIATES. All rights reserved. */ /* @@ -59,7 +59,7 @@ nvme_fabric_prop_set_cmd_sync(struct spdk_nvme_ctrlr *ctrlr, return rc; } - if (nvme_wait_for_completion(ctrlr->adminq, status)) { + if (nvme_wait_for_completion_robust_lock(ctrlr->adminq, status, &ctrlr->ctrlr_lock)) { if (!status->timed_out) { free(status); } @@ -145,7 +145,7 @@ nvme_fabric_prop_get_cmd_sync(struct spdk_nvme_ctrlr *ctrlr, return rc; } - if (nvme_wait_for_completion(ctrlr->adminq, status)) { + if (nvme_wait_for_completion_robust_lock(ctrlr->adminq, status, &ctrlr->ctrlr_lock)) { if (!status->timed_out) { free(status); } diff --git a/test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c b/test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c index bd78cd9d5..ec174b9d2 100644 --- a/test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c +++ b/test/unit/lib/nvme/nvme_fabric.c/nvme_fabric_ut.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: BSD-3-Clause * Copyright (c) Intel Corporation. * All rights reserved. + * Copyright (c) 2022 NVIDIA CORPORATION & AFFILIATES. All rights reserved. */ #include "spdk/stdinc.h" @@ -166,6 +167,17 @@ nvme_wait_for_completion(struct spdk_nvme_qpair *qpair, return 0; } +DEFINE_RETURN_MOCK(nvme_wait_for_completion_robust_lock, int); +int +nvme_wait_for_completion_robust_lock(struct spdk_nvme_qpair *qpair, + struct nvme_completion_poll_status *status, + pthread_mutex_t *robust_mutex) +{ + status->timed_out = false; + HANDLE_RETURN_MOCK(nvme_wait_for_completion_robust_lock); + return 0; +} + DEFINE_RETURN_MOCK(spdk_nvme_ctrlr_cmd_admin_raw, int); int spdk_nvme_ctrlr_cmd_admin_raw(struct spdk_nvme_ctrlr *ctrlr,