[PATCH 0/2] mm: fix swapcache writeout and cluster table publication

[PATCH 0/2] mm: fix swapcache writeout and cluster table publication

[Nhat Pham]

Sashiko's review of the vswap series uncovered two bugs in existing swap
code: reclaim skips swapcache folios without anon mappings, and cluster
tables can be published while allocator locks are dropped.

Allow swapcache folios through pageout() and keep cluster tables private
until the allocator locks are reacquired. The second patch completes
Kairui Song's locking sketch.

Nhat Pham (2):
  mm: vmscan: allow writeout of swapcache folios without anon mappings
  mm/swap: publish cluster tables after reacquiring allocator locks

 mm/swapfile.c | 106 +++++++++++++++++++++++++++++++++++---------------
 mm/vmscan.c   |   3 +-
 2 files changed, 76 insertions(+), 33 deletions(-)


base-commit: 32959c9a49d24faffb39413322ef80014f88dd16

[PATCH 1/2] mm: vmscan: allow writeout of swapcache folios without anon mappings

[Nhat Pham]

Unfaulted zswap readahead folios can be dirty and in swapcache without
an anonymous mapping. pageout() rejects them before swap_writeout(),
so reclaim cannot write them out while their swap entries remain
referenced.

Allow swapcache folios through the filesystem folio check. They already
have swap entries and can use the existing swap writeout path. Anonymous
folios outside swapcache have no mapping and already return PAGE_KEEP.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/sashiko-outbox-181078@kernel.org/
Fixes: 84798514db50 ("mm: Remove swap_writepage() and shmem_writepage()")
Cc: stable@vger.kernel.org
Signed-off-by: Nhat Pham <nphamcs@gmail.com>
---
 mm/vmscan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7db6be53f3a9..4b014e3c07bd 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -714,7 +714,8 @@ static pageout_t pageout(struct swap_io_ctx *ctx, struct address_space *mapping,
 	 */
 	if (folio_ref_count(folio) != 1 + folio_nr_pages(folio) || !mapping)
 		return PAGE_KEEP;
-	if (!shmem_mapping(mapping) && !folio_test_anon(folio))
+	/* Anonymous folios outside swapcache returned PAGE_KEEP above. */
+	if (!shmem_mapping(mapping) && !folio_test_swapcache(folio))
 		return PAGE_ACTIVATE;
 	if (!folio_clear_dirty_for_io(folio))
 		return PAGE_CLEAN;

[PATCH 2/2] mm/swap: publish cluster tables after reacquiring allocator locks

[Nhat Pham]

Under memory pressure, swap_cluster_populate() can publish a table
while its locks are dropped for a sleeping allocation. A cached
allocator can consume and free that table before the populating
allocator resumes, leaving the resumed scan with a NULL table.

    CPU0                           CPU1
    swap_cluster_populate()
      drop local/global/ci locks
      allocate and publish table
                                   swap_alloc_fast()
                                     consume the table
                                   charge failure cleanup
                                     free the table
      reacquire local/global/ci
      return ci
    alloc_swap_scan_cluster()
      read the missing table

Keep the count table, cgroup table and zero bitmap private until
swap_cluster_alloc_table() restores all allocator locks. Install them
under the cluster lock and retain it through the scan. Initial bad-slot
setup takes only the cluster lock because the device is unpublished.

A stale reclaimer can return the isolated cluster to allocation while
its locks are dropped. Abandon installation if the list state changes or
a table appears, freeing only private allocations. Restore locks on
allocation failure too, so the caller can return a still-isolated
cluster to its free list.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/sashiko-outbox-181084@kernel.org/
Link: https://lore.kernel.org/all/sashiko-outbox-181086@kernel.org/
Co-developed-by: Kairui Song <ryncsn@gmail.com>
Fixes: cdd77f84d966 ("mm, swap: consolidate cluster allocation helpers")
Cc: stable@vger.kernel.org
Signed-off-by: Nhat Pham <nphamcs@gmail.com>
---
 mm/swapfile.c | 106 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 32 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 189f04d8838f..162a73a9e663 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -449,15 +449,23 @@ 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)
