linux-kernel - Re: [PATCH] mm/damon/stat: set last_refresh

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251028143250.50144-1-sj@kernel.org>
Date: Tue, 28 Oct 2025 07:32:49 -0700
From: SeongJae Park <sj@...nel.org>
To: SeongJae Park <sj@...nel.org>
Cc: Quanmin Yan <yanquanmin1@...wei.com>,
	akpm@...ux-foundation.org,
	damon@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	wangkefeng.wang@...wei.com,
	zuoze1@...wei.com
Subject: Re: [PATCH] mm/damon/stat: set last_refresh_jiffies to jiffies at startup

On Tue, 28 Oct 2025 07:19:14 -0700 SeongJae Park <sj@...nel.org> wrote:

> On Tue, 28 Oct 2025 14:19:27 +0800 Quanmin Yan <yanquanmin1@...wei.com> wrote:
> 
> > In DAMON_STAT's damon_stat_damon_call_fn(), time_before_eq() is used to
> > avoid unnecessarily frequent stat update.
> > 
> > On 32-bit systems, the kernel initializes jiffies to "-5 minutes" to make
> > jiffies wrap bugs appear earlier. However, this causes time_before_eq()
> > in DAMON_STAT to unexpectedly return true during the first 5 minutes
> > after boot on 32-bit systems (see [1] for more explanation, which fixes
> > another jiffies-related issue in DAMON). As a result, DAMON_STAT does not
> > update any monitoring results during that period, which can be more
> > confusing when DAMON_STAT_ENABLED_DEFAULT is enabled.
> > 
> > Fix it by setting last_refresh_jiffies to jiffies at startup.
> 
> Nice catch, thank you for this patch!
> 
> > 
> > [1] https://lkml.kernel.org/r/20250822025057.1740854-1-ekffu200098@gmail.com
> > 
> > Fixes: fabdd1e911da ("mm/damon/stat: calculate and expose estimated memory bandwidth")
> > Signed-off-by: Quanmin Yan <yanquanmin1@...wei.com>
> > ---
> >  mm/damon/stat.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/mm/damon/stat.c b/mm/damon/stat.c
> > index 6c4503d2aee3..6dc3e18de910 100644
> > --- a/mm/damon/stat.c
> > +++ b/mm/damon/stat.c
> > @@ -132,6 +132,9 @@ static int damon_stat_damon_call_fn(void *data)
> >  	struct damon_ctx *c = data;
> >  	static unsigned long last_refresh_jiffies;
> >  
> > +	if (unlikely(!last_refresh_jiffies))
> > +		last_refresh_jiffies = jiffies;
> > +
> 
> How about doing the initialization together with the declaration?  E.g.,
> 
>  static int damon_stat_damon_call_fn(void *data)
>  {
>         struct damon_ctx *c = data;
> -       static unsigned long last_refresh_jiffies;
> +       static unsigned long last_refresh_jiffies = jiffies;

Actually, a similar issue can happen again if DAMON_STAT is stopped and
restarted by user.  That is, if user stops DAMON_STAT just after
last_refresh_jiffies is updated, and restart it after 5 seconds or more, the
time_before_eq() on damon_call_fn() will return true, so stat updates will
happen earlier than expected.  Shouldn't be a real problem, but better to avoid
if possible.

How about making last_refresh_jiffies a global variable and initialize it on
damon_stat_start()?  To avoid unnecessary name conflicts, the variable name
would also better to be changed, e.g., damon_stat_last_refresh_jiffies.


Thanks,
SJ

[...]