Re: [PATCH] mm, swap: revalidate populated clusters before scanning

[Nhat Pham]

On Tue, Oct 20, 2026 at 6:12 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> On Mon, Oct 19, 2026 at 11:58 PM Youngjun Park <youngjun.park@xxxxxxx> wrote:
> >
> > On Mon, Oct 19, 2026 at 09:06:15AM -0700, Nhat Pham wrote:
> > > swap_cluster_populate() can publish a cluster's table with the cluster
> > > lock dropped. A cached allocator can use and empty it before the
> > > populating allocator reacquires the lock. The resumed scan can then
> > > dereference a NULL table.
> > >
> > > For example, CPU1 can retain a cached cluster offset on an SSD with
> > > per-page discard disabled:
> > >
> > >     CPU0                          CPU1
> > >     ----                          ----
> > >     swap_cluster_populate()
> > >       swap_cluster_alloc_table() fails
> > >       spin_unlock(&ci->lock)
> > >       local_unlock(&percpu_swap_cluster.lock)
> > >       swap_cluster_alloc_table()
> > >         rcu_assign_pointer(ci->table, table)
> > >                                   swap_alloc_fast()
> > >                                   local_unlock(&percpu_swap_cluster.lock)
> > >                                   mem_cgroup_try_charge_swap() fails
> > >                                   swap_cache_del_folio()
> > >                                     __swap_cluster_free_entries()
> > >                                       free_cluster()
> > >                                         __free_cluster()
> > >                                           swap_cluster_free_table()
> > >                                             rcu_assign_pointer(..., NULL)
> > >       local_lock(&percpu_swap_cluster.lock)
> > >       spin_lock(&ci->lock)
> > >       return ci
> > >     alloc_swap_scan_cluster()
> > >       cluster_scan_range()
> > >         __swap_table_get()
> >
> > Hello Nhat.
> >
> > Good catch!
> >
> > > Reject an unusable cluster under its lock before scanning it. Relocate
> > > it only while its table is present.
> > >
> > > Fixes: cdd77f84d966 ("mm, swap: consolidate cluster allocation helpers")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Nhat Pham <nphamcs@xxxxxxxxx>
> > > ---
> > >  mm/swapfile.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > index c3288910b3e3..988b7316e0a1 100644
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -981,7 +981,11 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
> > >       bool need_reclaim, ret, usable;
> > >
> > >       lockdep_assert_held(&ci->lock);
> > > -     VM_WARN_ON(!cluster_is_usable(ci, order));
> > > +     if (!cluster_is_usable(ci, order)) {
> > > +             if (cluster_table_is_alloced(ci))
> > > +                     relocate_cluster(si, ci);
> > > +             goto out_unlock;
> > > +     }
> >
> > If this patch is applied as bloew, does it prevent the issue described here?
> >
> > https://lore.kernel.org/linux-mm/20260720071342.50742-2-shikemeng@xxxxxxxxxxxxxxx/
> > (This patch publishes cluster table after lock acquired.)
> >
> > This patch is chagned the way I suggested during review... :(
>
> Ah I missed that patch series and your review! What happened to it?
> Did we just never land it?
>
> Looks like we are still doing neither Keming's approach (table
> copying) nor your approach (publishing at the end of
> swap_cluster_alloc_table()).

Ah nope, I saw that Kemeng implemented your approach:

https://lore.kernel.org/linux-mm/20260907091356.53026-2-shikemeng@xxxxxxxxxxxxxxx/

My bad! But yeah I think we still need this small fix (or Kemeng's
original proposal?), unless I missed something...?