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

[Nhat Pham]

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.

>
> That should also cover the problem reported by Kemeng a few weeks
> earlier too, and prevent further misuse. My Bad on that change :P

No worries. Lifetime is hard :)