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

[Jiayuan Chen]

On 10/30/26 6:11 PM, Jiayuan Chen wrote:
> On 10/15/26 2:05 PM, Jiayuan Chen 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.
> >
> > Fixes: 8578e0c00dcf ("mm, swap: use the swap table for the swap cache 
> > and switch API")
> > Cc: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
> > Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxxx>
> > ---
> > https://sashiko.dev/#/patchset/20261014090821.227366-1-jiayuan.chen@xxxxxxxxx 
> >
> > ---
> >   mm/swap_state.c | 16 +++++++++++++---
> >   1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/swap_state.c b/mm/swap_state.c
> > index ebef568cd62e..d9e36c505398 100644
> > --- 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;
>
>
>
>
> I spent some more time on this and I'm not happy with any of the
> options I have. Advice welcome.
>
>   1. Trigger the worker where the zap gives up (this patch). On the
>      swap entry side that works, __try_to_reclaim_swap() has the si.
>      On the present PTE side it doesn't: free_swap_cache() lost the
>      trylock, so it can't read folio->swap to find the si, as Kairui
>      pointed out. And that trylock-and-give-up is not in one place,
>      free_swap_cache(), do_wp_page() and __uprobe_write() all have
>      their own copy. Chasing every one of them doesn't feel right.
>
>   2. Clean up at memcg offline. Doesn't cover the root cgroup or a
>      cgroup that stays alive, and on mm-new the folios are reparented
>      at offline anyway, so there is nothing left to find there.
>
>   3. A swapon flag, say --clean-cache, that enables the cleanup.
>      Pro: covers everything, and the cleanup can live in its own
>      function without touching the existing paths. Con: we know to
>      turn it on because we hit this, others won't. Needs util-linux
>      too.
>
>   4. A per-device worker that walks the device on its own, say 1024
>      clusters per second, backing off when a pass finds nothing.
>      Pro: same as 3, and nothing to turn on. Con: a worker that wakes
>      up on its own for this, I expect objections.
>
>   5. A sysctl (or a drop_caches bit) to kick the cleanup by hand.
>      Pro: same as 4, all in the worker. Con: same as 3, someone has
>      to know to poke it.
>
> 4 is the one I'd pick if nobody objects. Thoughts?


Plesase ignore his. I reply the wrong thread......