Re: [PATCH] mm/swap: fix swap_cache_get_folio() returning a folio that left the swap cache

[Jiayuan Chen]

Hi Andrew,

On 10/16/26 10:06 AM, Andrew Morton wrote:
> On Thu, 15 Oct 2026 14:05:27 +0800 Jiayuan Chen <jiayuan.chen@xxxxxxxxx> wrote:
>
> > From: Jiayuan Chen <jiayuan.chen@xxxxxxxxxx>
> >
> > swap_cache_get_folio() reads the folio from the swap table and takes a
> > reference with folio_try_get(), without any lock. If the folio is
> > removed from the swap cache and freed in between, and its memory gets
> > reused, folio_try_get() still succeeds and we return a page that has
> > nothing to do with the swap entry:
> >
> >    CPU 0                                  CPU 1
> >    do_swap_page()
> >      swap_cache_get_folio()
> >        swp_tb = swap_table_get()
> >        folio = swp_tb_to_folio(swp_tb)
> >                                           madvise(MADV_DONTNEED)
> >                                             zap_nonpresent_ptes()
> >                                               swap_put_entries_direct()
> >                                                 swap_put_entries_cluster()
> >                                                   __try_to_reclaim_swap()
> >                                                     swap_cache_del_folio()
> >                                                     folio_put()   <- freed
> >                                           page reused, e.g. as an anon
> >                                           folio of another task
> >        folio_try_get(folio)               <- succeeds
> >        return folio
> >      swap_update_readahead()              <- may clear PG_readahead on it
> >      folio_lock_or_retry()                <- locks it
> >      folio_matches_swap_entry() fails, unlock and put
> >
> > filemap_get_entry() does the same lockless lookup for the page cache,
> > and after folio_try_get() it rechecks the xarray, because the folio may
> > have been freed and reused in between. swap_cache_get_folio() lost
> > that recheck when the swap cache moved to the swap table.
> >
> > So CPU 0 writes to a folio it doesn't own: it may clear PG_readahead and
> > takes the folio lock of someone else's page, before
> > folio_matches_swap_entry() finds out the folio is not ours.
> >
> > Recheck the slot after folio_try_get() and retry if it no longer holds
> > this folio.
> LLM says to me:
>
> : The original folio could conceivably be freed, its physical page
> : reused, and the same PFN inserted into the same swap-table slot before
> : the second lookup.  That would pass the check.  However, the returned
> : reference would then belong to the newly inserted folio, and the later
> : swap-entry validation would still be required.  I don't see that as
> : invalidating this fix.



I had the same worry at first, data corruption or something worse, but
after thinking it through I agree. Passing the check there is fine:
the folio we return is the one currently in the slot, which is what
the function promises, and callers still check folio_matches_swap_entry()
under the folio lock before using it. That's why I didn't mention it in 
the changelog.


> > --- a/mm/swap_state.c
> > +++ b/mm/swap_state.c
> > @@ -92,17 +92,27 @@ void show_swap_cache_info(void)
> >    */
> >   struct folio *swap_cache_get_folio(swp_entry_t entry)
> >   {
> > +	struct swap_cluster_info *ci = __swap_entry_to_cluster(entry);
> > +	unsigned int ci_off = swp_cluster_offset(entry);
> >   	unsigned long swp_tb;
> >   	struct folio *folio;
> >   
> >   	for (;;) {
> > -		swp_tb = swap_table_get(__swap_entry_to_cluster(entry),
> > -					swp_cluster_offset(entry));
> > +		swp_tb = swap_table_get(ci, ci_off);
> >   		if (!swp_tb_is_folio(swp_tb))
> >   			return NULL;
> >   		folio = swp_tb_to_folio(swp_tb);
> > -		if (likely(folio_try_get(folio)))
> > +		if (!folio_try_get(folio))
> > +			continue;
> > +		/*
> > +		 * The folio may have been freed and its memory reused before
> > +		 * we got the reference. Make sure it is still in the slot.
> > +		 */
> > +		swp_tb = swap_table_get(ci, ci_off);
> > +		if (likely(swp_tb_is_folio(swp_tb) &&
> > +			   swp_tb_to_folio(swp_tb) == folio))
> >   			return folio;
> > +		folio_put(folio);
> >   	}
> >   
> >   	return NULL;
> This return is now unreachable...


Right, and it never was reachable, the loop has no break. It should have 
been dropped earlier...