block: fix race in blk_time_get_ns() returning 0

blk_time_get_ns() populates the per-plug cached timestamp and then
returns it by re-reading the field:

	if (!plug->cur_ktime) {
		plug->cur_ktime = ktime_get_ns();
		current->flags |= PF_BLOCK_TS;
	}
	return plug->cur_ktime;

This is problematic when the compiler emits the final
"return plug->cur_ktime" as a reload from memory, after PF_BLOCK_TS has
already been set.

Since the cached timestamp is now invalidated from finish_task_switch()
(fad156c2af22 "block: invalidate cached plug timestamp after task
switch"), a task preempted between setting PF_BLOCK_TS and that reload
has plug->cur_ktime zeroed by blk_plug_invalidate_ts() when it is
scheduled back in.  The reload then returns 0.

A 0 handed back here is stored as a start timestamp -- e.g.
blk_account_io_start() writes it to rq->start_time_ns -- and later
subtracted from "now".  blk_account_io_done() then adds (now - 0), i.e.
roughly the system uptime, to the per-group nsecs[] counters.  On an
otherwise idle, healthy device this appears as sudden ~uptime-sized jumps
in the diskstats time fields (write_ticks/discard_ticks/time_in_queue).

The solution is to be explicit in our reads and writes to this field
that is preemption volatile.  We also add a barrier() to ensure that any
setting of PF_BLOCK_TS is ordered to happen after the cur_ktime update.

This issue was discovered using AI-assisted kprobes looking for paths
that were leaking zeroed timestamps in a live system, based on the
observation that we were sometimes seeing uptime-sized jumps in kernel
exported counters. This was flagged by NodeDiskIOSaturation
prometheus alerts that started firing on all hosts post 7.1.3 kernel
upgrade, due to node-exporter now exporting a nonsensical
node_disk_io_time_weighted_seconds_total.

Fixes: fad156c2af22 ("block: invalidate cached plug timestamp after task switch")
Cc: stable@vger.kernel.org
Signed-off-by: Mike Waychison <mike@waychison.com>
Assisted-by: Claude:claude-opus-4.8
Link: https://patch.msgid.link/20260715192950.2488921-1-mike@waychison.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/block/blk.h b/block/blk.h
index fb95d3c58950c1b66a4f2cd551c0d767e4a8f0bc..eaac05815cb036cba753d060270fa1acc2be928b 100644 (file)
--- a/block/blk.h
+++ b/block/blk.h
@@ -717,6 +717,7 @@ static inline int req_ref_read(struct request *req)
 static inline u64 blk_time_get_ns(void)
 {
        struct blk_plug *plug = current->plug;
+       u64 now;
 
        if (!plug || !in_task())
                return ktime_get_ns();
@@ -725,12 +726,18 @@ static inline u64 blk_time_get_ns(void)
         * 0 could very well be a valid time, but rather than flag "this is
         * a valid timestamp" separately, just accept that we'll do an extra
         * ktime_get_ns() if we just happen to get 0 as the current time.
+        *
+        * cur_ktime can be zeroed by pre-emption the moment PF_BLOCK_TS is set.
         */
-       if (!plug->cur_ktime) {
-               plug->cur_ktime = ktime_get_ns();
+       now = READ_ONCE(plug->cur_ktime);
+       if (!now) {
+               now = ktime_get_ns();
+               WRITE_ONCE(plug->cur_ktime, now);
+               /* Ensure PF_BLOCK_TS is set after cur_ktime. */
+               barrier();
                current->flags |= PF_BLOCK_TS;
        }
-       return plug->cur_ktime;
+       return now;
 }
 
 static inline ktime_t blk_time_get(void)