zram: fix idle age_sec underflow in idle_store()

PATCH Applied Hao Jia 3 participants 7 messages Aug 28 – Sep 1

Subsystems (3)

Stable branch Block layer zram

lore

Share & tools

lore

Hao Jia

author

2026-08-28 08:32 UTC #0 lore raw

PATCH v2 1/1 zram: fix idle age_sec underflow in idle_store()

From: Hao Jia <[email protected]>

After commit 2e8ff2f51dde ("zram: use u32 for entry ac_time tracking"), idle_store() computes the idle cutoff as:

	cutoff = ktime_sub((u32)ktime_get_boottime_seconds(), age_sec);

Because the left operand is cast to u32, when age_sec exceeds the current uptime the subtraction wraps modulo 2^32 and the huge result is zero-extended into the s64 cutoff. mark_idle() then marks every entry as idle instead of matching nothing. For instance, running

	echo 86400 > /sys/block/zramX/idle

on a machine up for only two minutes marks all newly written pages idle and hands them to idle writeback and recompression.

No slot can have been accessed before the system booted, so an age_sec that reaches back past uptime cannot match any slot. Return early in that case, without walking the table or taking any slot locks.

Track the cutoff as time64_t rather than ktime_t. Both cutoff and ac_time are boot-time values in seconds, so a plain arithmetic comparison against ac_time in mark_idle() is correct and no ktime helpers are needed.

Fixes 2e8ff2f51dde ("zram: use u32 for entry ac_time tracking")

Cc [email protected]

Signed-off-by Hao Jia <[email protected]>

---
 drivers/block/zram/zram_drv.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

drivers/block/zram/zram_drv.c +13 -8

