Re: [RFC PATCH v6 12/12] mm, swap: back vswap clusters with a VM_SPARSE array

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: vswap_grow_clusters() publishes nr_mapped_clusters before spin_lock_init()
- [High] mm, swap: race on unlocked table publish in swap_cluster_populate()
--

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

mm, swap: back vswap clusters with a VM_SPARSE array

This commit replaces the xarray of dynamically allocated vswap cluster
structures with a VM_SPARSE vmalloc array that grows on demand a page at a
time. Emptied vswap clusters now free their tables and return to
si->free_clusters for reuse rather than being freed via RCU.

Link: https://lore.kernel.org/all/20260916101929.149106-1-hebaoquan@kylinos.cn/

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index fe5460d15f555..4cb6586327c45 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -519,12 +550,19 @@ static int swap_cluster_alloc_table(struct swap_info_struct *si,
>  	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_table(si, ci);
>  		swap_cluster_free_count_table(table);
>  		return -ENOMEM;
>  	}
>  #endif
>  
> +	/* The virtual table shares the swap table's lifetime. */
> +	if (swap_is_vswap(si) && vswap_cluster_alloc_vtable(ci, gfp)) {
> +		swap_cluster_free_table(si, ci);
> +		swap_cluster_free_count_table(table);
> +		return -ENOMEM;
> +	}
> +
>  	/*
>  	 * Make tables visible to cluster_is_usable() after everything is
>  	 * ready.
> @@ -575,10 +613,8 @@ swap_cluster_populate(struct swap_info_struct *si,
>  	/*
>  	 * Only cluster isolation from the allocator does table allocation.
>  	 * Swap allocator uses percpu clusters and holds the local lock.
> -	 * vswap clusters are destroyed rather than freed to si->free_clusters.
>  	 */
> -	VM_WARN_ON_ONCE(swap_is_vswap(si));
> -	lockdep_assert_held(&this_cpu_ptr(&percpu_swap_cluster)->lock);
> +	swap_percpu_cluster_assert_held(si);
>  	if (!(si->flags & SWP_SOLIDSTATE))
>  		lockdep_assert_held(&si->global_cluster_lock);
>  	lockdep_assert_held(&ci->lock);
> @@ -595,7 +631,7 @@ swap_cluster_populate(struct swap_info_struct *si,
>  	spin_unlock(&ci->lock);
>  	if (!(si->flags & SWP_SOLIDSTATE))
>  		spin_unlock(&si->global_cluster_lock);
> -	local_unlock(&percpu_swap_cluster.lock);
> +	swap_percpu_cluster_unlock(si);
>  
>  	ret = swap_cluster_alloc_table(si, ci, __GFP_HIGH | __GFP_NOMEMALLOC |
>  					       GFP_KERNEL);
> @@ -608,7 +644,7 @@ 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);
> +	swap_percpu_cluster_lock(si);
>  	if (!(si->flags & SWP_SOLIDSTATE))
>  		spin_lock(&si->global_cluster_lock);
>  	spin_lock(&ci->lock);

[Severity: High]
Can a concurrent fast-path allocator race with the unlocked
swap_cluster_alloc_table() call in swap_cluster_populate()?

With vswap clusters now returning to si->free_clusters in __free_cluster()
instead of being destroyed with CLUSTER_FLAG_DEAD, other CPUs (or other
orders on the same CPU) can still hold an offset into a freed cluster in
percpu_vswap_cluster.offset[order].

When cluster_alloc_swap_entry() -> alloc_swap_scan_list() ->
isolate_lock_cluster() isolates ci from si->free_clusters, it sets
ci->flags = CLUSTER_FLAG_NONE and calls swap_cluster_populate(). If the
initial atomic allocation fails, swap_cluster_populate() drops ci->lock and
calls swap_cluster_alloc_table(si, ci, __GFP_HIGH | __GFP_NOMEMALLOC |
GFP_KERNEL), which publishes ci->virtual_table and ci->table before ci->lock
is re-acquired:

mm/swapfile.c:swap_cluster_alloc_table() {
	...
	rcu_assign_pointer(ci->table, table);
	return 0;
}

