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

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.

[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);
    ...
}

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?

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