+/*
+ * Return with ci locked. An unlocked allocator also needs si's outer locks;
+ * initial bad-slot setup passes NULL because the device is not published.
+ */
+static int swap_cluster_alloc_table(struct swap_info_struct *si,
+				    struct swap_cluster_info *ci,
+				    gfp_t gfp, bool locked)
 {
 	struct swap_table *table = NULL;
+#ifdef CONFIG_MEMCG
+	struct swap_memcg_table *memcg_table = NULL;
+#endif
+#if !SWAP_TABLE_HAS_ZEROFLAG
+	unsigned long *zero_bitmap = NULL;
+#endif
 	struct folio *folio;
-
-	/* The cluster must be empty and not on any list during allocation. */
-	VM_WARN_ON_ONCE(ci->flags || !cluster_is_empty(ci));
-	if (rcu_access_pointer(ci->table))
-		return 0;
+	int ret = -ENOMEM;
 
 	if (SWP_TABLE_USE_PAGE) {
 		folio = folio_alloc(gfp | __GFP_ZERO, 0);
@@ -467,27 +475,49 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
 		table = kmem_cache_zalloc(swap_table_cachep, gfp);
 	}
 	if (!table)
-		return -ENOMEM;
+		goto lock;
 
 #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) {
-			swap_cluster_free_count_table(table);
-			return -ENOMEM;
-		}
+		memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
+		if (!memcg_table)
+			goto lock;
 	}
 #endif
 
 #if !SWAP_TABLE_HAS_ZEROFLAG
-	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);
-		return -ENOMEM;
+	zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
+	if (!zero_bitmap)
+		goto lock;
+#endif
+	ret = 0;
+
+lock:
+	if (!locked) {
+		if (si) {
+			local_lock(&percpu_swap_cluster.lock);
+			if (!(si->flags & SWP_SOLIDSTATE))
+				spin_lock(&si->global_cluster_lock);
+		}
+		spin_lock(&ci->lock);
 	}
+	lockdep_assert_held(&ci->lock);
+
+	/* A stale reclaimer may have returned the cluster to allocation. */
+	if (ci->flags != CLUSTER_FLAG_NONE || cluster_table_is_alloced(ci))
+		ret = -EAGAIN;
+	if (ret)
+		goto free;
+
+	/* The cluster must be empty and not on any list during allocation. */
+	VM_WARN_ON_ONCE(!cluster_is_empty(ci));
+#ifdef CONFIG_MEMCG
+	VM_WARN_ON_ONCE(ci->memcg_table);
+	ci->memcg_table = memcg_table;
+#endif
+#if !SWAP_TABLE_HAS_ZEROFLAG
+	VM_WARN_ON_ONCE(ci->zero_bitmap);
+	ci->zero_bitmap = zero_bitmap;
 #endif
 
 	/*
@@ -496,6 +526,17 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
 	 */
 	rcu_assign_pointer(ci->table, table);
 	return 0;
+
+free:
+#ifdef CONFIG_MEMCG
+	kfree(memcg_table);
+#endif
+#if !SWAP_TABLE_HAS_ZEROFLAG
+	bitmap_free(zero_bitmap);
+#endif
+	if (table)
+		swap_cluster_free_count_table(table);
+	return ret;
 }
 
 /*
@@ -546,8 +587,8 @@ swap_cluster_populate(struct swap_info_struct *si,
 		lockdep_assert_held(&si->global_cluster_lock);
 	lockdep_assert_held(&ci->lock);
 
-	if (!swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
-					  __GFP_NOWARN))
+	if (!swap_cluster_alloc_table(si, ci, __GFP_HIGH | __GFP_NOMEMALLOC |
+					      __GFP_NOWARN, true))
 		return ci;
 
 	/*
@@ -560,8 +601,8 @@ swap_cluster_populate(struct swap_info_struct *si,
 		spin_unlock(&si->global_cluster_lock);
 	local_unlock(&percpu_swap_cluster.lock);
 
-	ret = swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
-					   GFP_KERNEL);
+	ret = swap_cluster_alloc_table(si, ci, __GFP_HIGH | __GFP_NOMEMALLOC |
+					     GFP_KERNEL, false);
 
 	/*
 	 * Back to atomic context. We might have migrated to a new CPU with a
@@ -571,13 +612,9 @@ swap_cluster_populate(struct swap_info_struct *si,
 	 * could happen with ignoring the percpu cluster is fragmentation,
 	 * which is acceptable since this fallback and race is rare.
 	 */
