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

[Kairui Song]

On Tue, Oct 20, 2026 at 9:23 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> 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...?

Right I remember that, I acked part of that and was waiting the update
on that sashiko comment, as the first patch isn't perfect, while
Kemeng's mail at "shikemeng@xxxxxxxxxxxxxxx" is now bouncing back...
I'm not sure if Kemeng still plans to update that.