Re: [PATCH v3] mm/damon/ops-common: putback folios on invalid migrate nid

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Invalid address and PTE advancement after page-to-folio conversion for large folios in DAMON scanning loops.
- [High] DAMON migration causes NR_ISOLATED_ANON and NR_ISOLATED_FILE counter underflow.
--

--- Patch [1]: [PATCH v3] mm/damon/ops-common: putback folios on invalid migrate nid ---
commit 1f243815b710aba6f1b966b6f98b83cb7c51edeb
Author: liyouhong <[email protected]>

mm/damon/ops-common: putback folios on invalid migrate nid

damon_pa_migrate() and damos_va_migrate() isolate folios into a local list
and then call damon_migrate_pages().  When target_nid is invalid (including
the scheme default NUMA_NO_NODE / -1), damon_migrate_pages() returns early
without putting the folios back to the LRU.

Callers then discard the list head while those folios remain isolated with
an extra reference taken by folio_isolate_lru().  The pages stay off the
LRU for as long as the mapping exists (anon active+inactive counts drop
while RSS does not), and the leftover references can pin the pages after
the mapping is gone.

Put the folios back on the invalid-nid path so ignored migration requests
still return them to the LRU.

Fixes: 7e6c3130690a ("mm/damon/ops-common: ignore migration request to invalid nodes")
Cc: <[email protected]>
Assisted-by: Cursor:grok-4.5
Reviewed-by: SJ Park <[email protected]>
Signed-off-by: liyouhong <[email protected]>

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index e59f77eca83b2..0981a5e3af172 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
[ ... ]

While reviewing this migration fix, I noticed a couple of pre-existing issues
in the DAMON migration code:

mm/damon/vaddr.c:damos_va_migrate_pmd_entry() {
[ ... ]
	for (; addr < next; pte += nr, addr += nr * PAGE_SIZE) {
		nr = 1;
		ptent = ptep_get(pte);

		if (pte_none(ptent) || !pte_present(ptent))
			continue;
		folio = vm_normal_folio(walk->vma, addr, ptent);
		if (!folio)
			continue;
		if (damos_va_filter_out(s, folio, walk->vma, addr, pte, NULL))
			continue;
		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
				migration_lists);
		nr = folio_nr_pages(folio);
	}
[ ... ]
}

[Severity: High]
This isn't a bug introduced by this patch, but does this iteration overshoot
the end of large folios?

If the current PTE corresponds to a tail page (meaning we are at an offset
into the folio), adding the full folio_nr_pages() to both pte and addr seems
like it will advance past the end of the folio, skipping subsequent, unrelated
memory.

This same pattern appears in damos_va_stat_pmd_entry(), damon_pa_migrate(),
and damon_pa_stat().


mm/damon/vaddr.c:damos_va_migrate_dests_add() {
[ ... ]
isolate:
	if (!folio_isolate_lru(folio))
		return;

	list_add(&folio->lru, &migration_lists[i]);
}

[Severity: High]
This is a pre-existing issue, but does this isolation miss incrementing the
isolated VM stats?

Folios are isolated here and added to the migration list, but I don't see
NR_ISOLATED_ANON or NR_ISOLATED_FILE being updated.

Later, when the core migrate_pages() code processes these folios via
damon_migrate_pages():

mm/migrate.c:migrate_folio_done() {
[ ... ]
	if (likely(!page_has_movable_ops(&src->page)) && reason != MR_DEMOTION)
		mod_node_page_state(folio_pgdat(src), NR_ISOLATED_ANON +
				    folio_is_file_lru(src), -folio_nr_pages(src));
[ ... ]
}

Will this decrement cause the node page state counters to underflow?

Since the counters weren't incremented originally during DAMON's isolation, it
looks like a successful migration will drop the isolated stat and wrap it
around. This could hang memory reclaim since too_many_isolated() might
perpetually return true.

This also seems to apply to damon_pa_migrate().

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