Re: [PATCH 1/4] mm/damon: defensively skip zone device folios in damon_get_folio()

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

New issues:
- [Medium] The explicit `folio_is_zone_device()` check added by the patch is dead code and fails to achieve its stated intent.

Pre-existing issues:
- [High] Early return in damon_ptep_mkold and damon_pmdp_mkold skips MMU notifier aging for device-exclusive swap PTEs.
- [High] The scheme's `last_applied` pointer caches a freed folio pointer, leading to a dangling pointer comparison.
--

--- Patch [1]: [PATCH 1/4] mm/damon: defensively skip zone device folios in damon_get_folio() ---
commit 8181b2e77c1b49bcb808528f343f72fdbf9d168c
Author: Gregory Price <[email protected]>

mm/damon: defensively skip zone device folios in damon_get_folio()

All DAMON physical- and virtual-address operations obtain their folios
through damon_get_folio(). That helper already excludes ZONE_DEVICE
memory implicitly via pfn_to_online_page() and folio_test_lru(), but
this is inconsistent with other callers in mm/ which test explicitly.

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index 0981a5e3af172..0db1383de5d01 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -32,7 +32,8 @@ struct folio *damon_get_folio(unsigned long pfn)
>  	folio = page_folio(page);
>  	if (!folio_try_get(folio))
>  		return NULL;
> -	if (unlikely(page_folio(page) != folio) || !folio_test_lru(folio)) {
> +	if (unlikely(page_folio(page) != folio) || !folio_test_lru(folio) ||
> +	    folio_is_zone_device(folio)) {

[Severity: Medium]
Since ZONE_DEVICE folios do not have the LRU flag set, won't the
!folio_test_lru(folio) check always evaluate to true for them?

Additionally, earlier in damon_get_folio(), pfn_to_online_page(pfn) is
called, which strictly returns NULL for ZONE_DEVICE memory. Doesn't this
mean ZONE_DEVICE memory will never reach this new folio_is_zone_device()
check at all, making it dead code?

>  		folio_put(folio);
>  		folio = NULL;
>  	}

[Severity: High]
This is a pre-existing issue, but I have a question about how
damon_get_folio() returning NULL interacts with device-exclusive memory.

When DAMON encounters a device-exclusive PFN swap PTE in damon_ptep_mkold()
or damon_pmdp_mkold(), damon_get_folio() returns NULL because
pfn_to_online_page() rejects it. Both of these callers then immediately
early-return on !folio:

mm/damon/ops-common.c:damon_ptep_mkold() {
    ...
    folio = damon_get_folio(pfn);
    if (!folio)
        return;
    ...
}

Doesn't this early return silently bypass the mmu_notifier_clear_young()
call completely, breaking DAMON's ability to monitor device accesses
(which relies on the MMU notifier for device-exclusive PTEs)?

[Severity: High]
This isn't a bug introduced by this patch, but looking at how DAMOS
physical address apply functions interact with folios, I noticed
something about the last_applied pointer:

In mm/damon/paddr.c:damon_pa_pageout(), a reference is dropped:

mm/damon/paddr.c:damon_pa_pageout() {
    ...
    folio_put(folio);
    ...
    s->last_applied = folio;
}

Since folio_put() drops the reference, could this store a dangling
pointer across DAMON region processing boundaries?

If that physical memory is subsequently reallocated for a new folio,
it will reside at the exact same address. When this pointer is later
checked in damon_pa_invalid_damos_folio():

mm/damon/paddr.c:damon_pa_invalid_damos_folio() {
    ...
    if (folio == s->last_applied) {
        folio_put(folio);
        return true;
    }
    ...
}

Could this falsely match the dangling pointer and erroneously skip a
valid, newly allocated folio from DAMOS actions?

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