-	local_lock(&percpu_swap_cluster.lock);
-	if (!(si->flags & SWP_SOLIDSTATE))
-		spin_lock(&si->global_cluster_lock);
-	spin_lock(&ci->lock);
-
 	if (ret) {
-		move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
+		if (ret != -EAGAIN)
+			move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
 		spin_unlock(&ci->lock);
 		return NULL;
 	}
@@ -825,9 +862,13 @@ static int swap_cluster_setup_bad_slot(struct swap_info_struct *si,
 
 	ci = cluster_info + idx;
 	/* Need to allocate swap table first for initial bad slot marking. */
-	if (!ci->count && swap_cluster_alloc_table(ci, GFP_KERNEL))
-		return -ENOMEM;
-	spin_lock(&ci->lock);
+	if (!ci->count) {
+		ret = swap_cluster_alloc_table(NULL, ci, GFP_KERNEL, false);
+		if (ret)
+			goto out;
+	} else {
+		spin_lock(&ci->lock);
+	}
 	/* Check for duplicated bad swap slots. */
 	if (__swap_table_xchg(ci, ci_off, SWP_TB_BAD) != SWP_TB_NULL) {
 		pr_warn("Duplicated bad slot offset %d\n", offset);
@@ -835,6 +876,7 @@ static int swap_cluster_setup_bad_slot(struct swap_info_struct *si,
 	} else {
 		ci->count++;
 	}
+out:
 	spin_unlock(&ci->lock);
 
 	WARN_ON(ci->count > SWAPFILE_CLUSTER);

Re: [PATCH 2/2] mm/swap: publish cluster tables after reacquiring allocator locks

[Nhat Pham]

On Fri, Nov 6, 2026 at 9:04 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> Under memory pressure, swap_cluster_populate() can publish a table
> while its locks are dropped for a sleeping allocation. A cached
> allocator can consume and free that table before the populating
> allocator resumes, leaving the resumed scan with a NULL table.
>
>     CPU0                           CPU1
>     swap_cluster_populate()
>       drop local/global/ci locks
>       allocate and publish table
>                                    swap_alloc_fast()
>                                      consume the table
>                                    charge failure cleanup
>                                      free the table
>       reacquire local/global/ci
>       return ci
>     alloc_swap_scan_cluster()
>       read the missing table
>
> Keep the count table, cgroup table and zero bitmap private until
> swap_cluster_alloc_table() restores all allocator locks. Install them
> under the cluster lock and retain it through the scan. Initial bad-slot
> setup takes only the cluster lock because the device is unpublished.
>
> A stale reclaimer can return the isolated cluster to allocation while
> its locks are dropped. Abandon installation if the list state changes or
> a table appears, freeing only private allocations. Restore locks on
> allocation failure too, so the caller can return a still-isolated
> cluster to its free list.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/all/sashiko-outbox-181084@kernel.org/
> Link: https://lore.kernel.org/all/sashiko-outbox-181086@kernel.org/
> Co-developed-by: Kairui Song <ryncsn@gmail.com>

I think I need a

Signed-off-by: Kairui Song <ryncsn@gmail.com>

here as well...

(Kairui - up to you if you feel like Co-developed-by + Signed-off-by
or Suggested-by + Acked-by is more appropriate here. I just filled in
the sketch you gave me.)

Re: [PATCH 2/2] mm/swap: publish cluster tables after reacquiring allocator locks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm/swap: NULL deref in swap_cluster_assert_empty() on stale
  reclaimer race
- [Medium] mm/swap: ABA race on cluster state check in
  swap_cluster_alloc_table()
--

commit 22e220b18bd05df45a828a3170169b92c0c14f76
Author: Nhat Pham <nphamcs@gmail.com>

mm/swap: publish cluster tables after reacquiring allocator locks

This commit keeps newly allocated swap cluster tables private until
swap_cluster_alloc_table() reacquires the allocator and cluster locks, and
abandons installation if a stale reclaimer returned the cluster while the
locks were dropped.

Link: https://lore.kernel.org/all/sashiko-outbox-181084@kernel.org/
Link: https://lore.kernel.org/all/sashiko-outbox-181086@kernel.org/

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 189f04d8838f3..162a73a9e6638 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -467,27 +475,49 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>  		table = kmem_cache_zalloc(swap_table_cachep, gfp);
>  	}
>  	if (!table)
> -		return -ENOMEM;
> +		goto lock;
>  
>  #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) {
> -			swap_cluster_free_count_table(table);
> -			return -ENOMEM;
> -		}
> +		memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
> +		if (!memcg_table)
> +			goto lock;
>  	}
>  #endif
>  
>  #if !SWAP_TABLE_HAS_ZEROFLAG
> -	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);
> -		return -ENOMEM;
> +	zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
> +	if (!zero_bitmap)
> +		goto lock;
> +#endif
> +	ret = 0;
> +
> +lock:
> +	if (!locked) {
> +		if (si) {
> +			local_lock(&percpu_swap_cluster.lock);
> +			if (!(si->flags & SWP_SOLIDSTATE))
> +				spin_lock(&si->global_cluster_lock);
> +		}
> +		spin_lock(&ci->lock);
>  	}
> +	lockdep_assert_held(&ci->lock);
> +
> +	/* A stale reclaimer may have returned the cluster to allocation. */
> +	if (ci->flags != CLUSTER_FLAG_NONE || cluster_table_is_alloced(ci))
> +		ret = -EAGAIN;