Because CLUSTER_FLAG_NONE is <= CLUSTER_FLAG_USABLE, an isolated cluster is
only prevented from passing cluster_is_usable(ci, order) by ci->table being
NULL. Once rcu_assign_pointer(ci->table, table) executes while ci->lock is
dropped, a concurrent vswap_alloc() (or swap_alloc_fast()) with a cached
offset into ci can lock ci->lock, see cluster_is_usable(ci, order) return
true, and allocate entries via alloc_swap_scan_cluster():

mm/swapfile.c:vswap_alloc() {
	if (offset != SWAP_ENTRY_INVALID) {
		ci = swap_cluster_lock(vswap_si, offset);
		if (cluster_is_usable(ci, order)) {
			if (cluster_is_empty(ci))
				offset = cluster_offset(vswap_si, ci);
			alloc_swap_scan_cluster(vswap_si, ci, folio, offset,
						NULL);
		} ...
	}
}

If that fast-path allocation claims ci for a different order and relocates it
onto another cluster list, or if those entries are then freed so
__free_cluster() frees ci->table and ci->virtual_table back to NULL before
swap_cluster_populate() re-acquires spin_lock(&ci->lock),
swap_cluster_populate() will still see ret == 0 and return ci to
alloc_swap_scan_list() -> alloc_swap_scan_cluster().

Could this trigger VM_WARN_ON(!cluster_is_usable(ci, order)) in
alloc_swap_scan_cluster(), corrupt cluster list and order state, or
dereference a NULL ci->table in cluster_scan_range() -> __swap_table_get()?

[ ... ]

> @@ -1206,50 +1229,146 @@ static unsigned int alloc_swap_scan_list(struct swap_info_struct *si,

[ ... ]

> +static int vswap_grow_clusters(struct swap_info_struct *si)
>  {

[ ... ]

> +	/*
> +	 * Publish the new clusters before they become reachable by offset.
> +	 * A zeroed page leaves them off-list with CLUSTER_FLAG_NONE, which
> +	 * is what move_cluster() expects.
> +	 */
> +	WRITE_ONCE(si->nr_mapped_clusters, first + nr);
> +	for (i = first; i < first + nr; i++) {
> +		ci = &si->cluster_info[i];
> +		spin_lock_init(&ci->lock);
> +		INIT_LIST_HEAD(&ci->list);
> +		spin_lock(&ci->lock);
> +		move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
> +		spin_unlock(&ci->lock);
>  	}

[Severity: High]
What happens if swap_cluster_readahead() observes the updated
si->nr_mapped_clusters in vswap_grow_clusters() before the loop finishes
running spin_lock_init(&ci->lock) on each new cluster?

In mm/swap_state.c, swap_cluster_readahead() bounds end_offset using
READ_ONCE(si->nr_mapped_clusters) and then locks each cluster in the range
via swap_cache_read_folio() -> __swap_cache_alloc_folio() ->
__swap_cache_alloc() -> swap_cluster_lock():

mm/swap_state.c:swap_cluster_readahead() {
	...
	max = si->max;
	if (swap_is_vswap(si))
		max = min(max, READ_ONCE(si->nr_mapped_clusters) *
			       SWAPFILE_CLUSTER);
	if (end_offset >= max)
		end_offset = max - 1;

	blk_start_plug(&plug);
	for (offset = start_offset; offset <= end_offset ; offset++) {
		/* Ok, do the async read-ahead now */
		ra_entry = swp_entry(swp_type(entry), offset);
		folio = swap_cache_read_folio(&ctx, ra_entry, gfp_mask, mpol,
				ilx, offset != entry_offset);
	...
}

When a readahead window spans across a cluster boundary into the newly grown
range [first, first + nr) (or after cluster first is added to
si->free_clusters while clusters first + 1 .. first + nr - 1 are still being
initialized), swap_cluster_readahead() can call spin_lock(&ci->lock) on
si->cluster_info[i] before spin_lock_init(&ci->lock) has executed, or while
spin_lock_init(&ci->lock) is concurrently resetting the lock.

Should vswap_grow_clusters() initialize the new clusters before publishing
si->nr_mapped_clusters with smp_store_release() (paired with
smp_load_acquire() in readers), or initialize spin_lock_init(&ci->lock) and
INIT_LIST_HEAD(&ci->list) for the entire batch before making the new clusters
visible?

[ ... ]

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