Re: [PATCH v2 1/3] mm/damon: move damon_hugetlb_mkold() from vaddr to ops-common

[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 read-modify-write of huge PTE in damon_hugetlb_mkold clobbers hardware-updated dirty bits.
- [High] Unaligned addresses in DAMON mkold functions cause incorrect secondary MMU aging.
--

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

mm/damon: move damon_hugetlb_mkold() from vaddr to ops-common

This commit moves the damon_hugetlb_mkold function from the virtual address
space monitoring operations set to the common operations set. The physical
address space monitoring operations set will now be able to use it to
support access monitoring of hugetlb-backed memory.

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index fbda70d8ea4d0..f5fe92b825bbc 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
[ ... ]
> @@ -98,6 +99,35 @@ void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>  }
>  
> +#ifdef CONFIG_HUGETLB_PAGE
> +void damon_hugetlb_mkold(pte_t *pte, struct mm_struct *mm,
> +		struct vm_area_struct *vma, unsigned long addr)
> +{
> +	bool referenced = false;
> +	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);
> +	}

[Severity: Critical]
This is a pre-existing issue, but does this non-atomic sequence overwrite
the PTE and silently clobber concurrent hardware updates to the dirty bit?

In damon_hugetlb_mkold(), the entry is read via huge_ptep_get(), manually
cleared of the young bit with pte_mkold(), and unconditionally written back
with set_huge_pte_at(). A hardware update to the dirty bit occurring between
the read and the write could be lost. If the dirty bit is overwritten with a
stale clean copy, data could be reclaimed and lost instead of being written
back.

> +
> +	if (mmu_notifier_clear_young(mm, addr,
> +				     addr + huge_page_size(hstate_vma(vma))))

[Severity: High]
This is a pre-existing issue, but could passing an unaligned addr to
mmu_notifier_clear_young() cause incorrect secondary MMU aging?

When mmu_notifier_clear_young() receives a range that crosses page
boundaries due to an unaligned start address, the secondary MMU (such as
KVM) may age multiple adjacent host pages instead of just the target page.
This would corrupt working set statistics for adjacent pages and lead to
incorrect memory management, like reclaiming hot pages.

This unaligned addr behavior appears to originate from damon_rand() and is
also present in damon_ptep_mkold() and damon_pmdp_mkold().

> +		referenced = true;
> +
> +	if (referenced)
> +		folio_set_young(folio);
> +
> +	folio_set_idle(folio);
> +	folio_put(folio);
> +}
> +#endif	/* CONFIG_HUGETLB_PAGE */

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