Re: [PATCH 1/7] mm/damon/core: initialize damos->last_applied

On Sat, 18 Jul 2026 00:32:20 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Uninitialized memory use in DAMOS quota goals allocation (last_psi_total and nid).
> - [High] damos_quota_init() modifies the passed quota struct, unintentionally resetting runtime quota statistics of actively running schemes.
> --
> 
> --- Patch [1]: [PATCH 1/7] mm/damon/core: initialize damos->last_applied ---
> commit 316d27ac2ea17607b5c9df656020658fa1b425df
> Author: SJ Park <[email protected]>
> 
> mm/damon/core: initialize damos->last_applied
> 
> This commit initializes the last_applied field of the damos scheme struct to
> NULL in damon_new_scheme(). This prevents an uninitialized value from causing
> a DAMON scheme to unexpectedly skip applying an action to a folio the very
> first time.
> 
> Link: https://lore.kernel.org/[email protected]
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 603b102ff80f9..20d267c615faf 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -705,6 +705,7 @@ struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
> >  	INIT_LIST_HEAD(&scheme->ops_filters);
> >  	scheme->stat = (struct damos_stat){};
> >  	scheme->max_nr_snapshots = 0;
> > +	scheme->last_applied = NULL;
> >  	INIT_LIST_HEAD(&scheme->list);
> >  
> >  	scheme->quota = *(damos_quota_init(quota));
> 
> [Severity: High]
> This is a pre-existing issue, but does damos_quota_init() unintentionally reset 
> the runtime statistics of actively running schemes?
> 
> When updating a DAMON context via sysfs, damon_commit_ctx() calls 
> damon_new_test_ctx() which iterates over running schemes and calls 
> damon_new_scheme(), passing the active &src_scheme->quota pointer.
> 
> Inside damon_new_scheme(), it executes:
> scheme->quota = *(damos_quota_init(quota));
> 
> If damos_quota_init() directly modifies the passed pointer:
> 
> mm/damon/core.c:damos_quota_init() {
>     quota->esz = 0;
>     quota->total_charged_sz = 0;
>     quota->total_charged_ns = 0;
>     quota->charged_sz = 0;
>     ...
> }
> 
> It appears this silently clears the tracking variables of the actively running 
> scheme, potentially breaking the ability to track memory usage limits.

No.  The damon_new_scheme() is for a case the commit is adding new scheme to
the destination context.  There is no data to erase.

> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing initialization,
> I noticed a potential issue with quota goals. Are the metric-specific fields in
> damos_new_quota_goal() left uninitialized?
> 
> When allocating a new goal via sysfs or damon_commit_ctx():
> 
> mm/damon/core.c:damos_new_quota_goal() {
>     ...
>     goal = kmalloc_obj(*goal);
>     ...
> }
> 
> It seems fields like last_psi_total and nid are not zero-initialized.
> 
> Later, damos_commit_quota_goal_union() misses the switch case for 
> DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP, leaving dst->nid uninitialized.
> 
> This can result in using garbage values when calculating current_value:
> 
> mm/damon/core.c:damos_set_quota_goal_current_value() {
>     ...
>     goal->current_value = damos_get_node_eligible_mem_bp(c, s, goal->nid);
>     ...
> }

This is no issue.  The caller should set the nid.  And core functions using the
nid validates it before really using it.

> 
> And for PSI goals:
> 
> mm/damon/core.c:damos_set_quota_goal_current_value() {
>     ...
>     goal->current_value = now_psi_total - goal->last_psi_total;
>     goal->last_psi_total = now_psi_total;
>     ...
> }
> 
> Could subtracting an uninitialized last_psi_total underflow and cause the 
> quota tuner to aggressively zero out the quota?

Good finding.  I will separately fix this.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1


Thanks,
SJ