Re: [PATCH] mm/khugepaged: don't map folios that are not uptodate in MADV_COLLAPSE

[Pedro Falcato]

On Fri, Nov 06, 2026 at 01:29:31PM +0100, David Hildenbrand (Arm) wrote:
> On 11/6/26 13:09, Kiryl Shutsemau wrote:
> > On Thu, Nov 05, 2026 at 07:16:44PM +0100, David Hildenbrand (Arm) wrote:
> >> madvise(MADV_COLLAPSE) can map PMD-sized pagecache folios that are not
> >> uptodate. For example, shmem fallocate() can put such folios into the
> >> page cache.
> >>
> >> collapse_file() already refuses to proceed with folios that are not
> >> uptodate. Let's just do the same in try_collapse_pte_mapped_thp() and skip
> >> such folios, leaving handling to the page faults.
> >>
> >> Note that collapse_file() effectively clears shmem folios through
> >> shmem_get_folio(); let's keep this fix simple and simply let the page
> >> fault path deal with that.
> >>
> >> Fixes: 34488399fa08 ("mm/madvise: add file and shmem support to MADV_COLLAPSE")
> >> Reported-by: Chris Mason <mason@xxxxxxxxxx>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> > 
> > Reviewed-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
> > 
> > Separately from this fix: nothing on the mapping side catches this.
> > Every caller checks uptodate on its own (next_uptodate_folio(),
> > filemap_fault(), shmem_get_folio()), and khugepaged was the one that
> > went straight from filemap_lock_folio() to do_set_pmd().
> > 
> > Should do_set_pmd() and set_pte_range() assert it? Something like
> > 
> >         VM_WARN_ON_ONCE_FOLIO(folio->mapping && !folio_test_anon(folio) &&
> >                               !folio_test_uptodate(folio), folio);
> 
> I think so. But looking at these hard-to-trigger but in the end very nasty
> problems, should we actually reject the operation and do a pr_warn_once() or
> something?
> 

Yes. FWIW, this is something that's been going through my mind. Our page table
manipulation APIs (and usage) are way too fragile and allow you to do the
most insane things without ever realizing it (e.g you can clear a pte, set
over a PTE, get-and-clear and discard the result, and do all of the above with
no locks held, set a pte for a folio that's not uptodate, set a pte for a folio
for which you don't have the lock, etc).

Some basic assertions (even if there's no easy way to back down from those)
would be nice, or ideally yes a rejection.

I would just add that these very much should be VM_WARN_ON_ONCE instead of
pr_warn_once(). pr_warn_once() will (AIUI) not be caught by fuzzers, at least
not as easily.

Also, if they're inexpensive enough, we might want to upgrade them to full
WARN_ON_ONCE().

-- 
Pedro