Re: [PATCH] zram: fix idle age_sec underflow in idle_store()

[Sergey Senozhatsky <senozhatsky@chromium.org>]

Adding Brian.

On (26/08/25 17:23), Hao Jia wrote:
> After commit 2e8ff2f51dde ("zram: use u32 for entry ac_time tracking"),
> idle_store() computes the idle cutoff as follows:
> 
> 	cutoff = ktime_sub((u32)ktime_get_boottime_seconds(), age_sec);
> 
> Because the left operand is cast to a 32-bit unsigned integer, 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()
> subsequently marks every entry as idle instead of matching nothing,
> breaking the intended semantics of /sys/block/zramX/idle.  For instance,
> running echo 86400 > idle on a machine up for only two minutes causes
> all newly written pages to be marked as idle and handed over to idle
> writeback and recompression.
> 
> Drop the explicit cast to perform the subtraction in signed arithmetic
> again, restoring the previous behavior: an age_sec greater than uptime
> yields a negative cutoff, and one equal to uptime yields 0. Since a
> cutoff of 0 is now a valid computed value, it can no longer double as
> the "all" sentinel. Switch to KTIME_MIN instead, which no ac_time can
> be after.

Can we instead just reject such cases?   It should be an error to
request writeback of pages that are 2y old on a system that has
uptime of 2d.

Maybe something like this:

+             time64_t uptime;

-             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 -ERANGE;
+
+             cutoff = uptime - age_sec;
      }

We are probably fine doing just basic arithmetics operations on those two.
One is u32 the other one is s64: we should be fine just doing >= and sub.
I guess we also can use plain comparison
	cutoff > zram->table[index].attr.ac_time
in mark_idle().  Or am I hallucinating?

[keeping the rest of the original patch]

> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index a9b3bb1d3bef..da51f70b7121 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -438,7 +438,7 @@ static void mark_idle(struct zram *zram, ktime_t cutoff)
>  		}
>  
>  #ifdef CONFIG_ZRAM_TRACK_ENTRY_ACTIME
> -		is_idle = !cutoff ||
> +		is_idle = cutoff == KTIME_MIN ||
>  			ktime_after(cutoff, zram->table[index].attr.ac_time);
>  #endif
>  		if (is_idle)
> @@ -453,7 +453,7 @@ 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;
> +	ktime_t cutoff = KTIME_MIN;
>  
>  	if (!sysfs_streq(buf, "all")) {
>  		/*
> @@ -464,7 +464,7 @@ static ssize_t idle_store(struct device *dev, struct device_attribute *attr,
>  
>  		if (IS_ENABLED(CONFIG_ZRAM_TRACK_ENTRY_ACTIME) &&
>  		    !kstrtouint(buf, 0, &age_sec))
> -			cutoff = ktime_sub((u32)ktime_get_boottime_seconds(),
> +			cutoff = ktime_sub(ktime_get_boottime_seconds(),
>  					   age_sec);
>  		else
>  			return -EINVAL;
> @@ -475,7 +475,7 @@ static ssize_t idle_store(struct device *dev, struct device_attribute *attr,
>  		return -EINVAL;
>  
>  	/*
> -	 * A cutoff of 0 marks everything as idle, this is the
> +	 * A cutoff of KTIME_MIN marks everything as idle, this is the
>  	 * "all" behavior.
>  	 */
>  	mark_idle(zram, cutoff);
> -- 
> 2.34.1
>