[Severity: Medium]
Can a stale reclaimer trigger a NULL pointer dereference in
swap_cluster_assert_empty() while ci->lock is dropped here?

Because ci->table now remains NULL across the sleeping allocation in
swap_cluster_populate(), a stale reclaimer that dropped ci->lock before the
cluster was freed and re-isolated can reacquire ci->lock while ci->count is 0,
ci->flags is CLUSTER_FLAG_NONE, and ci->table is NULL:

CPU0                              CPU1
                                  cluster_reclaim_range()
                                    spin_unlock(&ci->lock)
                                    /* ci is freed and table is freed */
isolate_lock_cluster()
  ci->flags = CLUSTER_FLAG_NONE
swap_cluster_populate()
  /* atomic alloc fails */
  spin_unlock(&ci->lock)
  swap_cluster_alloc_table()
                                    spin_lock(&ci->lock)
                                  ...
                                  relocate_cluster()
                                    free_cluster()
                                      __free_cluster()
                                        swap_cluster_assert_empty()
                                          __swap_table_get()
                                            <-- NULL pointer dereference

In relocate_cluster(), !ci->count and ci->flags != CLUSTER_FLAG_FREE are both
true, so it calls free_cluster() -> __free_cluster() without checking
cluster_table_is_alloced(ci).  With CONFIG_DEBUG_VM enabled,
swap_cluster_assert_empty() calls __swap_table_get(), which dereferences the
NULL ci->table.  With SWP_PAGE_DISCARD enabled, free_cluster() also schedules
a redundant discard before swap_do_scheduled_discard() calls __free_cluster()
on the tableless cluster.

Should relocate_cluster() avoid calling free_cluster() when
!cluster_table_is_alloced(ci)?

[ ... ]

