[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>

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

[Nhat Pham]

On Mon, Oct 19, 2026 at 11:58 PM Youngjun Park <youngjun.park@lge.com> 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@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... :(

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

Thanks for the review, Youngjun!

>

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

[Nhat Pham]

On Tue, Oct 20, 2026 at 6:12 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Mon, Oct 19, 2026 at 11:58 PM Youngjun Park <youngjun.park@lge.com> 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@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... :(
>
> 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@huaweicloud.com/

My bad! But yeah I think we still need this small fix (or Kemeng's
original proposal?), unless I missed something...?

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm, swap: alloc_swap_scan_cluster() relocates and scans stolen clusters
- [High] mm, swap: alloc_swap_scan_cluster() relocates unusable cluster after reclaim
- [Medium] mm, swap: alloc_swap_scan_list() aborts free_clusters scan on stolen cluster
--

commit 4e1646405a5a83e283c6f4c0fe1ba0012bdd06c6
Author: Nhat Pham <nphamcs@gmail.com>

mm, swap: revalidate populated clusters before scanning

This commit checks cluster_is_usable() at the start of
alloc_swap_scan_cluster() to avoid dereferencing a NULL cluster table when a
cached allocator uses and frees a newly populated cluster while
swap_cluster_populate() has dropped the cluster lock.

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index c3288910b3e35..988b7316e0a18 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;
> +	}

[Severity: High]
Can this relocate a cluster that another CPU has already relocated or
isolated, and still scan a cluster that was stolen while
swap_cluster_populate() dropped ci->lock?

All callers of alloc_swap_scan_cluster() other than
isolate_lock_cluster(si, &si->free_clusters) either check
cluster_is_usable(ci, order) under ci->lock immediately prior or isolate ci
without dropping ci->lock. When swap_cluster_populate() publishes ci->table
in swap_cluster_alloc_table() while ci->lock is dropped, ci initially has
ci->flags == CLUSTER_FLAG_NONE and ci->count == 0, which satisfies
cluster_is_usable(ci, order) for all orders.

Consequently, !cluster_is_usable(ci, order) at entry to
alloc_swap_scan_cluster() can only happen if a racing CPU (CPU1) locked ci via
a cached offset (such as in swap_alloc_fast()) and ran
alloc_swap_scan_cluster() before the populating CPU (CPU0) reacquired
ci->lock. When CPU1 finished alloc_swap_scan_cluster(), CPU1 already called
relocate_cluster(si, ci) at the out label, placing ci onto frag_clusters,
full_clusters, discard_clusters, or free_clusters.

If CPU0 then calls relocate_cluster(si, ci) when cluster_table_is_alloced(ci)
is true:

1. If a third CPU (CPU2) subsequently isolated ci from frag_clusters or
full_clusters via isolate_lock_cluster() (setting ci->flags to
CLUSTER_FLAG_NONE) and dropped ci->lock inside cluster_reclaim_range() or
swap_reclaim_full_clusters(), CPU0's relocate_cluster(si, ci) sees
ci->flags == CLUSTER_FLAG_NONE:

mm/swapfile.c:relocate_cluster() {
	...
	} else if (ci->count != SWAPFILE_CLUSTER) {
		if (ci->flags != CLUSTER_FLAG_FRAG)
			move_cluster(si, ci, &si->frag_clusters[ci->order],
				     CLUSTER_FLAG_FRAG);
	} else {
		if (ci->flags != CLUSTER_FLAG_FULL)
			move_cluster(si, ci, &si->full_clusters,
				     CLUSTER_FLAG_FULL);
	}
}

This moves ci back onto si->frag_clusters[ci->order] or si->full_clusters
while CPU2 still holds it isolated, allowing another CPU to isolate the same
cluster concurrently.

2. If an entry in ci was freed after CPU1 allocated from it,
partial_free_cluster() moved ci to si->nonfull_clusters[ci->order] with
ci->flags == CLUSTER_FLAG_NONFULL. CPU0's relocate_cluster(si, ci) sees
ci->flags != CLUSTER_FLAG_FRAG and demotes ci from nonfull_clusters to
frag_clusters.

Conversely, because swap_cluster_populate() publishes ci->table before
reacquiring percpu_swap_cluster.lock, si->global_cluster_lock, and ci->lock,
what happens if CPU1 allocates from ci while ci->lock is dropped, but
cluster_is_usable(ci, order) remains true for CPU0's order?

mm/swapfile.c:cluster_is_usable() {
	if (unlikely(ci->flags > CLUSTER_FLAG_USABLE))
		return false;
	if (!cluster_table_is_alloced(ci))
		return false;
	if (!order)
		return true;
	return cluster_is_empty(ci) || order == ci->order;
}

For example, if CPU0 has order == 0 while CPU1 claimed ci for a higher order
and placed it on si->frag_clusters[ci->order], or if CPU1 allocated the same
order and placed ci on frag_clusters (or CPU2 isolated it from frag_clusters
and dropped ci->lock in cluster_reclaim_range()), cluster_is_usable(ci, order)
still returns true on CPU0. Wouldn't CPU0 then scan, allocate from, and
relocate a cluster that is already on frag_clusters or isolated by CPU2,
fragmenting high-order clusters and caching the same cluster on multiple CPUs?

[Severity: Medium]
Also, when alloc_swap_scan_cluster() takes the goto out_unlock path for a
stolen cluster, it returns SWAP_ENTRY_INVALID to alloc_swap_scan_list():

mm/swapfile.c:alloc_swap_scan_list() {
	do {
		struct swap_cluster_info *ci = isolate_lock_cluster(si, list);
		unsigned long offset;

		if (!ci)
			break;
		offset = cluster_offset(si, ci);
		found = alloc_swap_scan_cluster(si, ci, folio, offset);
		if (found)
			break;
	} while (scan_all);
}

In cluster_alloc_swap_entry(), both calls scanning &si->free_clusters pass
scan_all = false:

mm/swapfile.c:cluster_alloc_swap_entry() {
	if (si->flags & SWP_PAGE_DISCARD) {
		found = alloc_swap_scan_list(si, &si->free_clusters, folio, false);
		if (found)
			goto done;
	}
	...
	if (!(si->flags & SWP_PAGE_DISCARD)) {
		found = alloc_swap_scan_list(si, &si->free_clusters, folio, false);
		if (found)
			goto done;
	}
}

Because scan_all is false, won't alloc_swap_scan_list() exit the loop after a
single stolen cluster without trying to isolate another cluster from
si->free_clusters (causing PMD_ORDER allocations, or allocations when
nonfull_clusters and frag_clusters are empty, to fail even when free clusters
remain on si->free_clusters)?

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

[Severity: High]
Does the !usable path after cluster_reclaim_range() in
alloc_swap_scan_cluster() have a similar issue when jumping to out instead of
out_unlock?

In the scan loop just above:

mm/swapfile.c:alloc_swap_scan_cluster() {
		if (need_reclaim) {
			ret = cluster_reclaim_range(si, ci, offset, order, &usable);
			if (!usable)
				goto out;
}

cluster_reclaim_range() drops ci->lock to call __try_to_reclaim_swap() and
then rechecks cluster_is_usable(ci, order) after reacquiring ci->lock:

mm/swapfile.c:cluster_reclaim_range() {
	spin_unlock(&ci->lock);
	...
	spin_lock(&ci->lock);
	...
	if (!cluster_is_usable(ci, order)) {
		*usable = false;
		return false;
	}
}

While ci->lock is dropped in cluster_reclaim_range(), __try_to_reclaim_swap()
or a concurrent free on another CPU can free the last entry in ci, causing
__swap_cluster_free_entries() -> free_cluster() -> __free_cluster() to free
ci->table (setting ci->table to NULL) and move ci to si->free_clusters with
ci->flags = CLUSTER_FLAG_FREE.

Before cluster_reclaim_range() reacquires ci->lock, another CPU (CPU2) can
call isolate_lock_cluster(si, &si->free_clusters), which removes ci from
si->free_clusters, sets ci->flags = CLUSTER_FLAG_NONE, and drops ci->lock in
swap_cluster_populate() when atomic table allocation fails.

When cluster_reclaim_range() reacquires ci->lock, ci->count is 0, ci->flags is
CLUSTER_FLAG_NONE, and ci->table is NULL, so cluster_reclaim_range() sets
*usable = false. Back in alloc_swap_scan_cluster(), 'if (!usable) goto out;'
calls relocate_cluster(si, ci), which sees !ci->count and
ci->flags != CLUSTER_FLAG_FREE and calls free_cluster(si, ci) ->
__free_cluster(si, ci) while ci->table is NULL and CPU2 is running
swap_cluster_alloc_table() unlocked.

Won't that dereference a NULL ci->table in swap_cluster_assert_empty() (under
CONFIG_DEBUG_VM), race swap_cluster_free_table() against CPU2's unlocked
swap_cluster_alloc_table() on ci->memcg_table and ci->zero_bitmap, and place
ci back onto si->free_clusters while CPU2 holds it isolated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019160615.2348500-1-nphamcs@gmail.com?part=1

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

[Kairui Song]

On Tue, Oct 20, 2026 at 1:41 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)

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?

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.

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

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

[Kairui Song]

On Tue, Oct 20, 2026 at 9:23 PM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Tue, Oct 20, 2026 at 6:12 AM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > On Mon, Oct 19, 2026 at 11:58 PM Youngjun Park <youngjun.park@lge.com> 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@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... :(
> >
> > 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@huaweicloud.com/
>
> 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@huaweicloud.com" is now bouncing back...
I'm not sure if Kemeng still plans to update that.

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

[Nhat Pham]

On Tue, Oct 20, 2026 at 10:58 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Tue, Oct 20, 2026 at 1:41 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)
>
> 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 :)