Re: [PATCH v5 17/19] mm, swap: add xswap shrink trigger and defer it to a workqueue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mm: use-after-unmap of cluster_info in swap paths when
  xswap_try_shrink() runs
- [High] mm: race in xswap_try_shrink() setting CLUSTER_FLAG_NONE
  re-adds unmapped cluster
- [Low] mm: redundant xswap_lock acquire in __swapoff() for non-existent
  sysfs shrink
--

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

mm, swap: add xswap shrink trigger and defer it to a workqueue

This commit adds deferred shrinking of the xswap cluster_info VM_SPARSE
mapping via a workqueue scheduled from __free_cluster().

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 097ed288b1ca1..2e5383036fdcb 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
[ ... ]
> @@ -3681,6 +3701,15 @@ static int __swapoff(struct swap_info_struct *p)
>  	struct file *swap_file = NULL;
>  	int err;
>  
> +#ifdef CONFIG_XSWAP
> +	if (p->flags & SWP_XSWAP) {
> +		cancel_work_sync(&p->xswap_shrink_work);
> +		/* Wait out a shrink racing us from the sysfs write path. */
> +		mutex_lock(&p->xswap_lock);
> +		mutex_unlock(&p->xswap_lock);
> +	}
> +#endif

[Severity: Low]
This isn't a bug, but is the p->xswap_lock acquisition here needed?  There
is no sysfs write path that triggers a shrink, and cancel_work_sync() has
already waited for xswap_shrink_work after SWP_WRITEOK was cleared.

[ ... ]
> @@ -4413,6 +4452,153 @@ static int xswap_mapped_end(pte_t *pte, unsigned long addr, void *data)
[ ... ]
> +	spin_lock(&si->lock);
> +	for (i = start_idx; i < nr_mapped; i++) {
> +		ci = &si->cluster_info[i];
> +		if (READ_ONCE(ci->flags) != CLUSTER_FLAG_FREE)
> +			break;
> +		if (!spin_trylock(&ci->lock)) {
> +			spin_unlock(&si->lock);
> +			goto out_unlock;
> +		}
> +		spin_unlock(&ci->lock);
> +	}
> +	if (i != nr_mapped) {
> +		spin_unlock(&si->lock);
> +		goto out_unlock;
> +	}
> +
> +	for (i = start_idx; i < nr_mapped; i++) {
> +		ci = &si->cluster_info[i];
> +		list_del_init(&ci->list);
> +		WRITE_ONCE(ci->flags, CLUSTER_FLAG_NONE);
> +	}
> +	spin_unlock(&si->lock);

[Severity: High]
Can setting ci->flags to CLUSTER_FLAG_NONE cause a concurrent reclaimer to
re-add an unmapped cluster back onto si->free_clusters?

Both swap_reclaim_full_clusters() and cluster_reclaim_range() (called from
alloc_swap_scan_cluster()) isolate a cluster with ci->flags ==
CLUSTER_FLAG_NONE under rcu_read_lock(), temporarily drop ci->lock around
__try_to_reclaim_swap(), and then re-acquire ci->lock and call
relocate_cluster():

CPU0 (kworker)                    CPU1
xswap_try_shrink()
  flush_percpu_swap_cluster(si)
  synchronize_rcu()
                                  swap_reclaim_full_clusters()
                                    rcu_read_lock()
                                    isolate_lock_cluster()
                                      /* ci->flags = CLUSTER_FLAG_NONE */
                                    spin_unlock(&ci->lock)
                                    __try_to_reclaim_swap()
                                      /* frees last slot in ci: */
                                      __free_cluster(si, ci)
                                        /* ci->table = NULL */
                                        /* ci->flags = CLUSTER_FLAG_FREE */
  spin_lock(&si->lock)
  /* sees CLUSTER_FLAG_FREE,
   * trylocks ci->lock */
  list_del_init(&ci->list)
  ci->flags = CLUSTER_FLAG_NONE
  spin_unlock(&si->lock)
  xswap_unmap_clusters_locked()
    synchronize_rcu()
                                    spin_lock(&ci->lock)
                                    if (ci->flags == CLUSTER_FLAG_NONE)
                                      relocate_cluster(si, ci)
                                        if (!ci->count &&
                                            ci->flags != CLUSTER_FLAG_FREE)
                                          free_cluster(si, ci)
                                            __free_cluster(si, ci)
                                    spin_unlock(&ci->lock)
                                    rcu_read_unlock()
    /* unmaps and frees ci pages
     * while ci is on free_clusters */  <-- use-after-free

Because xswap_try_shrink() overwrote CLUSTER_FLAG_FREE with
CLUSTER_FLAG_NONE before waiting out the RCU grace period in
xswap_unmap_clusters_locked(), relocate_cluster() sees !ci->count and
ci->flags != CLUSTER_FLAG_FREE and calls __free_cluster() a second time.
With CONFIG_DEBUG_VM=y, swap_cluster_assert_empty() immediately
dereferences the NULL ci->table; otherwise move_cluster() links ci back
onto si->free_clusters just before its backing pages are unmapped and
freed.

> +
> +	/*
> +	 * A cluster can be taken and freed between the recheck above and the
> +	 * unmap, putting it back in the per-cpu cache.  Drain and wait again.
> +	 */
> +	xswap_unmap_clusters_locked(si, start_idx, nr_unmap, true);
> +	atomic_long_inc(&si->xswap_shrinks);

[Severity: High]
Can unmapping cluster_info pages at runtime while si->users is live cause
a kernel page fault in swap paths that access si->cluster_info outside
rcu_read_lock()?

Several swap paths rely on get_swap_device() pinning si->users to keep
si->cluster_info mapped across preemptible or sleepable sections where
neither ci->lock nor rcu_read_lock() is held.  For example, in
do_swap_page() -> folio_alloc_swap() -> __swap_cache_alloc(), or in
swap_put_entries_cluster():

mm/swapfile.c:swap_put_entries_cluster() {
    ...
	swap_cluster_unlock(ci);

	if (!need_reclaim || !reclaim_cache)
		return;

	do {
		nr_reclaimed = __try_to_reclaim_swap(si, offset,
						     TTRS_UNMAPPED | TTRS_FULL);
    ...
}

Once swap_put_entries_cluster() drops ci->lock in task context outside
rcu_read_lock() after decrementing a cached slot's swap count to 0,
another CPU can reclaim the swapcache folio and free the tail cluster:

CPU0                              CPU1 (kworker)
swap_put_entries_cluster()
  /* decrements swap count to 0 */
  swap_cluster_unlock(ci)
                                  /* folio_free_swap() frees last slot
                                   * in tail cluster -> __free_cluster() */
                                  xswap_try_shrink()
                                    flush_percpu_swap_cluster(si)
                                    synchronize_rcu()
                                    /* unlinks FREE tail clusters */
                                    xswap_unmap_clusters_locked()
                                      synchronize_rcu()
                                      xswap_unmap_range()
  __try_to_reclaim_swap()
    swap_cache_get_folio()
      __swap_entry_to_cluster()
        swap_table_get(ci, ...)   <-- page fault on unmapped cluster_info

The same window exists when __swap_cache_alloc() drops ci->lock to sleep
in folio allocation and then calls spin_lock(&ci->lock), or when
swap_extend_table_alloc() sleeps in kzalloc() before locking ci->lock, if
the swap entry and its tail cluster are freed concurrently.

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