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

On Fri, 17 Jul 2026 00:52:19 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Incorrect PTE advancement for large folios in vaddr stat scanning and migration.
> - [High] Commit of DAMON parameters corrupts the active scheme's quota state by zeroing dynamic fields in-place.
> - [High] Failure in `damos_commit_dests` leaves the active scheme in an inconsistent state, leading to a NULL pointer dereference.
> --
> 
> --- Patch [1]: [PATCH 1/7] mm/damon/core: initialize damos->last_applied ---
> commit 90344a381581ba5ad7a7c87826b18bf948659516
> Author: SJ Park <[email protected]>
> 
> mm/damon/core: initialize damos->last_applied
> 
> This commit fixes an issue where the damos->last_applied field was being used
> without initialization, leading to DAMON unexpectedly skipping the application
> of DAMOS actions for a folio the first time per scheme. It addresses this by
> initializing the field in damon_new_scheme().
> 
> 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 isn't a bug introduced by this patch, but does this in-place zeroing
> corrupt the active scheme's quota state?
> 
> Looking at damon_new_scheme(), it passes the provided quota pointer to
> damos_quota_init(), which zeros out dynamic fields directly:
> 
> mm/damon/core.c:damos_quota_init() {
>     ...
>     quota->esz = 0;
>     quota->total_charged_sz = 0;
>     ...
> }
> 
> When damon_commit_ctx() applies new parameters to an active context, it copies
> schemes by calling damon_new_scheme() with a pointer to the running context's
> scheme quota.
> 
> Could this wipe out the current quota charge window progress and reset the
> goal tuner's feedback loop (esz_bp) on every parameter commit, causing
> inconsistent tuning pressure?

No.  damon_new_scheme() is called only for new scheme addition.

> 
> 
> [Severity: High]
> This is a pre-existing issue, but can the failure path in damos_commit_dests()
> lead to a NULL pointer dereference?
> 
> In mm/damon/core.c:damos_commit_dests(), if memory allocation fails for
> dst->node_id_arr or dst->weight_arr, the function returns -ENOMEM:
> 
> mm/damon/core.c:damos_commit_dests() {
>     ...
>     dst->node_id_arr = kmalloc_array(src->nr_dests,
>         sizeof(*dst->node_id_arr), GFP_KERNEL);
>     if (!dst->node_id_arr) {
>         dst->weight_arr = NULL;
>         return -ENOMEM;
>     }
>     ...
> }
> 
> However, dst->nr_dests is updated later. If the allocation fails while
> updating an active context during __damon_commit_ctx(), could the active
> scheme be left with weight_arr == NULL but a non-zero nr_dests?
> 
> If so, would this cause a kernel panic when kdamond resumes and calls
> damos_va_migrate_dests_add(), which iterates up to dests->nr_dests and
> unconditionally dereferences dests->weight_arr[i]?

No.  If damon_commit_ctx() fails, kdamond stops due to 'maybe_corrupted' field.
There is no way to resume it.

> 
> 
> [Severity: High]
> This is a pre-existing issue, but does the PTE iteration in
> damos_va_stat_pmd_entry() and damos_va_migrate_pmd_entry() overshoot large
> folios?
> 
> In both functions, when a valid folio is found, the loop increment is set to
> the total number of pages in the folio:
> 
> mm/damon/vaddr.c:damos_va_stat_pmd_entry() {
>     ...
>     if (!damos_va_filter_out(s, folio, vma, addr, pte, NULL))
>         *sz_filter_passed += folio_size(folio);
>     nr = folio_nr_pages(folio);
>     s->last_applied = folio;
>     ...
> }
> 
> If the current PTE maps a tail page of a large folio (for example, if the VMA
> starts in the middle of a large folio), wouldn't advancing pte += nr and
> addr += nr * PAGE_SIZE overshoot the end of the large folio?
> 
> Could this cause DAMON to skip subsequent PTEs in the mapped page table that
> might belong to entirely different folios, missing stats and migration actions
> for them?

Known low priority issue.

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


Thanks,
SJ