Re: [PATCH v5 15/19] mm, swap: make xswap cluster_info accesses RCU-safe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] mm, swap: missing RCU protection in swap_put_entries_cluster()
  reclaim
- [High] mm, swap: xswap_unmap_clusters() skips RCU sync when vm_start
  >= vm_end
- [High] mm, swap: relocate_cluster() after lock drop re-adds freed
  cluster
- [High] mm, swap: unmapped xswap cluster_info access in swapin and
  table alloc paths
--

commit 167e6417672879cf6894286f3431ad8960ae663b
Author: Baoquan He <hebaoquan@kylinos.cn>

mm, swap: make xswap cluster_info accesses RCU-safe

This commit adds RCU read-side protection, per-CPU cluster cache flushing,
and acquire/release ordering on nr_clusters_mapped so that runtime xswap
shrinking can safely unmap cluster_info pages.

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 2624233b4fc28..bc30ab42203cc 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1239,6 +1239,9 @@ static unsigned long alloc_swap_scan_cluster(struct swap_info_struct *si,
>  	lockdep_assert_held(&ci->lock);
>  	VM_WARN_ON(!cluster_is_usable(ci, order));
>  
> +	/* ci is used without ci->lock; an xswap unmap waits for this. */
> +	rcu_read_lock();
> +
>  	if (end < nr_pages || ci->count + nr_pages > SWAPFILE_CLUSTER)
>  		goto out;
>  
> @@ -1267,6 +1270,7 @@ static unsigned long alloc_swap_scan_cluster(struct swap_info_struct *si,
>  out:
>  	relocate_cluster(si, ci);
>  	swap_cluster_unlock(ci);
> +	rcu_read_unlock();

[Severity: High]
Can relocate_cluster() re-add a cluster to si->free_clusters after it was
freed and isolated while ci->lock was dropped?

CPU0                                  CPU1
alloc_swap_scan_cluster()
  rcu_read_lock()
  cluster_reclaim_range()
    spin_unlock(&ci->lock)
    __try_to_reclaim_swap()
                                      /* last slot freed */
                                      __free_cluster()
                                        ci->table = NULL
                                        ci->flags = CLUSTER_FLAG_FREE
                                      /* shrink or swap_cluster_populate()
                                       * isolates ci from free_clusters */
                                      ci->flags = CLUSTER_FLAG_NONE
                                      xswap_unmap_clusters()
                                        synchronize_rcu()
    spin_lock(&ci->lock)
    /* !cluster_is_usable(ci) ->
     * *usable = false, goto out */
  relocate_cluster(si, ci)
    /* !ci->count &&
     * ci->flags != CLUSTER_FLAG_FREE */
    free_cluster(si, ci)
      __free_cluster(si, ci)          <-- NULL deref or list corruption
  swap_cluster_unlock(ci)
  rcu_read_unlock()
                                        vm_area_unmap_pages()

When __free_cluster() runs a second time on a cluster whose table is already
NULL, swap_cluster_assert_empty() dereferences NULL under CONFIG_DEBUG_VM,
and move_cluster() links the isolated cluster back onto si->free_clusters
just before its backing page is unmapped.  swap_reclaim_full_clusters() has
the same issue at the end of its loop when checking ci->flags ==
CLUSTER_FLAG_NONE without verifying cluster_table_is_alloced(ci).

[ ... ]

> @@ -1310,6 +1314,9 @@ static void swap_reclaim_full_clusters(struct swap_info_struct *si, bool force)
>  		to_scan = swap_usage_in_pages(si) / SWAPFILE_CLUSTER;
>  
>  	while ((ci = isolate_lock_cluster(si, &si->full_clusters))) {
> +		/* As in alloc_swap_scan_cluster(). */
> +		rcu_read_lock();
> +
>  		offset = cluster_offset(si, ci);
>  		end = min(si->max, offset + SWAPFILE_CLUSTER);
>  		to_scan--;
> @@ -1334,6 +1341,7 @@ static void swap_reclaim_full_clusters(struct swap_info_struct *si, bool force)
>  			relocate_cluster(si, ci);
>  
>  		swap_cluster_unlock(ci);
> +		rcu_read_unlock();

[Severity: High]
Does swap_put_entries_cluster() need the same RCU protection around its
__try_to_reclaim_swap() loop?

mm/swapfile.c:swap_put_entries_cluster() {
    ...
	if (ci_batch != -1)
		__swap_cluster_free_entries(si, ci, ci_batch, ci_off - ci_batch);
	swap_cluster_unlock(ci);

	if (!need_reclaim || !reclaim_cache)
		return;

	do {
		nr_reclaimed = __try_to_reclaim_swap(si, offset,
						     TTRS_UNMAPPED | TTRS_FULL);
		offset++;
		if (nr_reclaimed)
			offset = round_up(offset, abs(nr_reclaimed));
	} while (offset < end);
}

Callers such as shmem_free_swap() -> swap_put_entries_direct() invoke
swap_put_entries_cluster() in preemptible context outside RCU.  Once
__swap_cluster_put_entry() drops the swap count of cached slots to 0 and
swap_cluster_unlock(ci) releases ci->lock, concurrent reclaim can free the
swapcache folios and empty the cluster, allowing xswap to finish
synchronize_rcu() and unmap the cluster_info page before
__try_to_reclaim_swap() indexes si->cluster_info.

[ ... ]

> @@ -4289,12 +4333,33 @@ static void xswap_unmap_clusters(struct swap_info_struct *si,
>  
>  	mutex_lock(&si->xswap_lock);
>  
> +	/*
> +	 * Publish the smaller range before its memory goes away.  Readers
> +	 * that use nr_clusters_mapped without xswap_lock pair with this
> +	 * store using smp_load_acquire().
> +	 */
> +	smp_store_release(&si->nr_clusters_mapped, start_idx);
> +
>  	if (vm_start >= vm_end) {
> -		WRITE_ONCE(si->nr_clusters_mapped, start_idx);
> +		/*
> +		 * The clusters sit inside one page that the preceding
> +		 * cluster shares, so there is nothing to unmap.  The page
> +		 * goes away with that cluster; publishing the smaller
> +		 * cluster count is still correct.
> +		 */
>  		mutex_unlock(&si->xswap_lock);
>  		return;
>  	}

[Severity: High]
Can returning early when vm_start >= vm_end allow a subsequent xswap_grow()
to reinitialize ci->lock while an earlier RCU reader is still using it?

CPU0                                  CPU1
swap_alloc_fast()
  rcu_read_lock()
  mapped = smp_load_acquire(
             &si->nr_clusters_mapped)
                                      xswap_unmap_clusters(si, start_idx, nr)
                                        smp_store_release(
                                          &si->nr_clusters_mapped, start_idx)
                                        /* vm_start >= vm_end: skips
                                         * flush_percpu_swap_cluster() and
                                         * synchronize_rcu() */
                                      xswap_grow()
                                        xswap_map_clusters(si, start_idx, ...)
                                          /* page already mapped */
                                          spin_lock_init(&ci->lock)
  ci = swap_cluster_lock(si, offset)  <-- races with spin_lock_init()

Because struct swap_cluster_info (40 or 48 bytes) does not evenly divide
PAGE_SIZE, shrinking a range that shares its page with start_idx - 1 takes
the vm_start >= vm_end return without flushing percpu_swap_cluster or
waiting for RCU readers.  If xswap_grow() maps those clusters again while a
reader from before the shrink is still in flight (or using an unflushed
per-CPU cache entry), the mapped: path in xswap_map_clusters() runs
spin_lock_init(&ci->lock) concurrently with that reader.

>  
> +	/*
> +	 * A per-cpu cluster cache can still hold an offset in this range.
> +	 * Invalidate those references, then wait out the readers that have
> +	 * already loaded one, so that nobody can dereference cluster_info
> +	 * past this point.  swapoff() needs the same before it releases.
> +	 */
> +	flush_percpu_swap_cluster(si);
> +	synchronize_rcu();

[Severity: High]
Can swapin and extend-table allocation paths still dereference an unmapped
cluster_info page across sleeping allocations where RCU is not held?

CPU0                                  CPU1
do_swap_page()
  pte_unmap_same() /* drops PTL */
  get_swap_device(entry)
  ...
  __swap_cache_alloc()
    spin_lock(&ci->lock)
    __swap_cache_add_check()
    spin_unlock(&ci->lock)
    folio_alloc_mpol() /* sleeps */
                                      /* entry freed (e.g. MADV_DONTNEED) */
                                      __free_cluster()
                                      xswap_unmap_clusters()
                                        synchronize_rcu()
                                        vm_area_unmap_pages()
    spin_lock(&ci->lock)              <-- page fault on unmapped ci

get_swap_device() only pins si->users, which does not block runtime xswap
shrinking.  Once the PTL is dropped in do_swap_page() (or the xarray lock in
shmem_swapin_folio()), a racing thread can free the swap entry and allow
xswap_unmap_clusters() to unmap the cluster_info page while CPU0 is
preempted before swap_cache_get_folio() or sleeping in __swap_cache_alloc().
The same race affects copy_pte_range() -> swap_retry_table_alloc() ->
swap_extend_table_alloc(), which drops the PTL, performs a GFP_KERNEL
kzalloc(), and then takes spin_lock(&ci->lock).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109055455.536925-1-baoquan.he@linux.dev?part=15