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

[Kairui Song]

Nhat Pham <nphamcs@xxxxxxxxx> 于 2026年10月21日周三 04:19写道：
>
> On Tue, Oct 20, 2026 at 10:58 AM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> >
> > On Tue, Oct 20, 2026 at 1:41 AM Nhat Pham <nphamcs@xxxxxxxxx> 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)
> >
> > Nice catch!
> >
> > I think the real problem is that it turns out calling
> > swap_cluster_alloc_table() without holding ci lock is bad idea... I
> > think this patch still leaves some other call site too?
>
> Do we have another callsite? There's a callsite in setup code, but I
> assume that's safe...?
>
> But at this point, who knows any more. The caching only stores a
> swapfile struct, and an offset. I wonder if there is a potential race
> between swapoff-then-re-swapon and cached allocation? That'd be pretty
> wild.
>
>
> >
> > We may instead add a "bool locked" to swap_cluster_alloc_table, and
> > let it lock the ci first before it does any assign (not the alloc) if
> > the allocation context is not locked. That is, keep the
> > sleep-allocated tables private until swap_cluster_populate()
> > reacquires local/global/ci, then install them under ci->lock,
> > something like pre-cdd77f84d966, always assign it atomically.
>
> Hmm so a bitlock?
>
> Lemme think about it a bit.

Ah, no, not that complex, just something like:

--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -449,9 +449,13 @@ static void swap_cluster_free_table(struct
swap_cluster_info *ci)
        swap_cluster_free_count_table(table);
 }

-static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
+static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp,
+               bool locked)
 {
        struct swap_table *table = NULL;
+       struct swap_memcg_table *memcg_table = NULL;
+       unsigned long *zeromap = NULL;
+
        struct folio *folio;

        /* The cluster must be empty and not on any list during allocation. */
@@ -472,8 +476,8 @@ static int swap_cluster_alloc_table(struct
swap_cluster_info *ci, gfp_t gfp)
 #ifdef CONFIG_MEMCG
        if (!mem_cgroup_disabled()) {
                VM_WARN_ON_ONCE(ci->memcg_table);
-               ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
-               if (!ci->memcg_table) {
+               memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
+               if (!memcg_table) {
                        swap_cluster_free_count_table(table);
                        return -ENOMEM;
                }
@@ -484,8 +488,8 @@ static int swap_cluster_alloc_table(struct
swap_cluster_info *ci, gfp_t gfp)
        VM_WARN_ON_ONCE(ci->zero_bitmap);
        ci->zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
        if (!ci->zero_bitmap) {
-               swap_cluster_free_table(ci);
                swap_cluster_free_count_table(table);
+               kfree(memcg_table);
                return -ENOMEM;
        }
 #endif
@@ -494,6 +498,20 @@ static int swap_cluster_alloc_table(struct
swap_cluster_info *ci, gfp_t gfp)
         * Make tables visible to cluster_is_usable() after everything is
         * ready.
         */
+       if (!locked) {
+               / * Lock it here before assign, not after the asssign
and return */
...

This should fix all potential issues I think?