Re: [RFC PATCH v3 05/18] mm/swap: change the swapon lock into a percpu rwsem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/swap: infinite loop in cluster_alloc_swap_entry() on disabled swap device
--

commit 75f9eec48d8910061cd39d390305f876926fa1d7
Author: Kairui Song <kasong@tencent.com>

mm/swap: change the swapon lock into a percpu rwsem

This commit converts swap_lock from a spinlock to a percpu rwsem named
swapon_rwsem so that readers of swap metadata can run concurrently without
contention against swapon and swapoff writers.

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index b72f4fc3e8117..4ffde6827d441 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
[ ... ]
> @@ -3152,14 +3155,14 @@ static int swap_device_disable(struct address_space *mapping,
>  
>  	end = ALIGN(si->max, SWAPFILE_CLUSTER);
>  unlock:
> -	spin_unlock(&swap_lock);
> +	percpu_up_write(&swapon_rwsem);
>  	if (err)
>  		return err;
>  
>  	del_from_avail_list(si, true);

[Severity: High]
Can clearing SWP_WRITEOK and calling percpu_up_write(&swapon_rwsem) in
mm/swapfile.c:swap_device_disable() before removing si from the available
devices in del_from_avail_list(si, true) cause a concurrent order-0 swap
allocation to enter an infinite loop in cluster_alloc_swap_entry()?

In swap_device_disable(), si->flags &= ~SWP_WRITEOK is performed under
swap_avail_lock, and then both swap_avail_lock and swapon_rwsem are released
before del_from_avail_list(si, true) removes si from swap_avail_head (or
masks si in swap_queue after the later commit "mm/swap: add priority queue
for swap device allocation", where percpu_up_write(&swapon_rwsem) also wakes
readers waiting on swapon_rwsem before del_from_avail_list() runs):

mm/swapfile.c:swap_device_disable() {
	...
	spin_lock(&swap_avail_lock);
	si->flags &= ~SWP_WRITEOK;
	spin_unlock(&swap_avail_lock);

	plist_del(&si->list, &swap_active_head);
	total_swap_pages -= si->pages;
	atomic_long_sub(si->pages, &nr_swap_pages);

	end = ALIGN(si->max, SWAPFILE_CLUSTER);
unlock:
	percpu_up_write(&swapon_rwsem);
	if (err)
		return err;

	del_from_avail_list(si, true);
}

If a concurrent order-0 folio_alloc_swap() selects si while si is still
visible as available but has SWP_WRITEOK cleared, it calls
cluster_alloc_swap_entry(si, folio).

First, alloc_swap_scan_list(si, &si->nonfull_clusters[order], folio, true)
isolates each cluster ci and sets ci->flags = CLUSTER_FLAG_NONE. Inside
alloc_swap_scan_cluster(), __swap_cluster_alloc_entries() returns false
because !(si->flags & SWP_WRITEOK), and relocate_cluster() moves every
non-full cluster onto si->frag_clusters[ci->order].

Then, for an order-0 folio, cluster_alloc_swap_entry() enters the
higher-order stealing loop:

mm/swapfile.c:cluster_alloc_swap_entry() {
	...
	/* Order 0 stealing from higher order */
	for (int o = 1; o < SWAP_NR_ORDERS; o++) {
		/*
		 * Clusters here have at least one usable slots and can't fail order 0
		 * allocation, but reclaim may drop si->lock and race with another user.
		 */
		found = alloc_swap_scan_list(si, &si->frag_clusters[o], folio, true);
		if (found)
			goto done;
		...
	}
}

And alloc_swap_scan_list() loops while scan_all is true:

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

Whenever si->frag_clusters[o] is non-empty, isolate_lock_cluster() pops ci
from si->frag_clusters[o] and sets ci->flags = CLUSTER_FLAG_NONE,
__swap_cluster_alloc_entries() fails because !(si->flags & SWP_WRITEOK), and
relocate_cluster() sees a non-empty, non-full cluster with
ci->flags != CLUSTER_FLAG_FRAG and appends ci right back onto
si->frag_clusters[o]. Because found is 0 and scan_all is true, wouldn't
alloc_swap_scan_list() spin forever on si->frag_clusters[o] (and also leave
nonfull_clusters drained into frag_clusters if swapoff later fails in
try_to_unuse() and re-enables the device)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793106426.git.lianux.mm@gmail.com?part=5