Re: [PATCH v6 10/12] mm, swap: defer memcg_table allocation for physical swap clusters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm, swap: infinite loop and cluster demotion on lazy memcg_table alloc failure
- [High] mm, swap: NULL ci->table deref when lazy memcg_table alloc fails in race
--

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

mm, swap: defer memcg_table allocation for physical swap clusters

This patch defers allocating ci->memcg_table for physical swap clusters when
vswap is enabled until a direct-use swap slot is allocated in
alloc_swap_scan_cluster(). Clusters that only hold vswap backings skip
allocating ci->memcg_table altogether.

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 12cd5caeaaf23..e95f2441fdd7c 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -476,7 +476,8 @@ static void swap_cluster_free_table(struct swap_cluster_info *ci)
>  	swap_cluster_free_count_table(table);
>  }
>  
> -static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
> +static int swap_cluster_alloc_table(struct swap_info_struct *si,
> +				    struct swap_cluster_info *ci, gfp_t gfp)
>  {
>  	struct swap_table *table = NULL;
>  	struct folio *folio;
> @@ -497,7 +498,14 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
>  		return -ENOMEM;
>  
>  #ifdef CONFIG_MEMCG
> -	if (!mem_cgroup_disabled()) {
> +	/*
> +	 * A physical cluster under vswap may hold only vswap backings, which
> +	 * record their memcg on the vswap cluster's table, not this one. Such
> +	 * clusters defer memcg_table allocation until they hand out a direct-use
> +	 * slot, including one for hibernation.
> +	 */
> +	if ((!vswap_is_enabled() || swap_is_vswap(si)) &&
> +	    !mem_cgroup_disabled()) {
>  		VM_WARN_ON_ONCE(ci->memcg_table);
>  		ci->memcg_table = kzalloc_obj(*ci->memcg_table, gfp);
>  		if (!ci->memcg_table) {
> @@ -575,8 +583,8 @@ swap_cluster_populate(struct swap_info_struct *si,
>  		lockdep_assert_held(&si->global_cluster_lock);
>  	lockdep_assert_held(&ci->lock);
>  
> -	if (!swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
> -					  __GFP_NOWARN))
> +	if (!swap_cluster_alloc_table(si, ci, __GFP_HIGH | __GFP_NOMEMALLOC |
> +					      __GFP_NOWARN))
>  		return ci;
>  
>  	/*
> @@ -589,8 +597,8 @@ swap_cluster_populate(struct swap_info_struct *si,
>  		spin_unlock(&si->global_cluster_lock);
>  	local_unlock(&percpu_swap_cluster.lock);
>  
> -	ret = swap_cluster_alloc_table(ci, __GFP_HIGH | __GFP_NOMEMALLOC |
> -					   GFP_KERNEL);
> +	ret = swap_cluster_alloc_table(si, ci, __GFP_HIGH | __GFP_NOMEMALLOC |
> +					       GFP_KERNEL);
>  
>  	/*
>  	 * Back to atomic context. We might have migrated to a new CPU with a

[ ... ]

> @@ -1090,7 +1098,9 @@ static bool __swap_cluster_alloc_entries(struct swap_info_struct *si,
>  /* Try use a new cluster for current CPU and allocate from it. */
>  static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
>  					    struct swap_cluster_info *ci,
> -					    struct folio *folio, unsigned long offset)
> +					    struct folio *folio,
> +					    unsigned long offset,
> +					    bool *nomem)
>  {
>  	unsigned int next = SWAP_ENTRY_INVALID, found = SWAP_ENTRY_INVALID;
>  	unsigned long start = ALIGN_DOWN(offset, SWAPFILE_CLUSTER);
> @@ -1119,6 +1129,23 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
>  			if (!ret)
>  				continue;
>  		}
> +#ifdef CONFIG_MEMCG
> +		/*
> +		 * Lazy-allocate memcg_table on the first direct-use slot of a
> +		 * physical cluster.
> +		 */
> +		if (vswap_is_enabled() &&
> +		    (!folio || !folio_test_swapcache(folio)) &&
> +		    !mem_cgroup_disabled() && !ci->memcg_table) {
> +			ci->memcg_table = kzalloc_obj(*ci->memcg_table,
> +						      GFP_ATOMIC | __GFP_NOWARN);
> +			if (!ci->memcg_table) {
> +				if (nomem)
> +					*nomem = true;
> +				goto out;
> +			}
> +		}
> +#endif
>  		if (!__swap_cluster_alloc_entries(si, ci, folio, offset % SWAPFILE_CLUSTER))
>  			break;
>  		found = offset;
> @@ -1128,7 +1155,15 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
>  		break;
>  	}
>  out:
> -	relocate_cluster(si, ci);
> +	/*
> +	 * On a discard-capable device, relocating a cluster whose memcg_table
> +	 * allocation failed queues a discard for slots that were never used,
> +	 * which folio_alloc_phys_swap() reads as progress and retries on.
> +	 */
> +	if (nomem && *nomem && !ci->count)
> +		__free_cluster(si, ci);
> +	else
> +		relocate_cluster(si, ci);

[Severity: High]
Can a concurrent caller with nomem == NULL race with swap_cluster_populate()
here and free ci->table before swap_cluster_populate() finishes isolating the
cluster?

In mm/swapfile.c, when isolate_lock_cluster() isolates an empty physical
cluster from si->free_clusters and the initial atomic table allocation fails,
swap_cluster_populate() drops ci->lock and percpu_swap_cluster.lock to call
swap_cluster_alloc_table() with GFP_KERNEL:

    spin_unlock(&ci->lock);
    if (!(si->flags & SWP_SOLIDSTATE))
        spin_unlock(&si->global_cluster_lock);
    local_unlock(&percpu_swap_cluster.lock);

    ret = swap_cluster_alloc_table(si, ci, __GFP_HIGH | __GFP_NOMEMALLOC |
                                           GFP_KERNEL);

With this patch, swap_cluster_alloc_table() skips allocating ci->memcg_table
for physical clusters when vswap_is_enabled() is true, and publishes
rcu_assign_pointer(ci->table, table) while ci->lock is not held.

Before swap_cluster_populate() re-acquires ci->lock, another caller whose
cached percpu_swap_cluster.offset[order] or si->global_cluster->next[order]
still points into ci (such as swap_alloc_fast(), cluster_alloc_swap_entry(),
or swap_alloc_hibernation_slot()) can lock ci, see cluster_is_usable(ci,
order) == true and cluster_is_empty(ci) == true, and call
alloc_swap_scan_cluster(si, ci, folio, offset, NULL) with nomem == NULL for a
direct-use slot.

If the lazy kzalloc_obj(*ci->memcg_table, GFP_ATOMIC | __GFP_NOWARN) fails in
alloc_swap_scan_cluster(), it jumps to out with ci->count == 0. Because nomem
is NULL, the check (nomem && *nomem && !ci->count) is false, so it calls
relocate_cluster(si, ci) -> free_cluster(si, ci), which either frees
ci->table via __free_cluster() and moves ci back to si->free_clusters, or
queues ci on si->discard_clusters when SWP_PAGE_DISCARD is set.

When swap_cluster_populate() then re-acquires ci->lock, it only checks if
(ret) before returning ci to isolate_lock_cluster() -> alloc_swap_scan_list()
-> alloc_swap_scan_cluster() -> cluster_scan_range() -> __swap_table_get(),
dereferencing a NULL ci->table (or allocating from a cluster on
si->discard_clusters).

>  	swap_cluster_unlock(ci);
>  	if (swap_is_vswap(si)) {
>  		this_cpu_write(percpu_vswap_cluster.offset[order], next);
> @@ -1149,7 +1184,13 @@ static unsigned int alloc_swap_scan_list(struct swap_info_struct *si,
>  					 bool scan_all)
>  {
>  	unsigned int found = SWAP_ENTRY_INVALID;
> +	bool nomem = false;
>  
> +	/*
> +	 * In rare cases alloc_swap_scan_cluster() can fail due to
> +	 * memcg_table allocation failure. Short-circuit to avoid looping
> +	 * over the list indefinitely.
> +	 */
>  	do {
>  		struct swap_cluster_info *ci = isolate_lock_cluster(si, list);
>  		unsigned long offset;
> @@ -1157,10 +1198,10 @@ static unsigned int alloc_swap_scan_list(struct swap_info_struct *si,
>  		if (!ci)
>  			break;
>  		offset = cluster_offset(si, ci);
> -		found = alloc_swap_scan_cluster(si, ci, folio, offset);
> +		found = alloc_swap_scan_cluster(si, ci, folio, offset, &nomem);
>  		if (found)
>  			break;
> -	} while (scan_all);
> +	} while (scan_all && !nomem);
>  
>  	return found;
>  }

[Severity: High]
What happens in cluster_alloc_swap_entry() and swap_alloc_slow() when the
lazy GFP_ATOMIC allocation of ci->memcg_table fails under memory pressure?

Previously, swap_cluster_populate() had a lock-dropping GFP_KERNEL fallback
when allocating ci->memcg_table, and swapon pre-allocated ci->memcg_table
with GFP_KERNEL for cluster 0 and the bad/EOF clusters. Moving physical
cluster ci->memcg_table allocation into alloc_swap_scan_cluster() under
ci->lock and percpu_swap_cluster.lock with GFP_ATOMIC | __GFP_NOWARN appears
to cause several issues when that atomic allocation fails during direct
physical swap allocation:

1. For an empty cluster isolated from si->free_clusters,
swap_cluster_populate() has just allocated ci->table (potentially dropping
locks to use GFP_KERNEL), and alloc_swap_scan_cluster() immediately calls
__free_cluster(si, ci) -> swap_cluster_free_table(ci), freeing the newly
allocated ci->table via call_rcu() and returning ci to si->free_clusters.

2. For a non-empty cluster (ci->count > 0) isolated from
si->nonfull_clusters[order] (such as cluster 0, the EOF cluster, or a
partially filled vswap-backing cluster), the check (nomem && *nomem &&
!ci->count) in alloc_swap_scan_cluster() is false, so relocate_cluster(si,
ci) demotes the cluster from si->nonfull_clusters[order] to
si->frag_clusters[ci->order].

3. In alloc_swap_scan_list(), stopping on !nomem skips any remaining clusters
on si->nonfull_clusters[order] that already have ci->memcg_table allocated,
while nomem is not propagated to cluster_alloc_swap_entry(). As a result,
cluster_alloc_swap_entry() continues to si->free_clusters,
swap_reclaim_full_clusters(), and the order-0 higher-order stealing loop over
si->frag_clusters[o] and si->nonfull_clusters[o], demoting or fragmenting
higher-order clusters.

4. When cluster_alloc_swap_entry(si, folio) returns 0 to
swap_alloc_slow(folio) for an order-0 folio (!folio_test_large(folio)),
swap_alloc_slow() does not bail out:

    plist_for_each_entry_safe(si, next, &swap_avail_head, avail_list) {
        /* Rotate the device and switch to a new cluster */
        plist_requeue(&si->avail_list, &swap_avail_head);
        spin_unlock(&swap_avail_lock);
        if (get_swap_device_info(si)) {
            found = cluster_alloc_swap_entry(si, folio);
            put_swap_device(si);
            if (found)
                return swp_entry(si->type, found);
            if (folio_test_large(folio))
                return (swp_entry_t){};
        }

Because no slots were allocated, si is not full and stays on swap_avail_head.
When two or more swap devices share the same priority on swap_avail_head,
plist_requeue(&si->avail_list, &swap_avail_head) moves si after next on each
iteration, causing swap_alloc_slow() to loop indefinitely between those
devices while holding percpu_swap_cluster.lock.

Could swap_cluster_populate() or a lock-dropping fallback in the caller
handle allocating ci->memcg_table with GFP_KERNEL, and avoid demoting
clusters or looping in swap_alloc_slow() when allocation fails?

> @@ -1181,7 +1222,8 @@ static unsigned int vswap_alloc_cluster(struct swap_info_struct *si,

[ ... ]

>

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