Re: [PATCH 2/2] mm/swap: publish cluster tables after reacquiring allocator locks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm/swap: NULL deref in swap_cluster_assert_empty() on stale
  reclaimer race
- [Medium] mm/swap: ABA race on cluster state check in
  swap_cluster_alloc_table()
--

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

mm/swap: publish cluster tables after reacquiring allocator locks

This commit keeps newly allocated swap cluster tables private until
swap_cluster_alloc_table() reacquires the allocator and cluster locks, and
abandons installation if a stale reclaimer returned the cluster while the
locks were dropped.

Link: https://lore.kernel.org/all/sashiko-outbox-181084@kernel.org/
Link: https://lore.kernel.org/all/sashiko-outbox-181086@kernel.org/

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 189f04d8838f3..162a73a9e6638 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -467,27 +475,49 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>  		table = kmem_cache_zalloc(swap_table_cachep, gfp);
>  	}
>  	if (!table)
> -		return -ENOMEM;
> +		goto lock;
>  
>  #ifdef CONFIG_MEMCG
>  	if (!mem_cgroup_disabled()) {
> -		VM_WARN_ON_ONCE(ci->memcg_table);
> -		ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
> -		if (!ci->memcg_table) {
> -			swap_cluster_free_count_table(table);
> -			return -ENOMEM;
> -		}
> +		memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
> +		if (!memcg_table)
> +			goto lock;
>  	}
>  #endif
>  
>  #if !SWAP_TABLE_HAS_ZEROFLAG
> -	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_count_table(table);
> -		return -ENOMEM;
> +	zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
> +	if (!zero_bitmap)
> +		goto lock;
> +#endif
> +	ret = 0;
> +
> +lock:
> +	if (!locked) {
> +		if (si) {
> +			local_lock(&percpu_swap_cluster.lock);
> +			if (!(si->flags & SWP_SOLIDSTATE))
> +				spin_lock(&si->global_cluster_lock);
> +		}
> +		spin_lock(&ci->lock);
>  	}
> +	lockdep_assert_held(&ci->lock);
> +
> +	/* A stale reclaimer may have returned the cluster to allocation. */
> +	if (ci->flags != CLUSTER_FLAG_NONE || cluster_table_is_alloced(ci))
> +		ret = -EAGAIN;

[Severity: Medium]
Can a stale reclaimer trigger a NULL pointer dereference in
swap_cluster_assert_empty() while ci->lock is dropped here?

Because ci->table now remains NULL across the sleeping allocation in
swap_cluster_populate(), a stale reclaimer that dropped ci->lock before the
cluster was freed and re-isolated can reacquire ci->lock while ci->count is 0,
ci->flags is CLUSTER_FLAG_NONE, and ci->table is NULL:

CPU0                              CPU1
                                  cluster_reclaim_range()
                                    spin_unlock(&ci->lock)
                                    /* ci is freed and table is freed */
isolate_lock_cluster()
  ci->flags = CLUSTER_FLAG_NONE
swap_cluster_populate()
  /* atomic alloc fails */
  spin_unlock(&ci->lock)
  swap_cluster_alloc_table()
                                    spin_lock(&ci->lock)
                                  ...
                                  relocate_cluster()
                                    free_cluster()
                                      __free_cluster()
                                        swap_cluster_assert_empty()
                                          __swap_table_get()
                                            <-- NULL pointer dereference

In relocate_cluster(), !ci->count and ci->flags != CLUSTER_FLAG_FREE are both
true, so it calls free_cluster() -> __free_cluster() without checking
cluster_table_is_alloced(ci).  With CONFIG_DEBUG_VM enabled,
swap_cluster_assert_empty() calls __swap_table_get(), which dereferences the
NULL ci->table.  With SWP_PAGE_DISCARD enabled, free_cluster() also schedules
a redundant discard before swap_do_scheduled_discard() calls __free_cluster()
on the tableless cluster.

Should relocate_cluster() avoid calling free_cluster() when
!cluster_table_is_alloced(ci)?

[ ... ]

> @@ -571,13 +612,9 @@ 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);
> -	if (!(si->flags & SWP_SOLIDSTATE))
> -		spin_lock(&si->global_cluster_lock);
> -	spin_lock(&ci->lock);
> -
>  	if (ret) {
> -		move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
> +		if (ret != -EAGAIN)
> +			move_cluster(si, ci, &si->free_clusters, CLUSTER_FLAG_FREE);
>  		spin_unlock(&ci->lock);
>  		return NULL;
>  	}

[Severity: Medium]
Can an ABA race on ci->flags and ci->table cause a failed allocator to put a
cluster back onto si->free_clusters while a second allocator still has it
isolated?

When CONFIG_DEBUG_VM and SWP_PAGE_DISCARD are disabled, a stale reclaimer
calling relocate_cluster() -> free_cluster() -> __free_cluster() moves the
tableless cluster back to si->free_clusters.  A second allocator can then
re-isolate it and also drop ci->lock for a sleeping table allocation:

CPU0                       CPU1                       CPU2
swap_cluster_populate()
  spin_unlock(&ci->lock)
  swap_cluster_alloc_table()
    /* alloc fails */
                           relocate_cluster()
                             ...
                             __free_cluster()
                               ci->flags =
                                 CLUSTER_FLAG_FREE
                                                      isolate_lock_cluster()
                                                        ci->flags =
                                                          CLUSTER_FLAG_NONE
                                                      swap_cluster_populate()
                                                        spin_unlock(&ci->lock)
    spin_lock(&ci->lock)
    /* flags == NONE,
     * table == NULL */
    return -ENOMEM
  move_cluster(..., FREE)  <-- puts ci on free_clusters while CPU2 owns it

When CPU0 reacquires ci->lock, ci->flags is back to CLUSTER_FLAG_NONE and
cluster_table_is_alloced(ci) is still false, so swap_cluster_alloc_table()
returns -ENOMEM instead of -EAGAIN and CPU0 moves the cluster back onto
si->free_clusters while CPU2 is still sleeping in swap_cluster_alloc_table().

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