Re: [RFC PATCH] mm/damon: adjust isolated pages stat for DAMOS_MIGRATE_{HOT,COLD}

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

Pre-existing issues:
- [High] Early return in damon_migrate_pages() leaves isolated folios on the caller's list, causing memory leaks, stat inflation, and stack UAF.
--

--- Patch [1]: [RFC PATCH] mm/damon: adjust isolated pages stat for DAMOS_MIGRATE_{HOT,COLD} ---
commit 1efec5dea2b48b50043eba6bdae1821a99318049
Author: SJ Park <[email protected]>

mm/damon: adjust isolated pages stat for DAMOS_MIGRATE_{HOT,COLD}

Callers of migrate_pages() should adjust NR_MIGRATED_{ANON,FILE} for
isolations and putback of the folios. This patch fixes DAMOS_MIGRATE_{HOT,COLD}
which mistakenly missed the stat adjustment, potentially confusing compaction
and reclaim.

Link: https://github.com/damonitor/damo
Link: https://lore.kernel.org/[email protected]

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index e59f77eca83b2..9809fa12b09a8 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c

[ ... ]

> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 5a6a78054784a..5c6c3a597fd0b 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -346,6 +346,8 @@ static unsigned long damon_pa_migrate(struct damon_region *r,
>  
>  		if (!folio_isolate_lru(folio))
>  			goto put_folio;
> +		node_stat_add_folio(folio, NR_ISOLATED_ANON +
> +				folio_is_file_lru(folio));
>  		list_add(&folio->lru, &folio_list);

[Severity: High]
This isn't a bug introduced by this patch, but if s->target_nid is invalid
(e.g., if it defaults to NUMA_NO_NODE), damon_migrate_pages() returns early
without draining the folio list:

mm/damon/ops-common.c:damon_migrate_pages() {
	if (target_nid < 0 || target_nid >= MAX_NUMNODES ||
			!node_state(target_nid, N_MEMORY))
		return nr_migrated;
    ...
}

Since folio_list is a stack variable here in damon_pa_migrate() and is passed
to damon_migrate_pages() later in this function, wouldn't this early return
permanently leak the isolated folios and cause a use-after-free when the stack
variable goes out of scope? 

Furthermore, with this patch applied, doesn't this error path also permanently
inflate the newly added NR_ISOLATED stats because the cleanup loop in
damon_migrate_folio_list() is never reached?

A similar issue appears to exist in mm/damon/vaddr.c:damos_va_migrate(),
where priv.migration_lists is freed without checking if any folios were left
behind by an early return.

>  put_folio:
>  		addr += folio_size(folio);

[ ... ]

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