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

[Sergey Senozhatsky]

On (26/08/28 16:38), Hao Jia wrote:
> On 2026/8/28 14:02, Sergey Senozhatsky wrote:
> > 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.
> 
> Thanks for the feedback! I've sent out v2 of the patch. Please take a look
> when you get a chance.
> 
> https://lore.kernel.org/all/20260828083149.45760-1-jiahao.kernel@xxxxxxxxx

Thanks, looks good.