Re: [RFC PATCH 3/6] mm/damon/vaddr: implement sz_filter_passed for migrate_{hot,cold} action

[SJ Park <sj@kernel.org>]

On Sat, 31 Oct 2026 20:36:41 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] mm/damon/vaddr: uninitialized priv.sz_filter_passed in damos_va_migrate()
> - [Medium] mm/damon/vaddr: double-counted sz_filter_passed in damos_va_migrate_pmd_entry()
> --
> 
> commit 872d2945863aba3a6ac672128498f5d65dbc8d40
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/vaddr: implement sz_filter_passed for migrate_{hot,cold} action
> 
> This commit adds sz_filter_passed accounting to the DAMON virtual address
> space implementation of the migrate_hot and migrate_cold DAMOS actions by
> accumulating folio sizes in damos_va_migrate_pmd_entry().
> 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index c271b6d63c027..112095c2d53fd 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -776,6 +776,7 @@ static bool damos_va_filter_out(struct damos *scheme, struct folio *folio,
> >  struct damos_va_migrate_private {
> >  	struct list_head *migration_lists;
> >  	struct damos *scheme;
> > +	unsigned long *sz_filter_passed;
> >  };
> >  
> >  /*
> > @@ -840,6 +841,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
> >  	struct damos_va_migrate_private *priv = walk->private;
> >  	struct list_head *migration_lists = priv->migration_lists;
> >  	struct damos *s = priv->scheme;
> > +	unsigned long *sz_filter_passed = priv->sz_filter_passed;
> 
> [Severity: Critical]
> Is priv.sz_filter_passed ever initialized before the page walk runs?
> 
> In damos_va_migrate(), struct damos_va_migrate_private priv is declared on
> the stack without an initializer, and only priv.scheme and
> priv.migration_lists are set before calling damon_va_walk_page_range():
> 
> mm/damon/vaddr.c:damos_va_migrate() {
> 	...
> 	struct damos_va_migrate_private priv;
> 	...
> 	priv.scheme = s;
> 	priv.migration_lists = kmalloc_objs(*priv.migration_lists, nr_dests);
> 	if (!priv.migration_lists)
> 		return 0;
> 
> 	for (int i = 0; i < nr_dests; i++)
> 		INIT_LIST_HEAD(&priv.migration_lists[i]);
> 
> 
> 	mm = damon_get_mm(target);
> 	if (!mm)
> 		goto free_lists;
> 
> 	damon_va_walk_page_range(mm, r->ar.start, r->ar.end, &walk_ops, &priv);
> 	...
> }
> 
> When damon_va_apply_scheme() invokes damos_va_migrate() for DAMOS_MIGRATE_HOT
> or DAMOS_MIGRATE_COLD and damos_va_migrate_pmd_entry() encounters a present
> folio that passes damos_va_filter_out(), won't *sz_filter_passed +=
> folio_size(folio) dereference an uninitialized stack pointer?

Nice finding.  I will set the field in damos_va_migrate(), to sz_filter_passed
parameter it received.

> 
> >  	struct damos_migrate_dests *dests = &s->migrate_dests;
> >  	struct folio *folio;
> >  	spinlock_t *ptl;
> > @@ -858,6 +860,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
> >  			goto huge_out;
> >  		if (damos_va_filter_out(s, folio, walk->vma, addr, NULL, pmd))
> >  			goto huge_out;
> > +		*sz_filter_passed += folio_size(folio);
> >  		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
> >  				migration_lists);
> >  huge_out:
> > @@ -885,6 +888,7 @@ static int damos_va_migrate_pmd_entry(pmd_t *pmd, unsigned long addr,
> >  		nr = folio_nr_pages(folio) - page_idx;
> >  		if (damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL))
> >  			continue;
> > +		*sz_filter_passed += folio_size(folio);
> 
> [Severity: Medium]
> Can *sz_filter_passed double-count a large folio that spans across multiple
> adjacent damon_region ranges?
> 
> In damos_va_stat_pmd_entry(), the walk checks damos_va_invalid_folio(folio, s)
> and records s->last_applied = folio so that a PMD-mapped THP or PTE-mapped
> large folio overlapping more than one damon_region is only counted once per
> scheme application pass:
> 
> mm/damon/vaddr.c:damos_va_stat_pmd_entry() {
> 		...
> 		if (damos_va_invalid_folio(folio, s))
> 			goto huge_unlock;
> 
> 		if (!damos_va_filter_out(s, folio, vma, addr, NULL, pmd))
> 			*sz_filter_passed += folio_size(folio);
> 		s->last_applied = folio;
> 		...
> }
> 
> In damos_va_migrate_pmd_entry(), s->last_applied is neither checked nor
> updated on either the THP path or the PTE loop path. If a large folio spans
> multiple regions and is not migrated during the first region's walk (for
> example, because folio_nid(folio) already matches the target node in
> damos_va_migrate_dests_add(), or because isolation or migration fails),
> walking the next overlapping damon_region will encounter the same folio and
> add the full folio_size(folio) to *sz_filter_passed again.
> 
> Should damos_va_migrate_pmd_entry() also check damos_va_invalid_folio(folio,
> s) and update s->last_applied = folio?

Good finding.  But this is a pre-existing issue.  I think it is better to be
fixed first, before this patch, though.  I will add the fix to this series, or
post it as a separate one before this series is merged.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261031202614.96036-1-sj@kernel.org?part=3
> 


Thanks,
SJ