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

On Tue, 28 Jul 2026 19:57:04 +0000 [email protected] wrote:

> 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?

This is what Gregory already mentioned about in the commit message.  I added a
question about this, too, though.  Let's keep discussing in the main thread.

> 
> >  		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)?

This is an intended behavior.

> 
> [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?

I agree it is not super ideal, but missing one access event from DAMON's
best-effort monitoring is no problem.

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


Thanks,
SJ