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

[sashiko-bot@kernel.org]

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