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

[Nhat Pham]

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()).

>
> As you pointed out, another context can free the table
> without alloc context knwoing this situation.
> I missed this situation.

Yeah I feel like it's still not safe with your approach...
swap_cluster_alloc_table() does not hold nor acquire cluster's lock.
Between its publishing of the table and the subsequent lock
acquisition in its caller (swap_cluster_populate()), someone can still
come in and free the table.

It's tricky. We do not have an explicit contract for the metadata's
lifetime, so it's a bit of reading the tea leaves for me...

I think this is the last gap, and it's rare enough, so this simple
check should suffice to cover the edgiest of the edge cases :) But who
knows :) If bugs re-surfaced again, I think we can spend a bit more
time ironcladding the lifetime of clusters/tables:

1. Allocate outside of lock context, but make swap_cluster_populate()
lock the cluster, check cluster's status, and only then publish the
table (and return with the cluster locked). Basically Keming's
approach.

2. Carefully enforce metadata's lifetime. Perhaps we can add a refcnt?
In addition to occupied slots holding cluster's refcnt, we can allow
for temporary holders. As long as we can acquire a refcnt, we know for
sure the metadata is not freed under us. This is my approach for the
old vswap patch series, but it's tricky, especially with caching in
the picture.

>
> IMHO, Publishing the table only after reacquiring the cluster lock, as Kemeng
> suggested, and rejecting an unusable cluster under the lock as in this
> patch both look correct to me!
>
> Reviewed-by: Youngjun Park <youngjun.park@xxxxxxx>

Thanks for the review, Youngjun!

>