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

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

[Nhat Pham]

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

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@vger.kernel.org
Signed-off-by: Nhat Pham <nphamcs@gmail.com>
---
 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 (end < nr_pages || ci->count + nr_pages > SWAPFILE_CLUSTER)
 		goto out;
@@ -1010,6 +1014,7 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
 	}
 out:
 	relocate_cluster(si, ci);
+out_unlock:
 	swap_cluster_unlock(ci);
 	if (si->flags & SWP_SOLIDSTATE) {
 		this_cpu_write(percpu_swap_cluster.offset[order], next);

base-commit: 4dbe24720eb3811563cd56b35d87ddf3f9beed2a
-- 
2.53.0-Meta

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

[Nhat Pham]

On Mon, Oct 19, 2026 at 9:06 AM Nhat Pham <nphamcs@gmail.com> 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()
>
> 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@vger.kernel.org
> Signed-off-by: Nhat Pham <nphamcs@gmail.com>

I'm revisiting the lifecycle of clusters, because vswap introduced
dynamic (i.e run-time freeing) behavior to it. For the most part it
looks good (both physical and virtual side), but this look racy to me.
I don't see how it can be safe. Once you drop the lock, all bets are
off...

But at the same time, I have not been able to reproduce it (yet). Will
report here if I manage to.

(Side note: my vswap code is actually safe in this scenario. CPU0
allocates a new cluster entirely, and lock it before publishing it. So
even if the cluster index is cached in CPU1, this race cannot happen).

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

[Youngjun Park]

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@vger.kernel.org
> Signed-off-by: Nhat Pham <nphamcs@gmail.com>
> ---
>  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@huaweicloud.com/
(This patch publishes cluster table after lock acquired.)

This patch is chagned the way I suggested during review... :(

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

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@lge.com>