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

[Sergey Senozhatsky]

On (26/08/28 13:23), Sergey Senozhatsky wrote:
> 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;

Or perhaps, instead "return len" and make it a fast path out, w/o the
need to iterate any slots, take locks, etc.

> +
> +             cutoff = uptime - age_sec;
>       }