index a9b3bb1d3bef..4ba0f77b2abd 100644
@@ -415,7 +415,7 @@ static ssize_t mem_used_max_store(struct device *dev,
  * Mark all pages which are older than or equal to cutoff as IDLE.
  * Callers should hold the zram init lock in read mode
  */
-static void mark_idle(struct zram *zram, ktime_t cutoff)
+static void mark_idle(struct zram *zram, time64_t cutoff)
 {
 	int is_idle = 1;
 	unsigned long nr_pages = zram->disksize >> PAGE_SHIFT;

@@ -439,7 +439,7 @@ static void mark_idle(struct zram *zram, ktime_t cutoff)
 
 #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME
 		is_idle = !cutoff ||
-			ktime_after(cutoff, zram->table[index].attr.ac_time);
+			cutoff > zram->table[index].attr.ac_time;
 #endif
 		if (is_idle)
 			set_slot_flag(zram, index, ZRAM_IDLE);

@@ -453,21 +453,26 @@ static ssize_t idle_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t len)
 {
 	struct zram *zram = dev_to_zram(dev);
-	ktime_t cutoff = 0;
+	time64_t cutoff = 0;
 
 	if (!sysfs_streq(buf, "all")) {
 		/*
 		 * If it did not parse as 'all' try to treat it as an integer
 		 * when we have memory tracking enabled.
 		 */
+		time64_t uptime;
 		u32 age_sec;
 
-		if (IS_ENABLED(CONFIG_ZRAM_TRACK_ENTRY_ACTIME) &&
-		    !kstrtouint(buf, 0, &age_sec))
-			cutoff = ktime_sub((u32)ktime_get_boottime_seconds(),
-					   age_sec);
-		else
+		if (!IS_ENABLED(CONFIG_ZRAM_TRACK_ENTRY_ACTIME) ||
+		    kstrtouint(buf, 0, &age_sec))
 			return -EINVAL;
+
+		/* No slot can be older than the system uptime */
+		uptime = ktime_get_boottime_seconds();
+		if (age_sec >= uptime)
+			return len;
+
+		cutoff = uptime - age_sec;
 	}
 
 	guard(rwsem_read)(&zram->dev_lock);

Signature · 1 line

2.34.1

Sergey Senozhatsky

maintainer Reviewed-by on v2 1/1

2026-08-28 09:20 UTC #1 lore raw

Quoted 24 lines · Hao Jia

After commit 2e8ff2f51dde ("zram: use u32 for entry ac_time tracking"), idle_store() computes the idle cutoff as:
	cutoff = ktime_sub((u32)ktime_get_boottime_seconds(), age_sec);
Because the left operand is cast to u32, when age_sec exceeds the current uptime the subtraction wraps modulo 2^32 and the huge result is zero-extended into the s64 cutoff. mark_idle() then marks every entry as idle instead of matching nothing. For instance, running
	echo 86400 > /sys/block/zramX/idle
on a machine up for only two minutes marks all newly written pages idle and hands them to idle writeback and recompression.
No slot can have been accessed before the system booted, so an age_sec that reaches back past uptime cannot match any slot. Return early in that case, without walking the table or taking any slot locks.
Track the cutoff as time64_t rather than ktime_t. Both cutoff and ac_time are boot-time values in seconds, so a plain arithmetic comparison against ac_time in mark_idle() is correct and no ktime helpers are needed.

Quoted 4 lines

Fixes 2e8ff2f51dde ("zram: use u32 for entry ac_time tracking")
Cc [email protected]
Suggested-by Sergey Senozhatsky <[email protected]>
Signed-off-by Hao Jia <[email protected]>

Reviewed-by Sergey Senozhatsky <[email protected]>

Andrew Morton

2026-08-28 17:24 UTC #2 lore raw

In reply to Hao Jia

Quoted 15 lines · On Fri, 28 Aug 2026 16:31:49 +0800 Hao Jia <[email protected]> wrote:

After commit 2e8ff2f51dde ("zram: use u32 for entry ac_time tracking"), idle_store() computes the idle cutoff as:
	cutoff = ktime_sub((u32)ktime_get_boottime_seconds(), age_sec);
Because the left operand is cast to u32, when age_sec exceeds the current uptime the subtraction wraps modulo 2^32 and the huge result is zero-extended into the s64 cutoff. mark_idle() then marks every entry as idle instead of matching nothing. For instance, running
	echo 86400 > /sys/block/zramX/idle
on a machine up for only two minutes marks all newly written pages idle and hands them to idle writeback and recompression.

Quoted 8 lines

No slot can have been accessed before the system booted, so an age_sec that reaches back past uptime cannot match any slot. Return early in that case, without walking the table or taking any slot locks.
Track the cutoff as time64_t rather than ktime_t. Both cutoff and ac_time are boot-time values in seconds, so a plain arithmetic comparison against ac_time in mark_idle() is correct and no ktime helpers are needed.

Thanks. Sashiko asked a couple of questions about this change:

	https://sashiko.dev/#/patchset/[email protected]

Sergey Senozhatsky

maintainer

2026-08-31 04:15 UTC #3 lore raw

Quoted 1 line · Andrew Morton

On Fri, 28 Aug 2026 16:31:49 +0800 Hao Jia <[email protected]> wrote:

[..]

Quoted 8 lines

Thanks. Sashiko asked a couple of questions about this change:
	https://sashiko.dev/#/patchset/[email protected]
Does this early return prevent marking valid boot-time pages as idle? When a page is accessed during the first second of system boot, its ac_time would be 0.

There is no possibility for zram to hold pages during first second of system boot, regardless of whether zram was configured as a swap device or as a block device (mount-ed with real filesystem).

Quoted 1 line

Can the early return above bypass this zram device initialization check?

We already do that, e.g. when kstrtouint(buf, 0, &age_sec) fails. Apart from that, that's not how one checks if device was initialized. We may want to consolidate those checks, just for symmetry.

Hao Jia

author

2026-08-31 08:38 UTC #4 lore raw

Quoted 17 lines · Sergey Senozhatsky

Quoted 1 line · Andrew Morton
On Fri, 28 Aug 2026 16:31:49 +0800 Hao Jia <[email protected]> wrote:
[..]
Quoted 8 lines
Thanks. Sashiko asked a couple of questions about this change:
	https://sashiko.dev/#/patchset/[email protected]
Does this early return prevent marking valid boot-time pages as idle? When a page is accessed during the first second of system boot, its ac_time would be 0.
There is no possibility for zram to hold pages during first second of system boot, regardless of whether zram was configured as a swap device or as a block device (mount-ed with real filesystem).

Quoted 5 lines

Quoted 1 line
Can the early return above bypass this zram device initialization check?
We already do that, e.g. when kstrtouint(buf, 0, &age_sec) fails. Apart from that, that's not how one checks if device was initialized. We may want to consolidate those checks, just for symmetry.

Agreed on both points -- the early return is not a new "bypass", and the write() return value was never a way to probe init state.

How about moving the init_done() check to the top, so all the early returns sit behind the same device-state check?

static ssize_t idle_store(...)
{
struct zram *zram = dev_to_zram(dev);
time64_t cutoff = 0;

   guard(rwsem_read)(&zram->dev_lock);
   if (!init_done(zram))
           return -EINVAL;

   if (!sysfs_streq(buf, "all")) {
           ...
   }

mark_idle(zram, cutoff);
return len;
}

The read lock then also covers the parsing, but that is non-sleeping and its cost is negligible.

Thanks,
Hao

Sergey Senozhatsky

maintainer

2026-08-31 09:38 UTC #5 lore raw

Quoted 24 lines · Hao Jia

Quoted 17 lines · Sergey Senozhatsky
Quoted 1 line · Andrew Morton
On Fri, 28 Aug 2026 16:31:49 +0800 Hao Jia <[email protected]> wrote:
[..]
Quoted 8 lines
Thanks. Sashiko asked a couple of questions about this change:
	https://sashiko.dev/#/patchset/[email protected]
Does this early return prevent marking valid boot-time pages as idle? When a page is accessed during the first second of system boot, its ac_time would be 0.
There is no possibility for zram to hold pages during first second of system boot, regardless of whether zram was configured as a swap device or as a block device (mount-ed with real filesystem).
Quoted 5 lines
Quoted 1 line
Can the early return above bypass this zram device initialization check?
We already do that, e.g. when kstrtouint(buf, 0, &age_sec) fails. Apart from that, that's not how one checks if device was initialized. We may want to consolidate those checks, just for symmetry.

Quoted 5 lines

Agreed on both points -- the early return is not a new "bypass", and the write() return value was never a way to probe init state.
How about moving the init_done() check to the top, so all the early returns sit behind the same device-state check?

Yeah, I don't know... Moving it under device lock doesn't buy us anything. We don't need device lock to validate integer rangers, etc. There are validations that we need to do under device lock because those require a consistent device state. But things like "is system uptime less than supplied sysfs data" don't logically require a device lock.

Hao Jia

author

2026-09-01 02:02 UTC #6 lore raw

Quoted 31 lines · Sergey Senozhatsky

Quoted 24 lines · Hao Jia
Quoted 17 lines · Sergey Senozhatsky
Quoted 1 line · Andrew Morton
On Fri, 28 Aug 2026 16:31:49 +0800 Hao Jia <[email protected]> wrote:
[..]
Quoted 8 lines
Thanks. Sashiko asked a couple of questions about this change:
	https://sashiko.dev/#/patchset/[email protected]
Does this early return prevent marking valid boot-time pages as idle? When a page is accessed during the first second of system boot, its ac_time would be 0.
There is no possibility for zram to hold pages during first second of system boot, regardless of whether zram was configured as a swap device or as a block device (mount-ed with real filesystem).
Quoted 5 lines
Quoted 1 line
Can the early return above bypass this zram device initialization check?
We already do that, e.g. when kstrtouint(buf, 0, &age_sec) fails. Apart from that, that's not how one checks if device was initialized. We may want to consolidate those checks, just for symmetry.
Quoted 5 lines
Agreed on both points -- the early return is not a new "bypass", and the write() return value was never a way to probe init state.
How about moving the init_done() check to the top, so all the early returns sit behind the same device-state check?

Quoted 6 lines

Yeah, I don't know... Moving it under device lock doesn't buy us anything. We don't need device lock to validate integer rangers, etc. There are validations that we need to do under device lock because those require a consistent device state. But things like "is system uptime less than supplied sysfs data" don't logically require a device lock.

Thanks for the clarification. The current patch is enough, so no change is needed.

Thanks,
Hao