Re: [PATCH 3/6] mm/damon/paddr: support hugetlb folios in access monitoring

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Non-atomic huge PTE modification in damon_hugetlb_mkold() causes a TOCTOU race with hardware page table walkers, silently dropping dirty bits.
- [High] Incorrect physical address advancement when starting from a tail page of a large folio.
--

commit fa7cc985c2891991281afce141ed40264af354fe
Author: Krishna Iyer <kiyer@crusoe.ai>

    mm/damon/paddr: support hugetlb folios in access monitoring
    
    DAMON's physical address space monitoring is blind to hugetlb-backed
    memory. Every access check starts at damon_get_folio(), which rejects
    folios that are not on the LRU lists. This commit adds
    damon_get_folio_incl_hugetlb() to additionally accept hugetlb folios.

[ ... ]

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index 62004206ca310..ece101d346844 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does this non-atomic huge PTE modification
drop hardware dirty bits?

In damon_hugetlb_mkold() in mm/damon/ops-common.c:

	pte_t entry = huge_ptep_get(mm, addr, pte);
	struct folio *folio = pfn_folio(pte_pfn(entry));
	unsigned long psize = huge_page_size(hstate_vma(vma));

	folio_get(folio);

	if (pte_young(entry)) {
		referenced = true;
		entry = pte_mkold(entry);
		set_huge_pte_at(mm, addr, pte, entry, psize);
	}

If a hardware page table walker sets the dirty bit concurrently between the
read and the write, does the non-atomic writeback via set_huge_pte_at()
silently overwrite and clear the dirty bit?

[Severity: High]
This isn't a bug introduced by this patch, but does this physical address
advancement skip memory pages?

In damon_pa_pageout() in mm/damon/paddr.c:

	addr = damon_pa_phys_addr(r->ar.start, addr_unit);
	while (addr < damon_pa_phys_addr(r->ar.end, addr_unit)) {
		folio = damon_get_folio(PHYS_PFN(addr));
...
put_folio:
		addr += folio_size(folio);
		folio_put(folio);
	}

If the region's start address is not naturally aligned to the folio boundary
and lands on a tail page of a large folio, does adding the full folio size
overshoot the start of the next folio?

The same pattern appears in damon_pa_stat() where it also over-accounts the
filter size:

		if (damos_pa_filter_out(s, folio))
			goto put_folio;
		else
			*sz_filter_passed += folio_size(folio) / addr_unit;
...
put_folio:
		addr += folio_size(folio);
		folio_put(folio);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830051407.50008-1-kiyer@crusoe.ai?part=3