[PATCH] nvme: avoid wrapping admin_timeout and io_timeout

[PATCH] nvme: avoid wrapping admin_timeout and io_timeout

[Yingjie Wang]

NVME_ADMIN_TIMEOUT and NVME_IO_TIMEOUT multiply the admin_timeout and
io_timeout module parameters by HZ in unsigned int arithmetic, and
nvme_fw_act_work() does the same through secs_to_jiffies(). The parameters
accept any unsigned int, so values above UINT_MAX / HZ wrap. With HZ=1000,
io_timeout=4294968 becomes 704 jiffies, and a controller created afterwards
times out I/O after 0.7 seconds instead of about 49.7 days. On 32-bit
systems, results above MAX_JIFFY_OFFSET are also unsafe relative delays and
can be treated as already expired.

These large values are used in practice. AWS documents
nvme_core.io_timeout=4294967295 for EBS volumes and says Amazon Linux uses
it by default. With HZ=1000, the current code happens to wrap this setting
to 4294966296 jiffies, which still behaves as a long timeout on 64-bit
systems.

Add nvme_timeout_to_jiffies(), which caps the seconds value so that the
result fits the u32 timeout fields and stays within MAX_JIFFY_OFFSET for
32-bit timers, and use it for both macros and for the firmware
activation wait. On 64-bit kernels every timeout that did not wrap is
unchanged. Larger values saturate instead of being rejected so that the
documented setting keeps a long timeout.

Fixes: 8ae4e4477d8f ("nvme: update timeout module parameter type")
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <1075151112@qq.com>
---

Notes:
    Unlike the recent tls_handshake_timeout fix, which rejects out-of-range
    values, this saturates so the documented io_timeout=4294967295 setting
    remains accepted as a long timeout.
    
    AWS documentation referenced above:
    https://docs.aws.amazon.com/ebs/latest/userguide/timeout-nvme-ebs-volumes.html
    
    Tested on nvme-7.4 f33c8bc78540, x86-64 (HZ=1000) in QEMU. With a
    3-second dm-delay read, io_timeout=4294968 timed out after 704 ms and
    failed with EIO on the base kernel; with this patch the read completed
    after 3 seconds with no timeout. The 4294967 boundary and a 1-second
    control behaved the same on both kernels. admin_timeout and io_timeout
    readback also matched the expected boundaries over nvme-loop.
    
    The affected objects built with W=1 for x86-64 allmodconfig and i386
    HZ=1000; the i386 objects had no 64-bit division helper dependency.

 drivers/nvme/host/core.c |  2 +-
 drivers/nvme/host/nvme.h | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 03671bba1754..af8ccbc11c53 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4891,7 +4891,7 @@ static void nvme_fw_act_work(struct work_struct *work)
 	if (ctrl->mtfa)
 		fw_act_timeout = jiffies + msecs_to_jiffies(ctrl->mtfa * 100);
 	else
-		fw_act_timeout = jiffies + secs_to_jiffies(admin_timeout);
+		fw_act_timeout = jiffies + NVME_ADMIN_TIMEOUT;
 
 	nvme_quiesce_io_queues(ctrl);
 	while (nvme_ctrl_pp_status(ctrl)) {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 82cdc7d32b22..8305fd82038b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -22,11 +22,20 @@
 
 extern const struct pr_ops nvme_pr_ops;
 
+/* Seconds to jiffies, capped for the u32 timeouts and timer delays */
+static inline unsigned int nvme_timeout_to_jiffies(unsigned int timeout)
+{
+	unsigned long max_secs;
+
+	max_secs = min_t(unsigned long, UINT_MAX, MAX_JIFFY_OFFSET) / HZ;
+	return min_t(unsigned long, timeout, max_secs) * HZ;
+}
+
 extern unsigned int nvme_io_timeout;
-#define NVME_IO_TIMEOUT	(nvme_io_timeout * HZ)
+#define NVME_IO_TIMEOUT	nvme_timeout_to_jiffies(nvme_io_timeout)
 
 extern unsigned int admin_timeout;
-#define NVME_ADMIN_TIMEOUT	(admin_timeout * HZ)
+#define NVME_ADMIN_TIMEOUT	nvme_timeout_to_jiffies(admin_timeout)
 
 #define NVME_DEFAULT_KATO	5
 

base-commit: f33c8bc785401669b29229bcf3d0086ba9f5b87c
-- 
2.43.0

Re: [PATCH] nvme: avoid wrapping admin_timeout and io_timeout

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>