Re: [RFC PATCH 5/9] mm/damon/vaddr: respect folio end for DAMOS_STAT

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Direct dereference of PTE pointer `*pte` instead of using the local safely fetched `ptent`.

Pre-existing issues:
- [High] Batched PTE advancement blindly skips interleaved folios and PTE-level Accessed/Young bits within a large folio's virtual footprint.
- [High] The exact page iteration bug fixed in `damos_va_stat_pmd_entry()` is also present in `damos_va_migrate_pmd_entry()` but was missed by this patch.
--

--- Patch [5]: [RFC PATCH 5/9] mm/damon/vaddr: respect folio end for DAMOS_STAT ---
commit 20f53909d4e28dc2c1b2dd4ce52890349fc6c85d
Author: SJ Park <sj@kernel.org>

mm/damon/vaddr: respect folio end for DAMOS_STAT

For applying DAMOS_STAT action to a region, DAMON virtual address space
operation set (vaddr) calls walk_page_range[_vma]() for the region.  The
pmd walk entry function, namely damon_va_stat_pmd_entry(), applies DAMOS
filters to folios of addresses of the region in the pmd.  It starts from
the walking address and advances the address by the size of the folio of
the address until it goes out of the pmd or the region.

Link: https://lore.kernel.org/20260514015053.149396-1-sj@kernel.org

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 0648400b2d65b..035cd509f6b58 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -831,6 +831,8 @@ static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
>  		return 0;
>  
>  	for (; addr < next; pte += nr, addr += nr * PAGE_SIZE) {
> +		unsigned long page_idx;
> +
>  		nr = 1;
>  		ptent = ptep_get(pte);
>  
> @@ -844,7 +846,8 @@ static int damos_va_stat_pmd_entry(pmd_t *pmd, unsigned long addr,
>  
>  		if (!damos_va_filter_out(s, folio, vma, addr, pte, NULL))
>  			*sz_filter_passed += folio_size(folio);
> -		nr = folio_nr_pages(folio);
> +		page_idx = folio_page_idx(folio, pte_page(*pte));

[Severity: Medium]
Is it safe to directly dereference *pte here instead of using the local
safely fetched ptent?

Reading a PTE directly on 32-bit architectures (like x86 PAE) can result in
a torn read if the hardware concurrently updates the Accessed/Dirty bits,
potentially leading to an incorrect PFN and subsequent wild pointer
arithmetic.

The local snapshot ptent was already obtained via ptep_get(pte) and should
likely be used here.

> +		nr = folio_nr_pages(folio) - page_idx;

[Severity: High]
This is a pre-existing issue, but does this batched PTE advancement blindly
skip interleaved folios and PTE-level Accessed/Young bits within a large
folio's virtual footprint?

For PTE-mapped large folios, hardware tracks the Accessed bit independently
per 4K PTE. By checking only the first PTE and blindly skipping the rest,
DAMON ignores accesses to subsequent pages of the folio, potentially
classifying hot memory as cold.

Furthermore, if userspace partially unmaps or interleaves different mappings,
the blind pte += nr skips those unrelated folios completely without
verifying they map the same folio.

>  		s->last_applied = folio;
>  	}
>  	pte_unmap_unlock(start_pte, ptl);

While reviewing this patch, I noticed the exact page iteration bug fixed in
damos_va_stat_pmd_entry() is also present in damos_va_migrate_pmd_entry()
but was missed by this patch.

[Severity: High]
This is a pre-existing issue, but does the DAMOS migrate action also advance
the PTE pointer unconditionally by the full size of the large folio?

mm/damon/vaddr.c:damos_va_migrate_pmd_entry() {
	...
	for (; addr < next; pte += nr, addr += nr * PAGE_SIZE) {
		...
		damos_va_migrate_dests_add(folio, walk->vma, addr, dests,
				migration_lists);
		nr = folio_nr_pages(folio);
	}
	...
}

If the scanning starts in the middle of a large folio, it will advance past
the end of the large folio and skip unrelated folios that follow it, failing
to migrate them.

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