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

[sashiko-bot@kernel.org]

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?

>  	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?

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