> @@ -571,13 +612,9 @@ swap_cluster_populate(struct swap_info_struct *si,
>  	 * could happen with ignoring the percpu cluster is fragmentation,
>  	 * which is acceptable since this fallback and race is rare.
>  	 */
> -	local_lock(&percpu_swap_cluster.lock);
> -	if (!(si->flags & SWP_SOLIDSTATE))
> -		spin_lock(&si->global_cluster_lock);
> -	spin_lock(&ci->lock);
> -
>  	if (ret) {
> -		move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
> +		if (ret != -EAGAIN)
> +			move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
>  		spin_unlock(&ci->lock);
>  		return NULL;
>  	}

[Severity: Medium]
Can an ABA race on ci->flags and ci->table cause a failed allocator to put a
cluster back onto si->free_clusters while a second allocator still has it
isolated?

When CONFIG_DEBUG_VM and SWP_PAGE_DISCARD are disabled, a stale reclaimer
calling relocate_cluster() -> free_cluster() -> __free_cluster() moves the
tableless cluster back to si->free_clusters.  A second allocator can then
re-isolate it and also drop ci->lock for a sleeping table allocation:

CPU0                       CPU1                       CPU2
swap_cluster_populate()
  spin_unlock(&ci->lock)
  swap_cluster_alloc_table()
    /* alloc fails */
                           relocate_cluster()
                             ...
                             __free_cluster()
                               ci->flags =
                                 CLUSTER_FLAG_FREE
                                                      isolate_lock_cluster()
                                                        ci->flags =
                                                          CLUSTER_FLAG_NONE
                                                      swap_cluster_populate()
                                                        spin_unlock(&ci->lock)
    spin_lock(&ci->lock)
    /* flags == NONE,
     * table == NULL */
    return -ENOMEM
  move_cluster(..., FREE)  <-- puts ci on free_clusters while CPU2 owns it

When CPU0 reacquires ci->lock, ci->flags is back to CLUSTER_FLAG_NONE and
cluster_table_is_alloced(ci) is still false, so swap_cluster_alloc_table()
returns -ENOMEM instead of -EAGAIN and CPU0 moves the cluster back onto
si->free_clusters while CPU2 is still sleeping in swap_cluster_alloc_table().

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

Re: [PATCH 2/2] mm/swap: publish cluster tables after reacquiring allocator locks

[Kairui Song]

On Sat, Nov 7, 2026 at 3:59 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Fri, Nov 6, 2026 at 9:04 AM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > Under memory pressure, swap_cluster_populate() can publish a table
> > while its locks are dropped for a sleeping allocation. A cached
> > allocator can consume and free that table before the populating
> > allocator resumes, leaving the resumed scan with a NULL table.
> >
> >     CPU0                           CPU1
> >     swap_cluster_populate()
> >       drop local/global/ci locks
> >       allocate and publish table
> >                                    swap_alloc_fast()
> >                                      consume the table
> >                                    charge failure cleanup
> >                                      free the table
> >       reacquire local/global/ci
> >       return ci
> >     alloc_swap_scan_cluster()
> >       read the missing table
> >
> > Keep the count table, cgroup table and zero bitmap private until
> > swap_cluster_alloc_table() restores all allocator locks. Install them
> > under the cluster lock and retain it through the scan. Initial bad-slot
> > setup takes only the cluster lock because the device is unpublished.
> >
> > A stale reclaimer can return the isolated cluster to allocation while
> > its locks are dropped. Abandon installation if the list state changes or
> > a table appears, freeing only private allocations. Restore locks on
> > allocation failure too, so the caller can return a still-isolated
> > cluster to its free list.
> >
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Link: https://lore.kernel.org/all/sashiko-outbox-181084@kernel.org/
> > Link: https://lore.kernel.org/all/sashiko-outbox-181086@kernel.org/
> > Co-developed-by: Kairui Song <ryncsn@gmail.com>
>
> I think I need a
>
> Signed-off-by: Kairui Song <ryncsn@gmail.com>
>
> here as well...
>
> (Kairui - up to you if you feel like Co-developed-by + Signed-off-by
> or Suggested-by + Acked-by is more appropriate here. I just filled in
> the sketch you gave me.)

Let's go with Suggested-by then :), I didn't even check if that sketch
actually works, so thanks for taking care of that!