From 06746448c1fcae6809f34f81eafe80a1ad4a0962 Mon Sep 17 00:00:00 2001 From: Seth Howell Date: Wed, 25 Sep 2019 15:32:40 -0700 Subject: [PATCH] nvme: fix confusion around nvme_ctrlr_set_state In most places, we are passing NVME_TIMEOUT_INFINITE as the timeout_in_ms argument to nvme_ctrlr_set_state, presumably in an attempt to specify an infinite timeout. However, nvme_ctrlr_set_state only checked against 0 when setting the actual timeout, and we didn't have any logic to check for overflow so we just ended up setting random timeout_tsc values which changes the behavior of the nvme_ctrlr_process_init function in several places. So, change NVME_TIMEOUT_INFINITE to 0, and add some integer overflow checking to nvme_ctrlr_set_state. Change-Id: Ic9d0cc57ed153df30c3b20313c3742072a5f992d Signed-off-by: Seth Howell Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/469485 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Ben Walker Reviewed-by: Shuhei Matsumoto Reviewed-by: Alexey Marchuk --- lib/nvme/nvme_ctrlr.c | 34 ++++++++++++++++++++++++++-------- lib/nvme/nvme_internal.h | 2 +- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index bd9013040..828421495 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -828,16 +828,34 @@ static void nvme_ctrlr_set_state(struct spdk_nvme_ctrlr *ctrlr, enum nvme_ctrlr_state state, uint64_t timeout_in_ms) { + uint64_t ticks_per_ms, timeout_in_ticks, now_ticks; + ctrlr->state = state; - if (timeout_in_ms == 0) { - SPDK_DEBUGLOG(SPDK_LOG_NVME, "setting state to %s (no timeout)\n", - nvme_ctrlr_state_string(ctrlr->state)); - ctrlr->state_timeout_tsc = NVME_TIMEOUT_INFINITE; - } else { - SPDK_DEBUGLOG(SPDK_LOG_NVME, "setting state to %s (timeout %" PRIu64 " ms)\n", - nvme_ctrlr_state_string(ctrlr->state), timeout_in_ms); - ctrlr->state_timeout_tsc = spdk_get_ticks() + (timeout_in_ms * spdk_get_ticks_hz()) / 1000; + if (timeout_in_ms == NVME_TIMEOUT_INFINITE) { + goto inf; } + + ticks_per_ms = spdk_get_ticks_hz() / 1000; + if (timeout_in_ms > UINT64_MAX / ticks_per_ms) { + SPDK_ERRLOG("Specified timeout would cause integer overflow. Defaulting to no timeout.\n"); + goto inf; + } + + now_ticks = spdk_get_ticks(); + timeout_in_ticks = timeout_in_ms * ticks_per_ms; + if (timeout_in_ticks > UINT64_MAX - now_ticks) { + SPDK_ERRLOG("Specified timeout would cause integer overflow. Defaulting to no timeout.\n"); + goto inf; + } + + ctrlr->state_timeout_tsc = timeout_in_ticks + now_ticks; + SPDK_DEBUGLOG(SPDK_LOG_NVME, "setting state to %s (timeout %" PRIu64 " ms)\n", + nvme_ctrlr_state_string(ctrlr->state), ctrlr->state_timeout_tsc); + return; +inf: + SPDK_DEBUGLOG(SPDK_LOG_NVME, "setting state to %s (no timeout)\n", + nvme_ctrlr_state_string(ctrlr->state)); + ctrlr->state_timeout_tsc = NVME_TIMEOUT_INFINITE; } static void diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index fcf463d3c..cbd6c0f74 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -559,7 +559,7 @@ enum nvme_ctrlr_state { NVME_CTRLR_STATE_ERROR }; -#define NVME_TIMEOUT_INFINITE UINT64_MAX +#define NVME_TIMEOUT_INFINITE 0 /* * Used to track properties for all processes accessing the controller.